Linux: Adjust gedents64 buffer size to int range [BZ #24740]

Message ID 87d0izwenl.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series
  • Linux: Adjust gedents64 buffer size to int range [BZ #24740]
Related show

Commit Message

Florian Weimer June 27, 2019, 9:39 a.m.
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.

Comments

Adhemerval Zanella June 27, 2019, 11:30 a.m. | #1
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.
Florian Weimer June 27, 2019, 1:09 p.m. | #2
* 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
Rafal Luzynski June 28, 2019, 8:36 a.m. | #3
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
Florian Weimer June 28, 2019, 8:43 a.m. | #4
* 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

Patch

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;