[PATCH] debug: Improve '%n' fortify detection (BZ 30932)
Adhemerval Zanella Netto
adhemerval.zanella@linaro.org
Fri Mar 14 18:42:45 GMT 2025
On 13/03/25 20:41, Arjun Shankar wrote:
> Hi Adhemerval,
>
> Took me a while to go through it since I had to learn about some of
> the context before I could meaningfully comment on the patch. I
> learned something while reviewing this. Thanks!
>
> The patch seems mostly okay to me, but I have some comments below.
Thanks for the review, I will send an updated version.
>
> Cheers!
> Arjun
>
>> The 7bb8045ec0 path made the '%n' fortify check ignore EMFILE errors
>> while trying to open /proc/self/maps, and this added a security
>> issue where EMFILE can be attacker-controlled thus making it
>> ineffective for some cases.
>>
>> The EMFILE failure is reinstated but with a different error
>> message. Also, to improve the false positive of the hardening for
>> the cases where no new files can be opened, the
>> _dl_readonly_area now uses _dl_find_object to check if the
>> memory area is within a writable ELF segment. The procfs method is
>> still used as afallback.
>>
>> Checked on x86_64-linux-gnu and i686-linux-gnu.
>
> Small typo: "afallback".
>
>> ---
>> Makeconfig | 2 +-
>> debug/Makefile | 14 ++
>> debug/readonly-area.c | 17 +-
>> debug/tst-sprintf-fortify-rdonly-dlopen.c | 1 +
>> debug/tst-sprintf-fortify-rdonly-mod.c | 53 ++++++
>> debug/tst-sprintf-fortify-rdonly.c | 155 ++++++++++++++++--
>> elf/Makefile | 1 +
>> elf/dl-readonly-area.c | 84 ++++++++++
>> elf/rtld.c | 1 +
>> include/stdlib.h | 15 ++
>> stdio-common/vfprintf-internal.c | 9 +-
>> stdio-common/vfprintf-process-arg.c | 28 ++--
>> sysdeps/generic/ldsodefs.h | 7 +
>> .../{readonly-area.c => readonly-area-arch.c} | 11 +-
>> .../{readonly-area.c => readonly-area-arch.c} | 21 +--
>> 15 files changed, 355 insertions(+), 64 deletions(-)
>> create mode 100644 debug/tst-sprintf-fortify-rdonly-dlopen.c
>> create mode 100644 debug/tst-sprintf-fortify-rdonly-mod.c
>> create mode 100644 elf/dl-readonly-area.c
>> rename sysdeps/mach/{readonly-area.c => readonly-area-arch.c} (90%)
>> rename sysdeps/unix/sysv/linux/{readonly-area.c => readonly-area-arch.c} (84%)
>>
>> diff --git a/Makeconfig b/Makeconfig
>> index aa547a443f..bf50406656 100644
>> --- a/Makeconfig
>> +++ b/Makeconfig
>> @@ -633,7 +633,7 @@ link-libc-printers-tests = $(link-libc-rpath) \
>> $(link-libc-tests-after-rpath-link)
>>
>> # This is how to find at build-time things that will be installed there.
>> -rpath-dirs = math elf dlfcn nss nis rt resolv mathvec support misc
>> +rpath-dirs = math elf dlfcn nss nis rt resolv mathvec support misc debug
>
> OK. Looks like this is needed for the test to be able to dlopen the test module.
>
>> rpath-link = \
>> $(common-objdir):$(subst $(empty) ,:,$(patsubst ../$(subdir),.,$(rpath-dirs:%=$(common-objpfx)%)))
>> else # build-static
>
>> diff --git a/debug/Makefile b/debug/Makefile
>> index 6a05205ce6..be432f7740 100644
>> --- a/debug/Makefile
>> +++ b/debug/Makefile
>> @@ -74,6 +74,7 @@ routines = \
>> readlink_chk \
>> readlinkat_chk \
>> readonly-area \
>> + readonly-area-arch \
>
> This is the fallback implementation for checking read-only status of
> an address range.
> The -arch suffix seems a bit confusing to me as a name. How about
> readonly-area-fallback.c?
Ok, I will change to that.
>
>> realpath_chk \
>> recv_chk \
>> recvfrom_chk \
>> @@ -179,9 +180,17 @@ CPPFLAGS-tst-longjmp_chk3.c += $(no-fortify-source) -D_FORTIFY_SOURCE=1
>> CPPFLAGS-tst-realpath-chk.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2
>> CPPFLAGS-tst-chk-cancel.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2
>> CFLAGS-tst-sprintf-fortify-rdonly.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2
>> +CFLAGS-tst-sprintf-fortify-rdonly-mod.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2
>> +CFLAGS-tst-sprintf-fortify-rdonly-dlopen.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2
>
> OK. Build test modules with fortification on so that the checks we
> test for are performed.
>
>> CFLAGS-tst-fortify-syslog.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2
>> CFLAGS-tst-fortify-wide.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2
>>
>> +$(objpfx)tst-sprintf-fortify-rdonly: \
>> + $(objpfx)tst-sprintf-fortify-rdonly-mod.so \
>> + $(objpfx)tst-sprintf-fortify-rdonly-dlopen.so
>> +LDLIBS-tst-sprintf-fortify-rdonly-mod.so = $(libsupport)
>> +LDLIBS-tst-sprintf-fortify-rdonly-dlopen.so = $(libsupport)
>> +
>
> Florian was mentioning that linking test modules against libsupport
> might not work properly. I don't remember the details though.
This is only required because of xstrdup, I will replace with strdup + abort.
>
>> # _FORTIFY_SOURCE tests.
>> # Auto-generate tests for _FORTIFY_SOURCE for different levels, compilers and
>> # preprocessor conditions based on tst-fortify.c.
>> @@ -302,6 +311,11 @@ tests-container += \
>> tst-fortify-syslog \
>> # tests-container
>>
>> +modules-names += \
>> + tst-sprintf-fortify-rdonly-dlopen \
>> + tst-sprintf-fortify-rdonly-mod \
>> + # modules-names
>> +
>
> OK. New test modules.
>
>> ifeq ($(have-ssp),yes)
>> tests += tst-ssp-1
>> endif
>
>> diff --git a/debug/readonly-area.c b/debug/readonly-area.c
>> index 04b437ed8d..e53bc00f9f 100644
>> --- a/debug/readonly-area.c
>> +++ b/debug/readonly-area.c
>> @@ -16,18 +16,13 @@
>> <https://www.gnu.org/licenses/>. */
>>
>> #include <stdlib.h>
>> +#include <ldsodefs.h>
>
> OK.
>
>>
>> -/* Return 1 if the whole area PTR .. PTR+SIZE is not writable.
>> - Return -1 if it is writable. */
>> -
>> -int
>> +enum readonly_error_type
>> __readonly_area (const void *ptr, size_t size)
>> {
>> - /* We cannot determine in general whether memory is writable or not.
>> - This must be handled in a system-dependent manner. to not
>> - unconditionally break code we need to return here a positive
>> - answer. This disables this security measure but that is the
>> - price people have to pay for using systems without a real
>> - implementation of this interface. */
>> - return 1;
>
> OK. This used to be a stub implementation but now it is the primary
> one and uses the new enum to differentiate failure modes. The Linux
> implementation was at sysdeps/unix/sysv/linux/readonly-area.c.
>
>> + if (GLRO(dl_readonly_area (ptr, size)))
>> + return readonly_noerror;
>
> OK. First try to verify that it is read-only by iterating the link
> map. If it is able to verify that the entire block (of size length) is
> read-only, it will return true. Otherwise, it will return false.
>
>> +
>> + return __readonly_area_arch (ptr, size);
>> }
>
> OK. Then we use the fallback (which will read from /proc on Linux).
>
>> diff --git a/debug/tst-sprintf-fortify-rdonly-dlopen.c b/debug/tst-sprintf-fortify-rdonly-dlopen.c
>> new file mode 100644
>> index 0000000000..7da3f51f16
>> --- /dev/null
>> +++ b/debug/tst-sprintf-fortify-rdonly-dlopen.c
>> @@ -0,0 +1 @@
>> +#include "tst-sprintf-fortify-rdonly-mod.c"
>
> OK. Rebuild the other module separately, for dlopen'ing.
>
>> diff --git a/debug/tst-sprintf-fortify-rdonly-mod.c b/debug/tst-sprintf-fortify-rdonly-mod.c
>> new file mode 100644
>> index 0000000000..def15f1ac4
>> --- /dev/null
>> +++ b/debug/tst-sprintf-fortify-rdonly-mod.c
>
> OK. New module for test.
>
>> @@ -0,0 +1,53 @@
>> +/* Testcase for BZ 30932.
>> + 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>
>> +#include <string.h>
>> +#include <support/support.h>
>> +
>> +static const char *str2 = "F";
>
> OK. We use this as a placeholder string to be written for every %s.
>
>> +static char buf2[10] = "%s";
>> +static char buf3[10] __attribute__ ((section (".data.rel.ro"))) = "%s%n%s%n";
>
> Format strings. buf2 is writeable, buf3 is relro (and therefore not writeable).
> Maybe we could use more descriptive names. I see that these are
> inherited from the existing test. Perhaps writeable_format and
> relro_format?
>
>> +
>> +void
>> +init_str2 (void)
>> +{
>> + strcpy (buf2 + 2, "%n%s%n");
>> +}
>> +
>
> Should this be init_buf2? Or, if you rename the variable, it could
> become init_writeable_format e.g..
Ack.
>
>> +int
>> +sprintf_buf2 (int *n1, int *n2)
>> +{
>> + char buf[128];
>> + return sprintf (buf, buf2, str2, n1, str2, n2);
>> +}
>
> OK.
>
>> +
>> +int
>> +sprintf_buf3 (int *n1, int *n2)
>> +{
>> + char buf[128];
>> + return sprintf (buf, buf3, str2, n1, str2, n2);
>> +}
>
> OK.
>
>> +
>> +int
>> +sprintf_buf2_malloc (int *n1, int *n2)
>> +{
>> + char buf[128];
>> + char *buf2_malloc = xstrdup (buf2);
>> + return sprintf (buf, buf2_malloc, str2, n1, str2, n2);
>> +}
>
> OK.
>
>> diff --git a/debug/tst-sprintf-fortify-rdonly.c b/debug/tst-sprintf-fortify-rdonly.c
>> index ba5a791020..432adba370 100644
>> --- a/debug/tst-sprintf-fortify-rdonly.c
>> +++ b/debug/tst-sprintf-fortify-rdonly.c
>> @@ -27,16 +27,63 @@
>> #include <support/check.h>
>> #include <support/support.h>
>> #include <support/temp_file.h>
>> +#include <support/xdlfcn.h>
>>
>> -jmp_buf chk_fail_buf;
>> -bool chk_fail_ok;
>> +static jmp_buf chk_fail_buf;
>> +static volatile int ret;
>> +static bool chk_fail_ok;
>
> OK.
>
>>
>> -const char *str2 = "F";
>> -char buf2[10] = "%s";
>
> OK. These declarations simply moved further down.
>
>> +static void
>> +handler (int sig)
>> +{
>> + if (chk_fail_ok)
>> + {
>> + chk_fail_ok = false;
>> + longjmp (chk_fail_buf, 1);
>> + }
>> + else
>> + _exit (127);
>> +}
>
> Fortify test signal handler. It will be called upon SIGABRT. If the
> check_fail_ok is true, we are inside a CHECK_FAIL_START /
> CHECK_FAIL_END scope, which is expected. We then reset the flag and
> return to the CHK_FAIL_START sigsetjmp.
>
> Shouldn't this be a siglongjmp with a sigjmp_buf, to correspond to the
> sigsetjmp?
It would be more conformant, however the __longjmp_chk (which would be
generated by using _FORTIFY_SOURCE) will be essentially the siglongjmp
(debug/longjmp_chk.c). I will change it.
>
>> +
>> +#define FORTIFY_FAIL \
>> + do { printf ("Failure on line %d\n", __LINE__); ret = 1; } while (0)
>
> OK.
>
>> +#define CHK_FAIL_START \
>> + chk_fail_ok = true; \
>> + if (! sigsetjmp (chk_fail_buf, 1)) \
>> + {
>
> Also accordingly needs a sigjmp_buf, otherwise OK.
>
> Once the check inside this scope leads to a SIGABRT, we will return
> from this point as if sigsetjmp returned 1, thus skipping the rest of
> the scope and not executing the FORTIFY_FAIL below.
>
>> +#define CHK_FAIL_END \
>> + chk_fail_ok = false; \
>> + FORTIFY_FAIL; \
>> + }
>
> OK. Whenever a test expected to SIGABRT does *not* sigabrt, we will
> execute the FORTIFY_FAIL here, thus logging an error that the
> fortification did not abort as expected.
>
>> +
>> +static const char *str2 = "F";
>> +static char buf2[10] = "%s";
>> +static char buf3[10] __attribute__ ((section (".data.rel.ro"))) = "%s%n%s%n";
>
> OK. Local buffers.
>
>> +
>> +extern void init_str2 (void);
>> +extern int sprintf_buf2 (int *, int *);
>> +extern int sprintf_buf3 (int *, int *);
>> +extern int sprintf_buf2_malloc (int *, int *);
>
> OK. These should get picked up from the module.
>
>> +
>> +#define str(__x) # __x
>> +void (*init_str2_dlopen)(void);
>> +int (*sprintf_buf2_dlopen)(int *, int *);
>> +int (*sprintf_buf3_dlopen)(int *, int *);
>> +int (*sprintf_buf2_malloc_dlopen)(int *, int *);
>
> OK. These should get populated from the dlopen'd module.
>
>>
>> static int
>> do_test (void)
>> {
>> + set_fortify_handler (handler);
>
> OK. Set up the handler that will facilitate the checks for expected
> fortification errors.
>
>> +
>> + {
>> + void *h = xdlopen ("tst-sprintf-fortify-rdonly-dlopen.so", RTLD_NOW);
>> + init_str2_dlopen = xdlsym (h, str(init_str2));
>> + sprintf_buf2_dlopen = xdlsym (h, str(sprintf_buf2));
>> + sprintf_buf3_dlopen = xdlsym (h, str(sprintf_buf3));
>> + sprintf_buf2_malloc_dlopen = xdlsym (h, str(sprintf_buf2_malloc));
>> + }
>> +
>
> OK.
>
>> struct rlimit rl;
>> int max_fd = 24;
>>
>> @@ -63,20 +110,94 @@ do_test (void)
>> }
>> TEST_VERIFY_EXIT (nfiles != 0);
>>
>
>> - /* When the format string is writable and contains %n,
>> - with -D_FORTIFY_SOURCE=2 it causes __chk_fail. However, if libc can not
>> - open procfs to check if the input format string in within a writable
>> - memory segment, the fortify version can not perform the check. */
>> - char buf[128];
>> - int n1;
>> - int n2;
>> -
>> strcpy (buf2 + 2, "%n%s%n");
>> - if (sprintf (buf, buf2, str2, &n1, str2, &n2) != 2
>> - || n1 != 1 || n2 != 2)
>> - FAIL_EXIT1 ("sprintf failed: %s %d %d", buf, n1, n2);
>> -
>> - return 0;
>
>> + init_str2 ();
>> + init_str2_dlopen ();
>> +
>> + /* buf2 is at a writable part of .bss segment, so libc should be able to
>> + check it without resorting to procfs. */
>> + {
>> + char buf[128];
>> + int n1;
>> + int n2;
>> + CHK_FAIL_START
>> + sprintf (buf, buf2, str2, &n1, str2, &n2);
>> + CHK_FAIL_END
>> + }
>> +
>
> OK. This is a check that expects a fortification failure, hence the
> CHK_FAIL_* scope.
>
>> + /* Same as before, but from an library. */
>> + {
>> + int n1;
>> + int n2;
>> + CHK_FAIL_START
>> + sprintf_buf2 (&n1, &n2);
>> + CHK_FAIL_END
>> + }
>
> OK. Same.
>
>> +
>> + {
>> + int n1;
>> + int n2;
>> + CHK_FAIL_START
>> + sprintf_buf2_dlopen (&n1, &n2);
>> + CHK_FAIL_END
>> + }
>
> OK. Same.
>
>> +
>> + /* buf3 is at a readonly part of .bss segment, so '%n' in format input
>> + should not trigger a fortify failure. */
>> + {
>> + char buf[128];
>> + int n1;
>> + int n2;
>> + if (sprintf (buf, buf3, str2, &n1, str2, &n2) != 2
>> + || n1 != 1 || n2 != 2)
>> + FAIL_EXIT1 ("sprintf failed: %s %d %d", buf, n1, n2);
>> + }
>
> OK.
>
>> +
>> + /* Same as before, but from an library. */
>> + {
>> + int n1;
>> + int n2;
>> + if (sprintf_buf3 (&n1, &n2) != 2 || n1 != 1 || n2 != 2)
>> + FAIL_EXIT1 ("sprintf failed: %d %d", n1, n2);
>> + }
>
> OK. Same.
>
>> +
>> + {
>> + int n1;
>> + int n2;
>> + if (sprintf_buf3_dlopen (&n1, &n2) != 2 || n1 != 1 || n2 != 2)
>> + FAIL_EXIT1 ("sprintf failed: %d %d", n1, n2);
>> + }
>
> OK. Same.
>
>> +
>> + /* However if the format string is placed on a writable memory not covered
>> + by ELF segments, libc needs to resort to procfs. */
>> + {
>> + char buf[128];
>> + int n1;
>> + int n2;
>> + char *buf2_malloc = xstrdup (buf2);
>> + CHK_FAIL_START
>> + sprintf (buf, buf2_malloc, str2, &n1, str2, &n2);
>> + CHK_FAIL_END
>> + }
>
> OK.
>
>> +
>> + /* Same as before, but from an library. */
>> + {
>> + int n1;
>> + int n2;
>> + CHK_FAIL_START
>> + sprintf_buf2_malloc (&n1, &n2);
>> + CHK_FAIL_END
>> + }
>
> OK.
>
>> +
>> + {
>> + int n1;
>> + int n2;
>> + CHK_FAIL_START
>> + sprintf_buf2_malloc_dlopen (&n1, &n2);
>> + CHK_FAIL_END
>> + }
>
> OK.
>
>> +
>> + return ret;
>
> OK. This ensures that any failures logged by CHK_FAIL_END lead to a
> test failure. All the positive test cases use EXIT_FAIL1 instead.
>
>> }
>>
>> #include <support/test-driver.c>
>
>> diff --git a/elf/Makefile b/elf/Makefile
>> index 1ea0e7037e..6675302a87 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>> @@ -72,6 +72,7 @@ dl-routines = \
>> dl-open \
>> dl-origin \
>> dl-printf \
>> + dl-readonly-area \
>
> OK. This is for _dl_readonly_area.
>
>> dl-reloc \
>> dl-runtime \
>> dl-scope \
>
>> diff --git a/elf/dl-readonly-area.c b/elf/dl-readonly-area.c
>> new file mode 100644
>> index 0000000000..09a98319db
>> --- /dev/null
>> +++ b/elf/dl-readonly-area.c
>
> OK. New implementation.
>
>> @@ -0,0 +1,84 @@
>> +/* Check if range is within a read-only from a loaded ELF object.
>> + 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 <ldsodefs.h>
>> +
>> +static bool
>> +check_relro (const struct link_map *l, uintptr_t start, uintptr_t end)
>> +{
>> + for (const ElfW(Phdr) *ph = l->l_phdr; ph < &l->l_phdr[l->l_phnum]; ++ph)
>> + if (ph->p_type == PT_GNU_RELRO)
>> + {
>> + uintptr_t relro_start = ALIGN_DOWN (l->l_addr + ph->p_vaddr,
>> + GLRO(dl_pagesize));
>> + uintptr_t relro_end = ALIGN_DOWN (l->l_addr + ph->p_vaddr
>> + + ph->p_memsz, GLRO(dl_pagesize));
>> + /* RELRO is caved out from a RW segment, so the next range is either
>> + RW or nonexistent. */
>> + return relro_start < start && relro_end >= end;
>> + }
>> + return false;
>> +}
>
> The link map has an l_relro_addr and an l_relro_size that could be
> used directly instead of the loop here.
Right, I forgot about these.
>
>> +
>> +bool
>> +_dl_readonly_area (const void *ptr, size_t size)
>> +{
>> + struct dl_find_object dlfo;
>> + if (_dl_find_object ((void *)ptr, &dlfo) != 0)
>> + return false;
>> +
>> + const struct link_map *l = dlfo.dlfo_link_map;
>> + uintptr_t ptr_start = (uintptr_t) ptr;
>> + uintptr_t ptr_end = ptr_start + size;
>> +
>> + for (const ElfW(Phdr) *ph = l->l_phdr; ph < &l->l_phdr[l->l_phnum]; ++ph)
>> + if (ph->p_type == PT_LOAD)
>> + {
>> + /* For segments with alignment larger than the page size,
>> + _dl_map_segment allocates additional space that is mark as
>> + PROT_NONE (so we can ignore). */
>> + uintptr_t from = l->l_addr
>> + + ALIGN_DOWN (ph->p_vaddr, GLRO(dl_pagesize));
>> + uintptr_t to = l->l_addr
>> + + ALIGN_UP (ph->p_vaddr + ph->p_filesz, GLRO(dl_pagesize));
>> +
>> + /* Found an entry that at least partially covers the area. */
>> + if (from < ptr_end && to > ptr_start)
>
> OK.
>
>> + {
>> + if (ph->p_flags & PF_W)
>> + return check_relro (l, ptr_start, ptr_end);
>
> OK.
>
>> +
>> + if ((ph->p_flags & PF_R) == 0)
>> + return readonly_area_writable;
>
> This should return a boolean false in line with the return type.
In fact I realized that _dl_readonly_area should return either if the
input in the read-only segment, read-write segment, and not found.
The fallback should only be called if range is not found. I will
change it.
>
>> +
>> + if (from < ptr_start && to >= ptr_end)
>> + return true;
>
> I think this should be "from <= ptr_start" to allow for the
> possibility that the string starts at the beginning of the section.
It should be, and the origin __readonly_area also does it. I will fix it.
>
>> + else if (from < ptr_start)
>> + size -= to - ptr_start;
>
> At this point we know we have a partial and only partial overlap, and
> that the section begins at-or-before the string's beginning and
> doesn't cover the end of the string. The size accounted for is from
> the beginning of the string to the end of the section. This looks OK
> except again, unless I'm missing some detail, the comparison should be
> adjusted to "from <= ptr_start" to account for the section and string
> starting at the same boundary.
You are right, this is another mistake I did by adapting the
sysdeps/unix/sysv/linux/readonly-area.c.
>
>> + else if (to >= ptr_end)
>> + size -= ptr_end - from;
>
> OK. This is similar but for partial coverage towards the end of the
> string. The comparison is correct, using ">=" to allow for the string
> and section to end at the same boundary.
>
>> + else
>> + size -= to - from;
>
> OK. Finally this case corresponds to the section only covering some
> middle part of the string. i.e. the string starts before the section
> and ends after it.
>
>> +
>> + if (size == 0)
>> + break;
>
> OK. We accounted for the entire string's length.
>
>> + }
>> + }
>> +
>> + return size == 0;
>> +}
>
> OK. If we have accounted for the entire string's length being
> read-only, potentially split across multiple sections, we return true.
> Otherwise, we return false.
>
>> diff --git a/elf/rtld.c b/elf/rtld.c
>> index 115f1da37f..69b0c9e111 100644
>> --- a/elf/rtld.c
>> +++ b/elf/rtld.c
>> @@ -372,6 +372,7 @@ struct rtld_global_ro _rtld_global_ro attribute_relro =
>> ._dl_error_free = _dl_error_free,
>> ._dl_tls_get_addr_soft = _dl_tls_get_addr_soft,
>> ._dl_libc_freeres = __rtld_libc_freeres,
>> + ._dl_readonly_area = _dl_readonly_area,
>> };
>
> OK. Initialize the function pointer in rtld_global_ro.
>
>> /* If we would use strong_alias here the compiler would see a
>> non-hidden definition. This would undo the effect of the previous
>> diff --git a/include/stdlib.h b/include/stdlib.h
>> index 57f4ab8545..6b554ba56f 100644
>> --- a/include/stdlib.h
>> +++ b/include/stdlib.h
>> @@ -368,6 +368,21 @@ struct abort_msg_s
>> extern struct abort_msg_s *__abort_msg;
>> libc_hidden_proto (__abort_msg)
>>
>> +enum readonly_error_type
>> +{
>> + readonly_noerror,
>> + readonly_area_writable,
>> + readonly_procfs_inaccessible,
>> + readonly_procfs_open_fail,
>> +};
>> +
>
> OK. Now we differentiate between multiple error modes when it comes to
> verifying read-only format specifiers.
>
>> +extern enum readonly_error_type __readonly_area (const void *ptr,
>> + size_t size)
>> + attribute_hidden;
>> +extern enum readonly_error_type __readonly_area_arch (const void *ptr,
>> + size_t size)
>> + attribute_hidden;
>> +
>
> OK. The main and system specific read-only area checker functions.
>
>> # if IS_IN (rtld)
>> extern __typeof (unsetenv) unsetenv attribute_hidden;
>> extern __typeof (__strtoul_internal) __strtoul_internal attribute_hidden;
>
>> diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
>> index aa9708bff5..fa41e1b242 100644
>> --- a/stdio-common/vfprintf-internal.c
>> +++ b/stdio-common/vfprintf-internal.c
>> @@ -576,7 +576,8 @@ static const uint8_t jump_table[] =
>>
>> /* Handle positional format specifiers. */
>> static void printf_positional (struct Xprintf_buffer *buf,
>> - const CHAR_T *format, int readonly_format,
>> + const CHAR_T *format,
>> + enum readonly_error_type readonly_format,
>
> OK. Use enum to clearly describe the type of error.
>
>> va_list ap, va_list *ap_savep,
>> int nspecs_done, const UCHAR_T *lead_str_end,
>> CHAR_T *work_buffer, int save_errno,
>> @@ -626,9 +627,7 @@ Xprintf_buffer (struct Xprintf_buffer *buf, const CHAR_T *format,
>> /* For the %m format we may need the current `errno' value. */
>> int save_errno = errno;
>>
>> - /* 1 if format is in read-only memory, -1 if it is in writable memory,
>> - 0 if unknown. */
>> - int readonly_format = 0;
>> + enum readonly_error_type readonly_format = readonly_noerror;
>
> OK. Again, use the new enum.
>
>>
>> /* Initialize local variables. */
>> grouping = (const char *) -1;
>> @@ -1045,7 +1044,7 @@ do_positional:
>>
>> static void
>> printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format,
>> - int readonly_format,
>> + enum readonly_error_type readonly_format,
>
> OK. Same.
>
>> va_list ap, va_list *ap_savep, int nspecs_done,
>> const UCHAR_T *lead_str_end,
>> CHAR_T *work_buffer, int save_errno,
>
>> diff --git a/stdio-common/vfprintf-process-arg.c b/stdio-common/vfprintf-process-arg.c
>> index 8d20493766..98dc9844d4 100644
>> --- a/stdio-common/vfprintf-process-arg.c
>> +++ b/stdio-common/vfprintf-process-arg.c
>> @@ -324,16 +324,24 @@ LABEL (form_pointer):
>> LABEL (form_number):
>> if ((mode_flags & PRINTF_FORTIFY) != 0)
>> {
>> - if (! readonly_format)
>> - {
>> - extern int __readonly_area (const void *, size_t)
>> - attribute_hidden;
>> - readonly_format
>> - = __readonly_area (format, ((STR_LEN (format) + 1)
>> - * sizeof (CHAR_T)));
>> - }
>> - if (readonly_format < 0)
>> - __libc_fatal ("*** %n in writable segment detected ***\n");
>> + if (readonly_format == readonly_noerror)
>> + readonly_format = __readonly_area (format, ((STR_LEN (format) + 1)
>> + * sizeof (CHAR_T)));
>> + switch (readonly_format)
>> + {
>> + case readonly_area_writable:
>> + __libc_fatal ("*** %n in writable segment detected ***\n");
>> + /* The format is not within ELF segmetns and opening /proc/self/maps
>> + failed because there are too many files. */
>> + case readonly_procfs_open_fail:
>> + __libc_fatal ("*** procfs could not open ***\n");
>> + /* The /proc/self/maps can not be opened either because it is not
>> + available or the process does not have the right permission. Since
>> + it should not be attacker-controlled we can avoid failure. */
>> + case readonly_procfs_inaccessible:
>> + case readonly_noerror:
>> + break;
>> + }
>
> Small typo here: "segments".
> Otherwise OK. The usage of readonly_format is adjusted to use the enum
> instead of int. __readonly_area will now use the new implementation
> that returns the enum. Then we handle all possible enum values.
>
>> }
>> /* Answer the count of characters written. */
>> void *ptrptr = process_arg_pointer ();
>> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
>> index 8465cbaa9b..ac95992ac8 100644
>> --- a/sysdeps/generic/ldsodefs.h
>> +++ b/sysdeps/generic/ldsodefs.h
>> @@ -676,6 +676,10 @@ struct rtld_global_ro
>> dlopen. */
>> int (*_dl_find_object) (void *, struct dl_find_object *);
>>
>> + /* Implementation of _dl_readonly_area, used in fortify routines to check
>> + if memory area is within a read-only ELF segment. */
>> + bool (*_dl_readonly_area) (const void *, size_t);
>> +
>
> OK. Entry in rtld_global_ro struct.
>
>> /* Dynamic linker operations used after static dlopen. */
>> const struct dlfcn_hook *_dl_dlfcn_hook;
>>
>> @@ -1280,6 +1284,9 @@ extern void _dl_show_scope (struct link_map *new, int from)
>> extern struct link_map *_dl_find_dso_for_object (const ElfW(Addr) addr);
>> rtld_hidden_proto (_dl_find_dso_for_object)
>>
>> +extern bool _dl_readonly_area (const void *ptr, size_t size)
>> + attribute_hidden;
>> +
>
> OK.
>
>> /* Initialization which is normally done by the dynamic linker. */
>> extern void _dl_non_dynamic_init (void)
>> attribute_hidden;
>
>> diff --git a/sysdeps/mach/readonly-area.c b/sysdeps/mach/readonly-area-arch.c
>> similarity index 90%
>> rename from sysdeps/mach/readonly-area.c
>> rename to sysdeps/mach/readonly-area-arch.c
>> index fb89413fe6..b1bb1cba39 100644
>> --- a/sysdeps/mach/readonly-area.c
>> +++ b/sysdeps/mach/readonly-area-arch.c
>
> OK. The old Linux and mach implementations are now a back-up and only
> called when the new link map based implementation doesn't return a
> useful result.
>
>> @@ -20,11 +20,8 @@
>> #include <stdint.h>
>> #include <mach.h>
>>
>> -/* Return 1 if the whole area PTR .. PTR+SIZE is not writable.
>> - Return -1 if it is writable. */
>> -
>> -int
>> -__readonly_area (const char *ptr, size_t size)
>> +enum readonly_error_type
>> +__readonly_area_arch (const void *ptr, size_t size)
>
> OK. Adjust for new enum.
>
>> {
>> vm_address_t region_address = (uintptr_t) ptr;
>> vm_size_t region_length = size;
>> @@ -46,11 +43,11 @@ __readonly_area (const char *ptr, size_t size)
>> continue;
>>
>> if (protection & VM_PROT_WRITE)
>> - return -1;
>> + return readonly_area_writable;
>
> OK. Same.
>
>>
>> if (region_address - (uintptr_t) ptr >= size)
>> break;
>> }
>>
>> - return 1;
>> + return readonly_noerror;
>
> OK. Same.
>
>> }
>
>> diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area-arch.c
>> similarity index 84%
>> rename from sysdeps/unix/sysv/linux/readonly-area.c
>> rename to sysdeps/unix/sysv/linux/readonly-area-arch.c
>> index 62d2070c72..3a33a9ceb4 100644
>> --- a/sysdeps/unix/sysv/linux/readonly-area.c
>> +++ b/sysdeps/unix/sysv/linux/readonly-area-arch.c
>
> OK. This is the Linux implementation that's now a back-up to the new
> link-map one.
>
>> @@ -23,11 +23,8 @@
>> #include <string.h>
>> #include "libio/libioP.h"
>>
>> -/* Return 1 if the whole area PTR .. PTR+SIZE is not writable.
>> - Return -1 if it is writable. */
>> -
>> -int
>> -__readonly_area (const char *ptr, size_t size)
>> +enum readonly_error_type
>> +__readonly_area_arch (const void *ptr, size_t size)
>
> OK. Adjust for new enum.
>
>> {
>> const void *ptr_end = ptr + size;
>>
>> @@ -42,11 +39,11 @@ __readonly_area (const char *ptr, size_t size)
>> to the /proc filesystem if it is set[ug]id. There has
>> been no willingness to change this in the kernel so
>> far. */
>> - || errno == EACCES
>> - /* Process has reached the maximum number of open files. */
>> - || errno == EMFILE)
>> - return 1;
>
> OK. Previously we treated ENOENT, EACCESS and EMFILE errors in the
> same way that we treated finding a readonly-area, i.e. no errors.
>
>> - return -1;
>
> OK. This corresponds to encountering any other fopen error and
> assuming writable area.
>
>> + || errno == EACCES)
>> + return readonly_procfs_inaccessible;
>
> OK. Now, we treat ENOENT and EACCESS as procfs being inaccessible.
>
>> + /* Process has reached the maximum number of open files or another
>> + unusual error. */
>> + return readonly_procfs_open_fail;
>
> OK. All other fopen errors (including EMFILE) are treated as a failure
> to open procfs.
>
>> }
>>
>> /* We need no locking. */
>> @@ -98,7 +95,5 @@ __readonly_area (const char *ptr, size_t size)
>> fclose (fp);
>> free (line);
>>
>> - /* If the whole area between ptr and ptr_end is covered by read-only
>> - VMAs, return 1. Otherwise return -1. */
>> - return size == 0 ? 1 : -1;
>> + return size == 0 ? readonly_noerror : readonly_area_writable;
>
> OK. Adjust for new enum.
>
>> }
>> --
>> 2.43.0
>>
>
More information about the Libc-alpha
mailing list