Patchwork Fix BZ 19165 -- overflow in fread / fwrite

login
register
mail settings
Submitter Paul Pluzhnikov
Date Dec. 7, 2015, 5:21 p.m.
Message ID <CALoOobOxcxieyrfNf9Eg=wmymDyKUPZ_F+atPP+Af8dyYjez_w@mail.gmail.com>
Download mbox | patch
Permalink /patch/9919/
State New
Headers show

Comments

Paul Pluzhnikov - Dec. 7, 2015, 5:21 p.m.
Ok, let's try one more time.

Previous attempt: https://sourceware.org/ml/libc-alpha/2015-10/msg00894.html
Tested on Linux/x86_64 with 6.0.0 (r222386).

Testing with (gcc-4.8 Ubuntu 4.8.4-2ubuntu1~14.04) 4:4.8.2-1ubuntu6
fails for apparently unrelated reasons:

build/math/test-double-vlen2-wrappers.o: In function `cos_vlen2':
/glibc-git/math/../sysdeps/x86_64/fpu/test-double-vlen2-wrappers.c:24:
undefined reference to `_ZGVbN2v_cos'
/glibc-git/build-system-gcc/math/test-double-vlen2-wrappers.o: In
function `sin_vlen2':
/glibc-git/math/../sysdeps/x86_64/fpu/test-double-vlen2-wrappers.c:25:
undefined reference to `_ZGVbN2v_sin'
... etc.

Thanks,


