| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ECADFF3FD767C149AD96A924E7EA6EAF982C37D2@USCULXMSG17.am.sony.com>
Date: Thu, 16 Jan 2020 17:32:15 +0000
From: <Tim.Bird@...y.com>
To: <masami.hiramatsu@...aro.org>, <siddhesh@...plt.org>
CC: <linux-kselftest@...r.kernel.org>, <shuah@...nel.org>,
<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] kselftest: Minimise dependency of get_size on C library
interfaces
> -----Original Message-----
> From: Masami Hiramatsu <masami.hiramatsu@...aro.org>
> Sent: Tuesday, January 14, 2020 8:14 PM
> To: Siddhesh Poyarekar <siddhesh@...plt.org>
> Cc: linux-kselftest@...r.kernel.org; Shuah Khan <shuah@...nel.org>; Linux kernel mailing list <linux-kernel@...r.kernel.org>; Bird, Tim
> <Tim.Bird@...y.com>
> Subject: Re: [PATCH] kselftest: Minimise dependency of get_size on C library interfaces
>
> Hi,
>
> [Cc: Tim Bird]
>
> 2020年1月14日(火) 1:43 Siddhesh Poyarekar <siddhesh@...plt.org>:
>
> >
> > It was observed[1] on arm64 that __builtin_strlen led to an infinite
> > loop in the get_size selftest. This is because __builtin_strlen (and
> > other builtins) may sometimes result in a call to the C library
> > function. The C library implementation of strlen uses an IFUNC
> > resolver to load the most efficient strlen implementation for the
> > underlying machine and hence has a PLT indirection even for static
> > binaries. Because this binary avoids the C library startup routines,
> > the PLT initialization never happens and hence the program gets stuck
> > in an infinite loop.
> >
> > On x86_64 the __builtin_strlen just happens to expand inline and avoid
> > the call but that is not always guaranteed.
> >
> > Further, while testing on x86_64 (Fedora 31), it was observed that the
> > test also failed with a segfault inside write() because the generated
> > code for the write function in glibc seems to access TLS before the
> > syscall (probably due to the cancellation point check) and fails
> > because TLS is not initialised.
> >
> > To mitigate these problems, this patch reduces the interface with the
> > C library to just the syscall function. The syscall function still
> > sets errno on failure, which is undesirable but for now it only
> > affects cases where syscalls fail.
> >
> > [1] https://bugs.linaro.org/show_bug.cgi?id=5479
> >
>
> Thank you for the fix! I confirmed this fixes the issue.
>
> ----
> root@...note2:/opt/kselftest/size# ./get_size
> TAP version 13
> # Testing system size.
> ok 1 get runtime memory use
> # System runtime memory report (units in Kilobytes):
> ---
> Total: 16085116
> Free: 2042880
> Buffer: 814052
> In use: 13228184
> ...
> 1..1
> ----
>
> Tested-by: Masami Hiramatsu <masami.hiramatsu@...aro.org>
>
>
> > Signed-off-by: Siddhesh Poyarekar <siddhesh@...plt.org>
> > Reported-by: Masami Hiramatsu <masami.hiramatsu@...aro.org>
> > ---
> > tools/testing/selftests/size/get_size.c | 24 ++++++++++++++++++------
> > 1 file changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/size/get_size.c b/tools/testing/selftests/size/get_size.c
> > index d4b59ab979a0..f55943b6d1e2 100644
> > --- a/tools/testing/selftests/size/get_size.c
> > +++ b/tools/testing/selftests/size/get_size.c
> > @@ -12,23 +12,35 @@
> > * own execution. It also attempts to have as few dependencies
> > * on kernel features as possible.
> > *
> > - * It should be statically linked, with startup libs avoided.
> > - * It uses no library calls, and only the following 3 syscalls:
> > + * It should be statically linked, with startup libs avoided. It uses
> > + * no library calls except the syscall() function for the following 3
> > + * syscalls:
> > * sysinfo(), write(), and _exit()
> > *
> > * For output, it avoids printf (which in some C libraries
> > * has large external dependencies) by implementing it's own
> > * number output and print routines, and using __builtin_strlen()
Since the code no longer uses __builtin_strlen(), this comment should
change also, to say something like "and string length function.
> > + *
> > + * The test may crash if any of the above syscalls fails because in some
> > + * libc implementations (e.g. the GNU C Library) errno is saved in
> > + * thread-local storage, which does not get initialized due to avoiding
> > + * startup libs.
> > */
> >
> > #include <sys/sysinfo.h>
> > #include <unistd.h>
> > +#include <sys/syscall.h>
> >
> > #define STDOUT_FILENO 1
> >
> > static int print(const char *s)
> > {
> > - return write(STDOUT_FILENO, s, __builtin_strlen(s));
> > + size_t len = 0;
> > +
> > + while (s[len] != '\0')
> > + len++;
> > +
> > + return syscall(SYS_write, STDOUT_FILENO, s, len);
> > }
> >
> > static inline char *num_to_str(unsigned long num, char *buf, int len)
> > @@ -80,12 +92,12 @@ void _start(void)
> > print("TAP version 13\n");
> > print("# Testing system size.\n");
> >
> > - ccode = sysinfo(&info);
> > + ccode = syscall(SYS_sysinfo, &info);
> > if (ccode < 0) {
> > print("not ok 1");
> > print(test_name);
> > print(" ---\n reason: \"could not get sysinfo\"\n ...\n");
> > - _exit(ccode);
> > + syscall(SYS_exit, ccode);
> > }
> > print("ok 1");
> > print(test_name);
> > @@ -101,5 +113,5 @@ void _start(void)
> > print(" ...\n");
> > print("1..1\n");
> >
> > - _exit(0);
> > + syscall(SYS_exit, 0);
> > }
> > --
> > 2.24.1
Thanks very much for this bug report and fix!!
Reviewed-by: Tim Bird <tim.bird@...y.com>
-- Tim
Powered by blists - more mailing lists