[PATCH v2] stdlib: Fix __libc_message_impl iovec size (BZ 32947)
Adhemerval Zanella Netto
adhemerval.zanella@linaro.org
Tue Jun 24 13:31:09 GMT 2025
On 24/06/25 09:59, Carlos O'Donell wrote:
> On 5/28/25 2:54 PM, Adhemerval Zanella Netto wrote:
>> On 20/05/25 09:40, Carlos O'Donell wrote:
>>> 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.
>
> Re-reviewing this after your comments.
>
> Still requesting 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.
>>
>> It is indeed by __libc_assert_fail.c:31. With my ubsan branch, the tst-bz20544 shows:
>>
>> (gdb) bt
>> #0 __pthread_kill_implementation (threadid=<optimized out>, signo=6, no_tid=0) at pthread_kill.c:44
>> #1 0x00007ffff7f9dbb2 in raise (sig=6) at ../sysdeps/posix/raise.c:26
>> #2 0x00007ffff7f0d6fb in ubsan_abort () at ubsan_error.c:32
>> #3 __ubsan_error (source=source@entry=0x7ffff7c175a0,
>> fmt=fmt@entry=0x7ffff7fa98e8 <error: Cannot access memory at address 0x7ffff7fa98e8>) at ubsan_error.c:56
>> #4 0x00007ffff7f08999 in __GI___ubsan_handle_out_of_bounds (_data=_data@entry=0x7ffff7c175a0,
>> index=<error reading variable: Cannot access memory at address 0x7fffffffd468>)
>> at ubsan_handle_out_of_bounds.c:29
>> #5 0x00007ffff77bada6 in __libc_message_impl (
>> fmt=fmt@entry=0x7ffff7b03be0 <error: Cannot access memory at address 0x7ffff7b03be0>)
>> at ../sysdeps/posix/libc_fatal.c:98
>> #6 0x00007ffff768e7e1 in __libc_assert_fail (
>> assertion=assertion@entry=0x7ffff7afcb3e <error: Cannot access memory at address 0x7ffff7afcb3e>,
>> file=file@entry=0x7ffff7afcb31 <error: Cannot access memory at address 0x7ffff7afcb31>,
>> line=line@entry=41,
>> function=function@entry=0x7ffff7b0abd0 <__PRETTY_FUNCTION__.0> <error: Cannot access memory at address 0x7ffff7b0abd0>) at __libc_assert_fail.c:31
>> #7 0x00007ffff76b3abb in __internal_atexit (func=func@entry=0x0, arg=arg@entry=0x0, d=<optimized out>,
>> listp=listp@entry=0x7ffff7b99d78 <__exit_funcs>) at cxa_atexit.c:41
>> #8 0x00007ffff76b3b04 in __GI___cxa_atexit (func=func@entry=0x0, arg=arg@entry=0x0, d=<optimized out>)
>> at cxa_atexit.c:68
>> #9 0x000055555555dd26 in atexit (func=func@entry=0x0) at atexit.c:46
>> #10 0x0000555555557ce9 in test_bz20544_atexit (closure=closure@entry=0x0) at tst-bz20544.c:46
>> #11 0x000055555555a261 in support_subprocess (callback=callback@entry=0x555555557cd0 <test_bz20544_atexit>,
>> closure=closure@entry=0x0) at support_subprocess.c:62
>> #12 0x0000555555558608 in support_capture_subprocess (
>> callback=callback@entry=0x555555557cd0 <test_bz20544_atexit>, closure=closure@entry=0x0)
>> at support_capture_subprocess.c:94
>> #13 0x0000555555557d4e in test_one_fn (test_fn=test_fn@entry=0x555555557cd0 <test_bz20544_atexit>)
>> at tst-bz20544.c:83
>> #14 0x0000555555557e2e in do_test () at tst-bz20544.c:106
>> #15 0x000055555555bc52 in support_test_main (argc=1, argv=0x7fffffffda48, config=config@entry=0x7fffffffd8c0)
>> at support_test_main.c:441
>> #16 0x0000555555557a79 in main (argc=<optimized out>, argv=<optimized out>) at ../support/test-driver.c:171
>>
>>>
>>>> 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). */
>
> My request for a comment remains.
>
> This is a non-trivial construct with magic numbers that we've gotten wrong
> before, and so it should have an explanation to support future reviews.
>
> $0.02.
Ack.
>
>>>> - 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?
>>
>> We don't need this because invalid usage should be catch at *build* time
>> due how the __libc_message macro is implemented. On include/stdio.h, it
>> only supports calls up to LIBC_MESSAGE_MAX_ARGS because of the
>> __libc_message_disp expansion and the __libc_messageN definitions.
>>
>> Issuing a __libc_message with a number of arguments larger than
>> LIBC_MESSAGE_MAX_ARGS with a bogus changes like:
>
> This only works if there is no defect in __libc_message_disp?
> > This only works if the library isn't being exploited by an attacker who called
> __libc_message_impl directly and bypassed the macro (though such an attack
> may attempt to skip the check).
I think once attacker have this way of program control, calling internal only
libc functions with crafted varargs arguments (which requires also some
way to tamper the stack), bypassing any runtime extra constraint on function
would also be possible.
>
> If __libc_message_disp is broken we might never catch it?
>
> We would need a test like the one below which shows a failure to compile and
> verify the test failed to compile?
>
> And we don't currently have "fails at compile time" tests like gcc.
> >> diff --git a/assert/__libc_assert_fail.c b/assert/__libc_assert_fail.c
>> index b50637a893..9e15ac9ceb 100644
>> --- a/assert/__libc_assert_fail.c
>> +++ b/assert/__libc_assert_fail.c
>> @@ -28,6 +28,6 @@ __libc_assert_fail (const char *assertion, const char *file, unsigned int line,
>> char linebuf[INT_BUFSIZE_BOUND (unsigned int)];
>> array_end (linebuf)[-1] = '\0';
>> char *linestr = _itoa_word (line, array_end (linebuf) - 1, 10, 0);
>> - __libc_message ("Fatal glibc error: %s:%s (%s): assertion failed: %s\n",
>> - file, linestr, function, assertion);
>> + __libc_message ("Fatal glibc error: %s:%s (%s): assertion failed: %s %s\n",
>> + file, linestr, function, assertion, "testing");
>> }
>>
>> triggers the build issue:
>>
>> In file included from __libc_assert_fail.c:22:
>> __libc_assert_fail.c: In function ‘__libc_assert_fail’:
>> ../include/stdio.h:199:25: error: implicit declaration of function ‘__libc_message5’; did you mean ‘__libc_message_impl’? [-Wimplicit-function-declaration]
>> 199 | __libc_message_disp (__libc_message, __VA_ARGS__)
>> | ^~~~~~~~~~~~~~
>> ../include/stdio.h:190:39: note: in definition of macro ‘__libc_message_concat_x’
>> 190 | #define __libc_message_concat_x(a,b) a##b
>> | ^
>> ../include/stdio.h:197:4: note: in expansion of macro ‘__libc_message_concat’
>> 197 | __libc_message_concat (b, __libc_message_nargs (__VA_ARGS__))(__VA_ARGS__)
>> | ^~~~~~~~~~~~~~~~~~~~~
>> ../include/stdio.h:199:4: note: in expansion of macro ‘__libc_message_disp’
>> 199 | __libc_message_disp (__libc_message, __VA_ARGS__)
>> | ^~~~~~~~~~~~~~~~~~~
>> __libc_assert_fail.c:31:3: note: in expansion of macro ‘__libc_message’
>> 31 | __libc_message ("Fatal glibc error: %s:%s (%s): assertion failed: %s %s\n",
>> | ^~~~~~~~~~~~~~
>>
>>>
>>> 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?
>>
>> We can't really code this test because it won't build unless we start to
>> add larger __libc_messageN, which defeats the macro itself.
>
> That would be perfect. That shows the macro is working as intended, and
> I would accept that as a way to ensure we don't make a mistake.
>
> But we don't have a "expected fails to compile" test framework.
>
> So my suggestion remains, either at test to verify __libc_message_disp,
> or a runtime check?
>
> Our current implementation actually *does* support 5 args without a
> trailing string.
>
> The following string doesn't trigger the test correctly:
> "%s %s %s %s %s" (without a trailing string)
>
> So we support 4 args with a trailing string, 5 args without a trailing
> string, and do not support 5 args with a trailing string or more than
> 5 args.
Right, but __libc_message_disp was not mean to be called directly, but
only through the wrapper __libc_message. But unfortunatelly we don't
have a easy way to avoid it or to instruct compiler that __libc_message
should be used instead.
>
> Example:
> ~~~
> diff --git a/posix/Makefile b/posix/Makefile
> index c0e224236a..6986aef948 100644
> --- a/posix/Makefile
> +++ b/posix/Makefile
> @@ -348,6 +348,7 @@ tests-internal := \
> bug-regex5 \
> bug-regex20 \
> bug-regex33 \
> + tst-libc-message \
> # tests-internal
>
> tests-container := \
> @@ -392,6 +393,7 @@ endif
> tests-static = \
> tst-exec-static \
> tst-spawn-static \
> + tst-libc-message \
> # tests-static
>
> tests += \
> diff --git a/posix/tst-libc-message.c b/posix/tst-libc-message.c
> new file mode 100644
> index 0000000000..f70e5201c4
> --- /dev/null
> +++ b/posix/tst-libc-message.c
> @@ -0,0 +1,30 @@
> +/* Internal test to verify __libc_fatal.
> + Copyright (C) 2025 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <stdio.h>
> +
> +static int
> +do_test (void)
> +{
> + /* We only support 4 arguments. Call with 5 to trigger failure. */
> + __libc_message_impl ("%s %s %s %s %s\n", "1", "2", "3", "4", "5");
> + /* We never reach here. */
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c
> index 25ef20cfc1..258e4f6546 100644
> --- a/sysdeps/posix/libc_fatal.c
> +++ b/sysdeps/posix/libc_fatal.c
> @@ -47,6 +47,10 @@ writev_for_fatal (int fd, const struct iovec *iov, size_t niov, size_t total)
> }
> #endif
>
> +/* At most a substring before each conversion specification and the
> + trailing substring (the plus one). */
> +#define IOVEC_MAX (LIBC_MESSAGE_MAX_ARGS * 2 + 1)
> +
> /* Abort with an error message. */
> void
> __libc_message_impl (const char *fmt, ...)
> @@ -61,7 +65,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[IOVEC_MAX];
> int iovcnt = 0;
> ssize_t total = 0;
>
> @@ -99,6 +103,16 @@ __libc_message_impl (const char *fmt, ...)
> iov[iovcnt].iov_len = len;
> total += len;
> iovcnt++;
> +
> + if (iovcnt > IOVEC_MAX)
> + {
> + /* Invalid caller, likely a defect in glibc. */
> + iov[0].iov_base = (char *) "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;
> + }
> }
> va_end (ap);
> ~~~
> Fatal glibc error: Internal __libc_message error. Too mmake[2]: Leaving directory '/home/carlos/src/glibc-review/posix'
> FAIL: posix/tst-libc-message
> original exit status 1
> Didn't expect signal from child: got `Aborted'
> make[1]: Leaving directory '/home/carlos/src/glibc-review'
>
> This just needs wrapping in subprogram execution and a check for the
> printed message to stderr for invalid direct usage of __libc_message_impl.
>
> Thoughts?
>
Ack, I will add a runtime check and a related test.
More information about the Libc-alpha
mailing list