Date: Mon, 6 Apr 2015 20:27:02 +0000 From: Anuranjan Shukla <anshukla@juniper.net> To: John Baldwin <jhb@freebsd.org>, "freebsd-arch@freebsd.org" <freebsd-arch@freebsd.org> Cc: "arch@FreeBSD.org" <arch@freebsd.org>, Simon Gerraty <sjg@juniper.net> Subject: Re: RFC: chgsbsize doesn't handle -ve sbsize correctly Message-ID: <D1483933.296F4%anshukla@juniper.net> In-Reply-To: <476184366.nOLsX7PO1g@ralph.baldwin.cx> References: <D14391A1.294AE%anshukla@juniper.net> <476184366.nOLsX7PO1g@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi John,
Correct, I'd miswrote int. I'm curious as to what point there is in
keeping sbsize signed in the first place. If we make it unsigned long, on
32 bit also you get 'more', and overflow could be detected someway like
'if (diff > 0 && new < ui_sbsize)'. I don't see much of a value with the
signed sbsize. Any particular reason you would favor otherwise?
Thanks for get back on this.
Anu
On 4/6/15, 12:11 PM, "John Baldwin" <jhb@freebsd.org> wrote:
>On Friday, April 03, 2015 07:38:59 AM Anuranjan Shukla wrote:
>> Hi,
>> In struct uidinfo{}, ui_sbsize member is an int. In a system with large
>> number of sockets owned by a user it's possible for ui_sbsize to roll
>>over
>> to a negative value. When that happens, further calls to _increase_
>>sbsize
>> don't detect this condition as they should per design. But when the
>> sockets start shutting down this condition would be detected and you'll
>> see the console prints ("negative sbsize for uid =3D") happen. As a wors=
t
>> case the console being flooded with these prints can create a DoS
>>scenario
>> (specially on single CPU systems).
>>=20
>> Regardless of the end result, the code itself needs to be fixed for
>> correctness. There are two things to note:
>> - Int doesn't seem to be the correct type for ui_sbsize. Should be a
>>u_int
>> atleast.
>> - Since there's no real prevention of the overflow as it happens, the
>> warning print isn't helpful and should be a DEBUG level log at best. If
>>we
>> change ui_sbsize to be a u_int, the negative check itself can go.
>>=20
>>=20
>> int
>> chgsbsize(uip, hiwat, to, max)
>> {
>> int diff;
>>=20
>> diff =3D to - *hiwat;
>> if (diff > 0) {
>> if (atomic_fetchadd_long(&uip->ui_sbsize, (long)diff) +
>> diff > max) {
>> <=3D=3D=3D=3D=3D=3D=3D -ve ui_sbsize goes undetected and we'll p=
ass the above
>> check=20
>> atomic_subtract_long(&uip->ui_sbsize,
>>(long)diff);
>> return (0);
>> }
>> } else {
>> atomic_add_long(&uip->ui_sbsize, (long)diff);
>> if (uip->ui_sbsize < 0)
>> printf("negative sbsize for uid =3D %d\n",
>> uip->ui_uid);
>>=20
>> <=3D=3D=3D=3D Now we'll detect, far too late, that sbsiz=
e went
>>-ve
>
>Note that ui_sbsize is long, not int. That doesn't help you on 32-bit
>platforms, but you are also stuck with 32 bits since you typically don't
>have 64-bit atomics on 32-bit platforms.
>
>Making it unsigned just means you can't detect overflow anymore. Instead
>I think it should be fixed to detect the overflow when increasing the size
>and fail then. Several nearby functions would need the same fix btw:
>
>Index: kern_resource.c
>=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>--- kern_resource.c (revision 281146)
>+++ kern_resource.c (working copy)
>@@ -1380,17 +1380,18 @@ chgproccnt(struct uidinfo *uip, int diff, rlim_t m
> int
> chgsbsize(struct uidinfo *uip, u_int *hiwat, u_int to, rlim_t max)
> {
>+ long new;
> int diff;
>=20
> diff =3D to - *hiwat;
>+ new =3D atomic_fetchadd_long(&uip->ui_sbsize, (long)diff) + diff;
> if (diff > 0) {
>- if (atomic_fetchadd_long(&uip->ui_sbsize, (long)diff) + diff > max) {
>+ if (new < 0 || new > max) {
> atomic_subtract_long(&uip->ui_sbsize, (long)diff);
> return (0);
> }
> } else {
>- atomic_add_long(&uip->ui_sbsize, (long)diff);
>- if (uip->ui_sbsize < 0)
>+ if (new < 0)
> printf("negative sbsize for uid =3D %d\n", uip->ui_uid);
> }
> *hiwat =3D to;
>
>
>--=20
>John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D1483933.296F4%anshukla>