From owner-freebsd-arch@FreeBSD.ORG Mon Apr 6 19:19:25 2015 Return-Path: Delivered-To: arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 92EB1ACC; Mon, 6 Apr 2015 19:19:25 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 68AF47E5; Mon, 6 Apr 2015 19:19:25 +0000 (UTC) Received: from ralph.baldwin.cx (pool-173-54-116-245.nwrknj.fios.verizon.net [173.54.116.245]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 4C8A5B913; Mon, 6 Apr 2015 15:19:24 -0400 (EDT) From: John Baldwin To: freebsd-arch@freebsd.org Subject: Re: RFC: chgsbsize doesn't handle -ve sbsize correctly Date: Mon, 06 Apr 2015 15:11:47 -0400 Message-ID: <476184366.nOLsX7PO1g@ralph.baldwin.cx> User-Agent: KMail/4.14.2 (FreeBSD/10.1-STABLE; KDE/4.14.2; amd64; ; ) In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 06 Apr 2015 15:19:24 -0400 (EDT) Cc: "arch@FreeBSD.org" , Anuranjan Shukla , Simon Gerraty X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Apr 2015 19:19:25 -0000 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