2015-12-07  Paul Pluzhnikov  <ppluzhnikov@google.com>

        [BZ #19165]
        * misc/sys/cdefs.h (__umul_size_t_overflow): New.
        (__umul_size_t_saturated): New.
        * libio/iofread.c (_IO_fread): Protect against overflow.
        * libio/iofread_u.c (__fread_unlocked): Likewise.
        * b/libio/iofwrite.c (_IO_fwrite): Likewise.
        * libio/iofwrite_u.c (fwrite_unlocked): Likewise.
Joseph Myers - Dec. 7, 2015, 6:13 p.m.
On Mon, 7 Dec 2015, Paul Pluzhnikov wrote:

> Testing with (gcc-4.8 Ubuntu 4.8.4-2ubuntu1~14.04) 4:4.8.2-1ubuntu6
> fails for apparently unrelated reasons:
> 
> build/math/test-double-vlen2-wrappers.o: In function `cos_vlen2':
> /glibc-git/math/../sysdeps/x86_64/fpu/test-double-vlen2-wrappers.c:24:
> undefined reference to `_ZGVbN2v_cos'
> /glibc-git/build-system-gcc/math/test-double-vlen2-wrappers.o: In
> function `sin_vlen2':
> /glibc-git/math/../sysdeps/x86_64/fpu/test-double-vlen2-wrappers.c:25:
> undefined reference to `_ZGVbN2v_sin'
> ... etc.

You should raise that with Andrew Senkevich, including details of whether 
libmvec got built, what symbols it exports, the command line used for that 
link, etc.
Paul Eggert - Dec. 7, 2015, 6:52 p.m.
On 12/07/2015 09:21 AM, Paul Pluzhnikov wrote:

> +#include <stddef.h>  /* for size_t.  */

Won't this pollute the namespace?  If not, please add a comment 
explaining why not.

> +__always_inline
> +static int
> +__umul_size_t_overflow (size_t a, size_t b)
> +{
> +#if __GNUC_PREREQ (5, 0)
> +  size_t result;
> +
> +  /* _Static_assert, but without triggering conformance test failures.  */

Can this problem, whatever it is, be fixed by altering _Static_assert in 
cdefs.h, rather than avoiding use of _Static_assert?

> +  __attribute__ ((__unused__))
> +  char sizeof_size_t_eq_sizeof_ulong[
> +    sizeof (size_t) == sizeof (unsigned long) ? 1 : -1];
> +
> +  return __builtin_umull_overflow (a, b, &result);

This should use __builtin_mul_overflow instead, so that you don't need 
to worry about checking whether size_t is the same width as unsigned long.

> +#else
> +  const size_t half_size_t = (size_t) 1 << 4 * sizeof (size_t);
> +  const size_t size_max = (size_t) -1;

The 4 is a mystery constant.  Better would be to declare half_size_t = 
size_max / 2 + 1.

Generally speaking let's not bother adding 'const' to locals that aren't 
later assigned to, as these 'const's are more trouble than they're 
worth.  (Similarly for 'register' and locals whose addresses are not taken.)

All the locals need to be protected with leading "__", since I assume 
this .h file can be included from user code.

Defining otherwise-unused static functions in headers will lead to 
problems with picky compilers.


The above comments suggest that this patch is part of a larger 
maintenance problem with glibc and integer overflow checking.  To 
simplify maintenance, I suggest instead that we leave cdefs.h alone, and 
instead copy and include gnulib's intprops.h for internal use only 
inside glibc, and use intprops.h's macro 'INT_MULTIPLY_OVERFLOW (a, b)' 
instead of defining and using '__umul_size_t_overflow (a, b)'.  This 
would simplify maintenance in the long run, as it already incorporates 
all the above comments.  Please see:

http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/intprops.h
Florian Weimer - Dec. 7, 2015, 7:14 p.m.
On 12/07/2015 06:21 PM, Paul Pluzhnikov wrote:
> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
> index 99e94cc..4474f09 100644
> --- a/misc/sys/cdefs.h
> +++ b/misc/sys/cdefs.h

I think it's better for now not to put this into an installed header
file.  include/stdlib.h  comes to my mind as a candidate location.

Then you can use _Static_assert and don't have to worry about including
<stddef.h>.

Florian
Paul Pluzhnikov - Dec. 10, 2015, 7:43 p.m.
On Mon, Dec 7, 2015 at 10:52 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:

> The above comments suggest that this patch is part of a larger maintenance
> problem with glibc and integer overflow checking.  To simplify maintenance,
> I suggest instead that we leave cdefs.h alone, and instead copy and include
> gnulib's intprops.h for internal use only inside glibc, and use intprops.h's
> macro 'INT_MULTIPLY_OVERFLOW (a, b)' instead of defining and using
> '__umul_size_t_overflow (a, b)'.  This would simplify maintenance in the
> long run, as it already incorporates all the above comments.  Please see:
>
> http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/intprops.h

Sigh. A "simple" overflow check in _IO_fread turned into a multi-month
ordeal ...

While I agree that what you are proposing is probably better, I am not
currently prepared to undertake that change. I'll un-assign BZ19165
from myself instead.
Florian Weimer - Dec. 10, 2015, 7:49 p.m.
On 12/10/2015 08:43 PM, Paul Pluzhnikov wrote:
> On Mon, Dec 7, 2015 at 10:52 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> 
>> The above comments suggest that this patch is part of a larger maintenance
>> problem with glibc and integer overflow checking.  To simplify maintenance,
>> I suggest instead that we leave cdefs.h alone, and instead copy and include
>> gnulib's intprops.h for internal use only inside glibc, and use intprops.h's
>> macro 'INT_MULTIPLY_OVERFLOW (a, b)' instead of defining and using
>> '__umul_size_t_overflow (a, b)'.  This would simplify maintenance in the
>> long run, as it already incorporates all the above comments.  Please see:
>>
>> http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/intprops.h
> 
> Sigh. A "simple" overflow check in _IO_fread turned into a multi-month
> ordeal ...

I don't think we want to import intprops.h because it is totally opaque.

> While I agree that what you are proposing is probably better, I am not
> currently prepared to undertake that change. I'll un-assign BZ19165
> from myself instead.

Do you mind if I try to move this forward?

Thanks,
Florian
Paul Pluzhnikov - Dec. 10, 2015, 7:53 p.m.
On Thu, Dec 10, 2015 at 11:49 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 12/10/2015 08:43 PM, Paul Pluzhnikov wrote:

>> I'll un-assign BZ19165 from myself instead.
>
> Do you mind if I try to move this forward?

Not at all (I thought that's exactly what un-assigning myself means:
let someone else take a stab ;-)

Thanks!
Rich Felker - Feb. 11, 2016, 2:26 a.m.
On Thu, Dec 10, 2015 at 11:53:04AM -0800, Paul Pluzhnikov wrote:
> On Thu, Dec 10, 2015 at 11:49 AM, Florian Weimer <fweimer@redhat.com> wrote:
> > On 12/10/2015 08:43 PM, Paul Pluzhnikov wrote:
> 
> >> I'll un-assign BZ19165 from myself instead.
> >
> > Do you mind if I try to move this forward?
> 
> Not at all (I thought that's exactly what un-assigning myself means:
> let someone else take a stab ;-)

I think the problem may be even worse than we all expected. I've been
trying to fix the corresponding issue in musl, and it looks like the
_kernel_ is spuriously failing these reads with EFAULT by pre-checking
the validity of the potential destination address range rather than
only checking if there would actually be data to copy. I haven't yet
dug into the kernel sources to figure out why this is happening but
read(2), readv(2), pread(2), etc. are probably all affected and I'm
skeptical of whether it makes sense to try to work around this in
libc. We should probably seek clarificatin from the Austin Group on
whether those interfaces are intended to have well-defined behavior
when the nbytes argument is greater than the size of the buffer. For
fread it's WG14's domain and getting a good answer from them on
whether invalid size yields UB is probably going to be difficult...

Rich
Florian Weimer - Feb. 11, 2016, 12:22 p.m.
On 02/11/2016 03:26 AM, Rich Felker wrote:
> I think the problem may be even worse than we all expected. I've been
> trying to fix the corresponding issue in musl, and it looks like the
> _kernel_ is spuriously failing these reads with EFAULT by pre-checking
> the validity of the potential destination address range rather than
> only checking if there would actually be data to copy.

Yes, system call behavior in this area is fairly regular: if a memory
region is passed, it is checked for validity as a whole, and not just
for the parts that are actually needed.  By now, this is part of the
user space interface, and probably cannot change without breaking
backwards compatibility.

Florian
Zack Weinberg - Feb. 11, 2016, 2:07 p.m.
(sorry for the malformatted message earlier)

On Thu, Feb 11, 2016 at 7:22 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 02/11/2016 03:26 AM, Rich Felker wrote:
> > I think the problem may be even worse than we all expected. I've been
> > trying to fix the corresponding issue in musl, and it looks like the
> > _kernel_ is spuriously failing these reads with EFAULT by pre-checking
> > the validity of the potential destination address range rather than
> > only checking if there would actually be data to copy.
>
> Yes, system call behavior in this area is fairly regular: if a memory
> region is passed, it is checked for validity as a whole, and not just
> for the parts that are actually needed.  By now, this is part of the
> user space interface, and probably cannot change without breaking
> backwards compatibility.

Backward compatibility doesn't seem like a strong argument to me --
changing this would only make programs that don't work now, start
working.  It would be much more problematic the other way around.

However, it seems to me there's a serious implementation-level
obstacle: for at least some device I/O, the kernel may need to
finalize memory access checks before it knows how much data is
available. And it would be very confusing if the behavior depended on
what type of fd you were using.

zw
Rich Felker - Feb. 11, 2016, 3:30 p.m.
On Thu, Feb 11, 2016 at 08:50:58AM -0500, Zack Weinberg wrote:
> On Feb 11, 2016 7:22 AM, "Florian Weimer" <fweimer@redhat.com> wrote:
> >
> > On 02/11/2016 03:26 AM, Rich Felker wrote:
> > > I think the problem may be even worse than we all expected. I've been
> > > trying to fix the corresponding issue in musl, and it looks like the
> > > _kernel_ is spuriously failing these reads with EFAULT by pre-checking
> > > the validity of the potential destination address range rather than
> > > only checking if there would actually be data to copy.
> >
> > Yes, system call behavior in this area is fairly regular: if a memory
> > region is passed, it is checked for validity as a whole, and not just
> > for the parts that are actually needed.  By now, this is part of the
> > user space interface, and probably cannot change without breaking
> > backwards compatibility.
> 
> Also, the kernel might need to finalize access checks and wire down the
> pages for DMA before it even knows how much data is available.

That makes no sense except for O_DIRECT wackiness where I doubt anyone
cares about correctness/standards. Normal file reads should always be
DMA into the fs cache followed by memcpy to the caller's buffer.

Rich
Zack Weinberg - Feb. 11, 2016, 3:58 p.m.
On Thu, Feb 11, 2016 at 10:30 AM, Rich Felker <dalias@libc.org> wrote:
> On Thu, Feb 11, 2016 at 08:50:58AM -0500, Zack Weinberg wrote:
>>
>> [...] the kernel might need to finalize access checks and wire down the
>> pages for DMA before it even knows how much data is available.
>
> That makes no sense except for O_DIRECT wackiness where I doubt anyone
> cares about correctness/standards. Normal file reads should always be
> DMA into the fs cache followed by memcpy to the caller's buffer.

I was thinking of character devices and maybe also sockets.  Those don't
go through the fs cache.

And I don't think anyone wants the behavior to vary depending on which
kind of fd you have passed to read().

zw
Paul Eggert - Feb. 11, 2016, 5:57 p.m.
On 02/11/2016 07:58 AM, Zack Weinberg wrote:
> I don't think anyone wants the behavior to vary depending on which
> kind of fd you have passed to read().

As an application developer I'd rather have 'read', 'fread', etc. fail, 
or even dump core, if I give them a buffer that is not entirely valid. I 
hope, for example, that checkers like valgrind object to this sort of 
thing, and would find it useful if glibc continues to report it as an 
error. If POSIX really requires that 'read' and 'fread' must succeed in 
this situation, then from my point of view it's a POSIX bug that needs 
to get fixed, as the benefit of the extra checking is significant.

Patch

diff --git a/libio/iofread.c b/libio/iofread.c
index eb69b05..515f862 100644
--- a/libio/iofread.c
+++ b/libio/iofread.c
@@ -29,7 +29,7 @@ 
 _IO_size_t
 _IO_fread (void *buf, _IO_size_t size, _IO_size_t count, _IO_FILE *fp)
 {
-  _IO_size_t bytes_requested = size * count;
+  _IO_size_t bytes_requested = __umul_size_t_saturated (size, count);
   _IO_size_t bytes_read;
   CHECK_FILE (fp, 0);
   if (bytes_requested == 0)
diff --git a/libio/iofread_u.c b/libio/iofread_u.c
index 997b714..9e2d213 100644
--- a/libio/iofread_u.c
+++ b/libio/iofread_u.c
@@ -32,7 +32,7 @@ 
 _IO_size_t
 __fread_unlocked (void *buf, _IO_size_t size, _IO_size_t count, _IO_FILE *fp)
 {
-  _IO_size_t bytes_requested = size * count;
+  _IO_size_t bytes_requested = __umul_size_t_saturated (size, count);
   _IO_size_t bytes_read;
   CHECK_FILE (fp, 0);
   if (bytes_requested == 0)
diff --git a/libio/iofwrite.c b/libio/iofwrite.c
index 48ad4bc..bb34257 100644
--- a/libio/iofwrite.c
+++ b/libio/iofwrite.c
@@ -29,7 +29,7 @@ 
 _IO_size_t
 _IO_fwrite (const void *buf, _IO_size_t size, _IO_size_t count, _IO_FILE *fp)
 {
-  _IO_size_t request = size * count;
+  _IO_size_t request = __umul_size_t_saturated (size, count);
   _IO_size_t written = 0;
   CHECK_FILE (fp, 0);
   if (request == 0)
diff --git a/libio/iofwrite_u.c b/libio/iofwrite_u.c
index 2b1c47a..6e002bf 100644
--- a/libio/iofwrite_u.c
+++ b/libio/iofwrite_u.c
@@ -33,7 +33,7 @@  _IO_size_t
 fwrite_unlocked (const void *buf, _IO_size_t size, _IO_size_t count,
 		 _IO_FILE *fp)
 {
-  _IO_size_t request = size * count;
+  _IO_size_t request = __umul_size_t_saturated (size, count);
   _IO_size_t written = 0;
   CHECK_FILE (fp, 0);
   if (request == 0)
diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index 99e94cc..4474f09 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -441,4 +441,37 @@ 
 # endif
 #endif
 
+#include <stddef.h>  /* for size_t.  */
+
+__always_inline
+static int
+__umul_size_t_overflow (size_t a, size_t b)
+{
+#if __GNUC_PREREQ (5, 0)
+  size_t result;
+
+  /* _Static_assert, but without triggering conformance test failures.  */
+  __attribute__ ((__unused__))
+  char sizeof_size_t_eq_sizeof_ulong[
+    sizeof (size_t) == sizeof (unsigned long) ? 1 : -1];
+
+  return __builtin_umull_overflow (a, b, &result);
+#else
+  const size_t half_size_t = (size_t) 1 << 4 * sizeof (size_t);
+  const size_t size_max = (size_t) -1;
+
+  return __glibc_unlikely ((a | b) >= half_size_t) && b > 1 && a > size_max / b;
+#endif
+}
+
+__always_inline
+static size_t
+__umul_size_t_saturated (size_t a, size_t b)
+{
+  const size_t size_max = (size_t) -1;
+
+  if (__umul_size_t_overflow (a, b)) return size_max;
+  return a * b;
+}
+
 #endif	 /* sys/cdefs.h */