Date: Mon, 06 Apr 2015 15:11:47 -0400 From: John Baldwin <jhb@freebsd.org> To: freebsd-arch@freebsd.org Cc: "arch@FreeBSD.org" <arch@freebsd.org>, Anuranjan Shukla <anshukla@juniper.net>, Simon Gerraty <sjg@juniper.net> Subject: Re: RFC: chgsbsize doesn't handle -ve sbsize correctly Message-ID: <476184366.nOLsX7PO1g@ralph.baldwin.cx> In-Reply-To: <D14391A1.294AE%anshukla@juniper.net> References: <D14391A1.294AE%anshukla@juniper.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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 =") happen. As a worst
> case the console being flooded with these prints can create a DoS scenario
> (specially on single CPU systems).
>
> 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.
>
>
> int
> chgsbsize(uip, hiwat, to, max)
> {
> int diff;
>
> diff = to - *hiwat;
> if (diff > 0) {
> if (atomic_fetchadd_long(&uip->ui_sbsize, (long)diff) +
> diff > max) {
> <======= -ve ui_sbsize goes undetected and we'll pass the above
> check
> 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 = %d\n",
> uip->ui_uid);
>
> <==== Now we'll detect, far too late, that sbsize 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
===================================================================
--- 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;
diff = to - *hiwat;
+ new = 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 = %d\n", uip->ui_uid);
}
*hiwat = to;
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?476184366.nOLsX7PO1g>