RFC: chgsbsize doesn't handle -ve sbsize correctly

[John Baldwin]

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