This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Linux: Adjust gedents64 buffer size to int range [BZ #24740]
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Thu, 27 Jun 2019 08:30:01 -0300
- Subject: Re: [PATCH] Linux: Adjust gedents64 buffer size to int range [BZ #24740]
- References: <87d0izwenl.fsf@oldenburg2.str.redhat.com>
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.