From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17110 invoked by alias); 1 May 2013 02:42:09 -0000 Mailing-List: contact libc-ports-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: libc-ports-owner@sourceware.org Received: (qmail 17093 invoked by uid 89); 1 May 2013 02:42:09 -0000 X-Spam-SWARE-Status: No, score=-7.2 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS,TW_FD autolearn=ham version=3.3.1 X-Spam-User: qpsmtpd, 2 recipients Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 01 May 2013 02:42:08 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r412g6P5021240 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 30 Apr 2013 22:42:06 -0400 Received: from [10.3.113.84] (ovpn-113-84.phx2.redhat.com [10.3.113.84]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r412g5Zn029326; Tue, 30 Apr 2013 22:42:05 -0400 Message-ID: <518080FD.1090402@redhat.com> Date: Wed, 01 May 2013 02:42:00 -0000 From: "Carlos O'Donell" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: KOSAKI Motohiro CC: libc-alpha@sourceware.org, libc-ports@sourceware.org Subject: Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check References: <1365900451-19026-1-git-send-email-kosaki.motohiro@gmail.com> <1365900451-19026-3-git-send-email-kosaki.motohiro@gmail.com> In-Reply-To: <1365900451-19026-3-git-send-email-kosaki.motohiro@gmail.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-05/txt/msg00001.txt.bz2 On 04/13/2013 08:47 PM, KOSAKI Motohiro wrote: > Signed-off-by: KOSAKI Motohiro > --- > ChangeLog | 14 ++++++++++++++ > bits/select.h | 6 +++--- > debug/Versions | 3 +++ > debug/fdelt_chk.c | 11 +++++++++++ > misc/bits/select2.h | 23 +++++++++++++---------- > misc/sys/select.h | 2 +- > sysdeps/x86/bits/select.h | 6 +++--- > 7 files changed, 48 insertions(+), 17 deletions(-) > > diff --git a/ChangeLog b/ChangeLog > index 5311919..36bac6a 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,17 @@ > +2013-04-13 KOSAKI Motohiro > + > + * misc/sys/select.h (__FD_ELT): Add fdset new argument. > + * bits/select.h (__FD_SET, __FD_CLR, __FD_ISSET): Adapt > + an above change. > + * sysdeps/x86/bits/select.h (__FD_SET, __FD_CLR, __FD_ISSET): > + Likewise. > + > + * debug/fdelt_chk.c (__fdelt_buffer_chk): New. > + * misc/bits/select2.h (__FD_ELT): Implement correct buffer > + overflow check instead of hardcoded FD_SET_SIZE. > + > + * debug/Versions: Added __fdelt_buffer_chk and __fdelt_buffer_warn. > + > 2013-03-25 KOSAKI Motohiro > > * debug/fdelt_chk.c (__fdelt_chk): Removed range check > diff --git a/bits/select.h b/bits/select.h > index ca87676..a10e18a 100644 > --- a/bits/select.h > +++ b/bits/select.h > @@ -30,8 +30,8 @@ > __FDS_BITS (__arr)[__i] = 0; \ > } while (0) > #define __FD_SET(d, s) \ > - ((void) (__FDS_BITS (s)[__FD_ELT(d)] |= __FD_MASK(d))) > + ((void) (__FDS_BITS (s)[__FD_ELT(d, s)] |= __FD_MASK(d))) > #define __FD_CLR(d, s) \ > - ((void) (__FDS_BITS (s)[__FD_ELT(d)] &= ~__FD_MASK(d))) > + ((void) (__FDS_BITS (s)[__FD_ELT(d, s)] &= ~__FD_MASK(d))) > #define __FD_ISSET(d, s) \ > - ((__FDS_BITS (s)[__FD_ELT (d)] & __FD_MASK (d)) != 0) > + ((__FDS_BITS (s)[__FD_ELT (d, s)] & __FD_MASK (d)) != 0) > diff --git a/debug/Versions b/debug/Versions > index c1722fa..1ef2994 100644 > --- a/debug/Versions > +++ b/debug/Versions > @@ -55,6 +55,9 @@ libc { > GLIBC_2.16 { > __poll_chk; __ppoll_chk; > } > + GLIBC_2.18 { > + __fdelt_buffer_chk; __fdelt_buffer_warn; > + } > GLIBC_PRIVATE { > __fortify_fail; > } > diff --git a/debug/fdelt_chk.c b/debug/fdelt_chk.c > index 6588be0..8e16dc5 100644 > --- a/debug/fdelt_chk.c > +++ b/debug/fdelt_chk.c > @@ -15,6 +15,7 @@ > License along with the GNU C Library; if not, see > . */ > > +#include > #include > > > @@ -25,3 +26,13 @@ __fdelt_nochk (long int d) > } > strong_alias (__fdelt_nochk, __fdelt_chk) > strong_alias (__fdelt_nochk, __fdelt_warn) > + > +long int > +__fdelt_buffer_chk (long int fd, size_t buflen) > +{ > + if (fd / 8 >= buflen) > + __chk_fail (); > + > + return fd / __NFDBITS;; > +} > +strong_alias (__fdelt_buffer_chk, __fdelt_buffer_warn) > diff --git a/misc/bits/select2.h b/misc/bits/select2.h > index 03558c9..a0ffbb7 100644 > --- a/misc/bits/select2.h > +++ b/misc/bits/select2.h > @@ -21,15 +21,18 @@ > #endif > > /* Helper functions to issue warnings and errors when needed. */ > -extern long int __fdelt_chk (long int __d); > -extern long int __fdelt_warn (long int __d) > +extern long int __fdelt_buffer_chk (long int __d, size_t buflen); > +extern long int __fdelt_buffer_warn (long int __d, size_t buflen) > __warnattr ("bit outside of fd_set selected"); > + > #undef __FD_ELT > -#define __FD_ELT(d) \ > - __extension__ \ > - ({ long int __d = (d); \ > - (__builtin_constant_p (__d) \ > - ? (0 <= __d && __d < __FD_SETSIZE \ > - ? (__d / __NFDBITS) \ > - : __fdelt_warn (__d)) \ > - : __fdelt_chk (__d)); }) > +#define __FD_ELT(d, s) \ > + __extension__ \ > + ({ \ > + long int __d = (d); \ > + (__bos0 (s) != (size_t) -1) \ > + ? (__builtin_constant_p (__d) && ((__d / 8) >= __bos0 (s))) \ > + ? __fdelt_buffer_warn(__d, __bos0 (s)) \ Space between function and bracket e.g. foo () not foo(). > + : __fdelt_buffer_chk(__d, __bos0 (s)) \ > + : __d / __NFDBITS; \ I'm not happy that this isn't very conservative. If __bos0 fails should we fall back to static FD_SETSIZE checking e.g. "__fdelt_buffer_warn (__d, FD_SETSIZE)"? It seems that that would be better than no checking. I know why you want to fall back to no check, because that way you don't require any kind of new flag to disable the check in the event it triggers when you don't want it to (when __bos0 fails). Does compiling ruby (or similar code) with this header result in calls to __fdelt_buffer_warn or __fdelt_buffer_chk? > + }) > diff --git a/misc/sys/select.h b/misc/sys/select.h > index 21351fe..341190c 100644 > --- a/misc/sys/select.h > +++ b/misc/sys/select.h > @@ -57,7 +57,7 @@ typedef long int __fd_mask; > #undef __NFDBITS > /* It's easier to assume 8-bit bytes than to get CHAR_BIT. */ > #define __NFDBITS (8 * (int) sizeof (__fd_mask)) > -#define __FD_ELT(d) ((d) / __NFDBITS) > +#define __FD_ELT(d, s) ((d) / __NFDBITS) > #define __FD_MASK(d) ((__fd_mask) 1 << ((d) % __NFDBITS)) > > /* fd_set for select and pselect. */ > diff --git a/sysdeps/x86/bits/select.h b/sysdeps/x86/bits/select.h > index 8b87188..b29f301 100644 > --- a/sysdeps/x86/bits/select.h > +++ b/sysdeps/x86/bits/select.h > @@ -56,8 +56,8 @@ > #endif /* GNU CC */ > > #define __FD_SET(d, set) \ > - ((void) (__FDS_BITS (set)[__FD_ELT (d)] |= __FD_MASK (d))) > + ((void) (__FDS_BITS (set)[__FD_ELT (d, set)] |= __FD_MASK (d))) > #define __FD_CLR(d, set) \ > - ((void) (__FDS_BITS (set)[__FD_ELT (d)] &= ~__FD_MASK (d))) > + ((void) (__FDS_BITS (set)[__FD_ELT (d, set)] &= ~__FD_MASK (d))) > #define __FD_ISSET(d, set) \ > - ((__FDS_BITS (set)[__FD_ELT (d)] & __FD_MASK (d)) != 0) > + ((__FDS_BITS (set)[__FD_ELT (d, set)] & __FD_MASK (d)) != 0) > Cheers, Carlos.