| Message ID | 20250508130732.2068630-1-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] stdlib: Fix __libc_message_impl iovec size (BZ 32947) | expand |
On 5/8/25 9:07 AM, Adhemerval Zanella wrote: > The iovec size should account for all substrings between each conversion > specification. For the format: Looking forward to a v3. > "abc %s efg" > > The list of substrings are: > > ["abc ", arg, " efg] > > which is 2 times the number of maximum arguments *plus* one. Agreed, each conversion specifier creates at most a prefix and the specifier and at most one suffix (the plus 1) for the whole string. Your change is technically correct, but I don't see the use that ubsan triggered on. I audited all users and we don't have any %s conversion specifiers in place t day. $ grep -rl '__libc_fatal.*%s' * $ Current list of single line __libc_fatal messages: ~~~ Can't allocate _hurd_ports\n hurd: Can't allocate sigstate\n hurd: Can't add reference on Mach thread\n msg receive failed on signal thread exc\n BUG: unexpected fault in signal thread\n hurd: Can't allocate file descriptor table\n hurd: Can't allocate initial file descriptors\n Fatal error: glibc detected an invalid stdio handle\n FATAL: exception not rethrown\n Fatal glibc error: rseq registration failed\n Illegal status in internal_getgrouplist.\n Illegal status in internal_getgrouplist.\n Illegal status in __nss_next.\n fork handler counter overflow *** invalid %N$ use detected ***\n *** %n in writable segments detected ***\n *** procfs could not open ***\n Fatal glibc error: cannot get entropy for arc4random\n Unexpected reloc type in static binary.\n Unexpected reloc type in static binary.\n Unexpected reloc type in static binary.\n Unexpected reloc type in static binary.\n Unexpected reloc type in static binary.\n Invalid DWARF unwind data.\n The futex facility returned an unexpected error code.\n Unexpected reloc type in static binary.\n Unexpected reloc type in static binary.\n Unexpected reloc type in static binary.\n Unexpected reloc type in static binary.\n Unexpected reloc type in static binary.\n Unexpected reloc type in static binary.\n ~~~ Two line __libc_fatal messages: ~~~ Fatal glibc error: failed to register TLS destructor: out of memory\n ~~~ \ Fatal glibc error: gconv module reference counter overflow\n ~~~ And via assert we call __asprintf or directly write via writev. However, via our own internal __libc_assert_fail, we do call __libc_message with up to 4 parameters. e.g. 31 __libc_message ("Fatal glibc error: %s:%s (%s): assertion failed: %s\n", 32 file, linestr, function, assertion); $ grep -r '__libc_message ("' * assert/__libc_assert_fail.c: __libc_message ("Fatal glibc error: %s:%s (%s): assertion failed: %s\n", debug/fortify_fail.c: __libc_message ("*** %s ***: terminated\n", msg); malloc/malloc.c: __libc_message ("%s\n", str); sysdeps/posix/libc_fatal.c: __libc_message ("%s", message); And for __libc_assert_fail we indeed trigger UB in this case. > This issue triggered 'out of bounds' errors by stdlib/tst-bz20544 when > glibc is built with experimental UBSAN support [1]. > > Checked on x86_64-linux-gnu. > [1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/ubsan-undef > -- > Changes from v1: > * Add bug report. > --- > sysdeps/posix/libc_fatal.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c > index d90cc6c681..25ef20cfc1 100644 > --- a/sysdeps/posix/libc_fatal.c > +++ b/sysdeps/posix/libc_fatal.c > @@ -61,7 +61,7 @@ __libc_message_impl (const char *fmt, ...) > if (fd == -1) > fd = STDERR_FILENO; > Why is it enough? Please add a short comment. /* At most a substring before each conversion specification and the trailing substring (the plus one). */ > - struct iovec iov[LIBC_MESSAGE_MAX_ARGS * 2 - 1]; > + struct iovec iov[LIBC_MESSAGE_MAX_ARGS * 2 + 1]; > int iovcnt = 0; > ssize_t total = 0; > We still get UB if any internal caller does this wrong. We are failing here, so performance doesn't matter. May we please add something like this? if (iovcnt > (LIBC_MESSAGE_MAX_ARGS * 2 + 1)) { /* Set up a static message. */ iov[0].iov_base = "Fatal glibc error: Internal __libc_message error. Too many arguments.\n"; iov[0].iov_len = strlen ("Fatal glibc error: Internal error. Too many arguments.\n"); total = len; iovcnt = 1; break; } That way we're never UB? Alternatively we could attempt to print as much of the error as possible, but that seems dangerous from a security perspective if the internal constraints have been violated.
On 5/20/25 8:40 AM, Carlos O'Donell wrote: > We still get UB if any internal caller does this wrong. > > We are failing here, so performance doesn't matter. > > May we please add something like this? > > if (iovcnt > (LIBC_MESSAGE_MAX_ARGS * 2 + 1)) > { > /* Set up a static message. */ > iov[0].iov_base = "Fatal glibc error: Internal __libc_message error. Too many arguments.\n"; > iov[0].iov_len = strlen ("Fatal glibc error: Internal error. Too many arguments.\n"); > total = len; > iovcnt = 1; > break; > } > > That way we're never UB? > > Alternatively we could attempt to print as much of the error as possible, > but that seems dangerous from a security perspective if the internal > constraints have been violated. Can we write a tests-internal test case that is compiled statically and can call __libc_message with >4 %s and trigger this message?
diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c index d90cc6c681..25ef20cfc1 100644 --- a/sysdeps/posix/libc_fatal.c +++ b/sysdeps/posix/libc_fatal.c @@ -61,7 +61,7 @@ __libc_message_impl (const char *fmt, ...) if (fd == -1) fd = STDERR_FILENO; - struct iovec iov[LIBC_MESSAGE_MAX_ARGS * 2 - 1]; + struct iovec iov[LIBC_MESSAGE_MAX_ARGS * 2 + 1]; int iovcnt = 0; ssize_t total = 0;