Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check

["Carlos O'Donell" <carlos at redhat dot com>]

On 04/13/2013 08:47 PM, KOSAKI Motohiro wrote:
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
> ---
>  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  <kosaki.motohiro@gmail.com>
> +
> +	* 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  <kosaki.motohiro@gmail.com>
>  
>  	* 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
>     <http://www.gnu.org/licenses/>.  */
>  
> +#include <sys/types.h>
>  #include <sys/select.h>
>  
>  
> @@ -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.