| Message ID | 87d0izwenl.fsf@oldenburg2.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Linux: Adjust gedents64 buffer size to int range [BZ #24740] | expand |
On 27/06/2019 06:39, Florian Weimer wrote: > The kernel interface uses type unsigned int, but there is an > internal conversion to int, so INT_MAX is the correct limit. > Part of the buffer will always be unused, but this is not a > problem. Such huge buffers do not occur in practice anyway. > > 2019-06-27 Florian Weimer <fweimer@redhat.com> > > [BZ #24740] > * sysdeps/unix/sysv/linux/getdents64.c (__getdents64): Adjust > buffer size if necessary. > * sysdeps/unix/sysv/linux/mips/mips64/getdents64.c (__getdents64): > Likewise. > * sysdeps/unix/sysv/linux/tst-getdents64.c (large_buffer_check): > New function. > (large_buffer_checks): Likewise. > (do_test): Call large_buffer_checks. LGTM, maybe with a suggestion below for the tests. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > diff --git a/sysdeps/unix/sysv/linux/getdents64.c b/sysdeps/unix/sysv/linux/getdents64.c > index a6dd22106d..5e3ef9994e 100644 > --- a/sysdeps/unix/sysv/linux/getdents64.c > +++ b/sysdeps/unix/sysv/linux/getdents64.c > @@ -19,11 +19,16 @@ > #include <string.h> > #include <dirent.h> > #include <errno.h> > +#include <limits.h> > > /* The kernel struct linux_dirent64 matches the 'struct dirent64' type. */ > ssize_t > __getdents64 (int fd, void *buf, size_t nbytes) > { > + /* The system call takes an unsigned int argument, and some length > + checks in the kernel use an int type. */ > + if (nbytes > INT_MAX) > + nbytes = INT_MAX; > return INLINE_SYSCALL_CALL (getdents64, fd, buf, nbytes); > } > libc_hidden_def (__getdents64) Ok. > diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c > index 1e22fa4325..8bf3abb0e0 100644 > --- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c > +++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c > @@ -23,12 +23,18 @@ > #include <sys/param.h> > #include <unistd.h> > #include <scratch_buffer.h> > +#include <limits.h> > > ssize_t > __getdents64 (int fd, void *buf0, size_t nbytes) > { > char *buf = buf0; > > + /* The system call takes an unsigned int argument, and some length > + checks in the kernel use an int type. */ > + if (nbytes > INT_MAX) > + nbytes = INT_MAX; > + > #ifdef __NR_getdents64 > ssize_t ret = INLINE_SYSCALL_CALL (getdents64, fd, buf, nbytes); > if (ret != -1) Ok. > diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c > index c1f7721221..ece46123f3 100644 > --- a/sysdeps/unix/sysv/linux/tst-getdents64.c > +++ b/sysdeps/unix/sysv/linux/tst-getdents64.c > @@ -19,6 +19,7 @@ > #include <dirent.h> > #include <errno.h> > #include <fcntl.h> > +#include <limits.h> > #include <stdbool.h> > #include <stdio.h> > #include <stdlib.h> > @@ -28,6 +29,48 @@ > #include <support/xunistd.h> > #include <unistd.h> > > +/* Called by large_buffer_checks below. */ > +static void > +large_buffer_check (int fd, char *large_buffer, size_t large_buffer_size) > +{ > + xlseek (fd, 0, SEEK_SET); > + ssize_t ret = getdents64 (fd, large_buffer, large_buffer_size); > + if (ret < 0) > + FAIL_EXIT1 ("getdents64 for buffer of %zu bytes failed: %m", > + large_buffer_size); > + if (ret < offsetof (struct dirent64, d_name)) > + FAIL_EXIT1 ("getdents64 for buffer of %zu returned small value %zd", > + large_buffer_size, ret); > +} > + > +/* Bug 24740: Make sure that the system call argument is adjusted > + properly for the int type. A large value should stay a large > + value, and not wrap around to something small, causing the system > + call to fail with EINVAL. */ > +static void > +large_buffer_checks (int fd) > +{ > + size_t large_buffer_size = UINT_MAX; > + large_buffer_size += 2; > + if (large_buffer_size > 2) > + { Maybe size_t large_buffer_size; if (!__builtin_add_overflow (UINT_MAX, 2, &large_buffer_size)) { Instead? > + char *large_buffer = malloc (large_buffer_size); > + if (large_buffer == NULL) > + printf ("warning: could not allocate %zu bytes of memory," > + " subtests skipped\n", large_buffer_size); > + else > + { > + large_buffer_check (fd, large_buffer, INT_MAX); > + large_buffer_check (fd, large_buffer, (size_t) INT_MAX + 1); > + large_buffer_check (fd, large_buffer, (size_t) INT_MAX + 2); > + large_buffer_check (fd, large_buffer, UINT_MAX); > + large_buffer_check (fd, large_buffer, (size_t) UINT_MAX + 1); > + large_buffer_check (fd, large_buffer, (size_t) UINT_MAX + 2); > + } > + free (large_buffer); > + } > +} > + > static int > do_test (void) > { > @@ -105,6 +148,8 @@ do_test (void) > rewinddir (reference); > } > > + large_buffer_checks (fd); > + > xclose (fd); > closedir (reference); > return 0; > Ok.
* Adhemerval Zanella: > Maybe > > size_t large_buffer_size; > if (!__builtin_add_overflow (UINT_MAX, 2, &large_buffer_size)) > { > > Instead? Sure, make sense. Re-tested and pushed with that change. Thanks, Florian
27.06.2019 15:09 Florian Weimer <fweimer@redhat.com> wrote: > [...] > Sure, make sense. Re-tested and pushed with that change. > > Thanks, > Florian Florian, the test for this patch always fails with timeout on my machine, Fedora 30 and I don't think it's the CPU too slow. I did not have time to investigate this thoroughly. I am going to do it later but I'd like to give this feedback already. Regards, Rafal
* Rafal Luzynski: > 27.06.2019 15:09 Florian Weimer <fweimer@redhat.com> wrote: >> [...] >> Sure, make sense. Re-tested and pushed with that change. >> >> Thanks, >> Florian > > Florian, the test for this patch always fails with timeout on > my machine, Fedora 30 and I don't think it's the CPU too slow. > I did not have time to investigate this thoroughly. I am going > to do it later but I'd like to give this feedback already. Ah. I see. It's the test harness asking malloc to perform a memset: #0 __memset_avx2_erms () at ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:151 #1 0x00007ffff7a9c86c in alloc_perturb (n=<optimized out>, p=<optimized out>) at malloc.c:1869 #2 _int_malloc (av=av@entry=0x7ffff7dd0b80 <main_arena>, bytes=bytes@entry=4294967297) at malloc.c:4143 #3 0x00007ffff7a9dd04 in __GI___libc_malloc (bytes=4294967297) at malloc.c:3058 Presumably I could replace it with mmap to avoid this. I think the last time something similar came up, we decided to retain MALLOC_PERTURB_. Thanks, Florian
diff --git a/sysdeps/unix/sysv/linux/getdents64.c b/sysdeps/unix/sysv/linux/getdents64.c index a6dd22106d..5e3ef9994e 100644 --- a/sysdeps/unix/sysv/linux/getdents64.c +++ b/sysdeps/unix/sysv/linux/getdents64.c @@ -19,11 +19,16 @@ #include <string.h> #include <dirent.h> #include <errno.h> +#include <limits.h> /* The kernel struct linux_dirent64 matches the 'struct dirent64' type. */ ssize_t __getdents64 (int fd, void *buf, size_t nbytes) { + /* The system call takes an unsigned int argument, and some length + checks in the kernel use an int type. */ + if (nbytes > INT_MAX) + nbytes = INT_MAX; return INLINE_SYSCALL_CALL (getdents64, fd, buf, nbytes); } libc_hidden_def (__getdents64) diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c index 1e22fa4325..8bf3abb0e0 100644 --- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c +++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c @@ -23,12 +23,18 @@ #include <sys/param.h> #include <unistd.h> #include <scratch_buffer.h> +#include <limits.h> ssize_t __getdents64 (int fd, void *buf0, size_t nbytes) { char *buf = buf0; + /* The system call takes an unsigned int argument, and some length + checks in the kernel use an int type. */ + if (nbytes > INT_MAX) + nbytes = INT_MAX; + #ifdef __NR_getdents64 ssize_t ret = INLINE_SYSCALL_CALL (getdents64, fd, buf, nbytes); if (ret != -1) diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c index c1f7721221..ece46123f3 100644 --- a/sysdeps/unix/sysv/linux/tst-getdents64.c +++ b/sysdeps/unix/sysv/linux/tst-getdents64.c @@ -19,6 +19,7 @@ #include <dirent.h> #include <errno.h> #include <fcntl.h> +#include <limits.h> #include <stdbool.h> #include <stdio.h> #include <stdlib.h> @@ -28,6 +29,48 @@ #include <support/xunistd.h> #include <unistd.h> +/* Called by large_buffer_checks below. */ +static void +large_buffer_check (int fd, char *large_buffer, size_t large_buffer_size) +{ + xlseek (fd, 0, SEEK_SET); + ssize_t ret = getdents64 (fd, large_buffer, large_buffer_size); + if (ret < 0) + FAIL_EXIT1 ("getdents64 for buffer of %zu bytes failed: %m", + large_buffer_size); + if (ret < offsetof (struct dirent64, d_name)) + FAIL_EXIT1 ("getdents64 for buffer of %zu returned small value %zd", + large_buffer_size, ret); +} + +/* Bug 24740: Make sure that the system call argument is adjusted + properly for the int type. A large value should stay a large + value, and not wrap around to something small, causing the system + call to fail with EINVAL. */ +static void +large_buffer_checks (int fd) +{ + size_t large_buffer_size = UINT_MAX; + large_buffer_size += 2; + if (large_buffer_size > 2) + { + char *large_buffer = malloc (large_buffer_size); + if (large_buffer == NULL) + printf ("warning: could not allocate %zu bytes of memory," + " subtests skipped\n", large_buffer_size); + else + { + large_buffer_check (fd, large_buffer, INT_MAX); + large_buffer_check (fd, large_buffer, (size_t) INT_MAX + 1); + large_buffer_check (fd, large_buffer, (size_t) INT_MAX + 2); + large_buffer_check (fd, large_buffer, UINT_MAX); + large_buffer_check (fd, large_buffer, (size_t) UINT_MAX + 1); + large_buffer_check (fd, large_buffer, (size_t) UINT_MAX + 2); + } + free (large_buffer); + } +} + static int do_test (void) { @@ -105,6 +148,8 @@ do_test (void) rewinddir (reference); } + large_buffer_checks (fd); + xclose (fd); closedir (reference); return 0;