Message ID | 2c6783afcba4c3e9046be2908f64180280d1f520.1722071588.git.fweimer@redhat.com |
---|---|
State | New |
Headers | show |
Series | [1/2] Define __libc_initial for the static libc | expand |
On 27/07/24 06:13, Florian Weimer wrote: > Secondary namespaces have a different malloc. Allocating the > buffer in one namespace and freeing it another results in > heap corruption. Fix this by using a static string (potentially > translated) in secondary namespaces. It would also be possible > to use the malloc from the initial namespace to manage the > buffer, but these functions would still not be safe to use in > auditors etc. because a call to strerror could still free a > buffer while it is used by the application. Another approach > could use proper initial-exec TLS, duplicated in secondary > namespaces, but that would need a callback interface for freeing > libc resources in namespaces on thread exit, which does not exist > today. > > Tested on x86_64-linux-gnu. Looks okay, although it also means that all interfaces that potentially use malloc/free might be broken on auditors. I also think it needs a regression check. > > --- > string/strerror_l.c | 35 ++++++++++++++++++++++++----------- > string/strsignal.c | 34 ++++++++++++++++++++-------------- > 2 files changed, 44 insertions(+), 25 deletions(-) > > diff --git a/string/strerror_l.c b/string/strerror_l.c > index 15cce261e6..70456e5bb4 100644 > --- a/string/strerror_l.c > +++ b/string/strerror_l.c > @@ -20,7 +20,7 @@ > #include <stdio.h> > #include <string.h> > #include <tls-internal.h> > - > +#include <libc-internal.h> Spurious line removal? > > static const char * > translate (const char *str, locale_t loc) > @@ -31,6 +31,12 @@ translate (const char *str, locale_t loc) > return res; > } > > +static char * > +unknown_error (locale_t loc) > +{ > + return (char *) translate ("Unknown error", loc); > +} > + > > /* Return a string describing the errno code in ERRNUM. */ > char * > @@ -40,18 +46,25 @@ __strerror_l (int errnum, locale_t loc) > char *err = (char *) __get_errlist (errnum); > if (__glibc_unlikely (err == NULL)) > { > - struct tls_internal_t *tls_internal = __glibc_tls_internal (); > - free (tls_internal->strerror_l_buf); > - if (__asprintf (&tls_internal->strerror_l_buf, "%s%d", > - translate ("Unknown error ", loc), errnum) > 0) > - err = tls_internal->strerror_l_buf; > - else > + if (__libc_initial) > { > - /* The memory was freed above. */ > - tls_internal->strerror_l_buf = NULL; > - /* Provide a fallback translation. */ > - err = (char *) translate ("Unknown error", loc); > + struct tls_internal_t *tls_internal = __glibc_tls_internal (); > + free (tls_internal->strerror_l_buf); > + if (__asprintf (&tls_internal->strerror_l_buf, "%s%d", > + translate ("Unknown error ", loc), errnum) > 0) > + err = tls_internal->strerror_l_buf; > + else > + { > + /* The memory was freed above. */ > + tls_internal->strerror_l_buf = NULL; > + /* Provide a fallback translation. */ > + err = unknown_error (loc); > + } > } > + else > + /* Secondary namespaces use a different malloc, so cannot > + participate in the buffer management. */ > + err = unknown_error (loc); > } > else > err = (char *) translate (err, loc); > diff --git a/string/strsignal.c b/string/strsignal.c > index 3114601564..808e474e15 100644 > --- a/string/strsignal.c > +++ b/string/strsignal.c > @@ -21,6 +21,7 @@ > #include <string.h> > #include <libintl.h> > #include <tls-internal.h> > +#include <libc-internal.h> > > /* Return a string describing the meaning of the signal number SIGNUM. */ > char * > @@ -30,21 +31,26 @@ strsignal (int signum) > if (desc != NULL) > return _(desc); > > - struct tls_internal_t *tls_internal = __glibc_tls_internal (); > - free (tls_internal->strsignal_buf); > + if (__libc_initial) > + { > + struct tls_internal_t *tls_internal = __glibc_tls_internal (); > + free (tls_internal->strsignal_buf); > > - int r; > + int r; > #ifdef SIGRTMIN > - if (signum >= SIGRTMIN && signum <= SIGRTMAX) > - r = __asprintf (&tls_internal->strsignal_buf, _("Real-time signal %d"), > - signum - SIGRTMIN); > - else > + if (signum >= SIGRTMIN && signum <= SIGRTMAX) > + r = __asprintf (&tls_internal->strsignal_buf, _("Real-time signal %d"), > + signum - SIGRTMIN); > + else > #endif > - r = __asprintf (&tls_internal->strsignal_buf, _("Unknown signal %d"), > - signum); > - > - if (r == -1) > - tls_internal->strsignal_buf = NULL; > - > - return tls_internal->strsignal_buf; > + r = __asprintf (&tls_internal->strsignal_buf, _("Unknown signal %d"), > + signum); > + > + if (r >= 0) > + return tls_internal->strsignal_buf; > + } > + /* Fall through on asprintf error, and for !__libc_initial: > + secondary namespaces use a different malloc and cannot > + participate in the buffer management. */ > + return _("Unknown signal"); > }
* Adhemerval Zanella Netto: > On 27/07/24 06:13, Florian Weimer wrote: >> Secondary namespaces have a different malloc. Allocating the >> buffer in one namespace and freeing it another results in >> heap corruption. Fix this by using a static string (potentially >> translated) in secondary namespaces. It would also be possible >> to use the malloc from the initial namespace to manage the >> buffer, but these functions would still not be safe to use in >> auditors etc. because a call to strerror could still free a >> buffer while it is used by the application. Another approach >> could use proper initial-exec TLS, duplicated in secondary >> namespaces, but that would need a callback interface for freeing >> libc resources in namespaces on thread exit, which does not exist >> today. >> >> Tested on x86_64-linux-gnu. > > Looks okay, although it also means that all interfaces that potentially > use malloc/free might be broken on auditors. The issue here is that this kind of internal TLS isn't replicated along with libc. Most other uses are. We did have a similar issue with dlerror, which I fixed a while back, by making sure that the right malloc and free were called. > I also think it needs a regression check. I tried to write a regression test, but it's difficult to get a reliable crash because tcache, fastbins etc. tend to be interoperable between the mallocs. The test didn't look very valuable to me. Should I posted a v2 with the whitespace removal glitch fixed? Thanks, Florian
On 02/08/24 10:41, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> On 27/07/24 06:13, Florian Weimer wrote: >>> Secondary namespaces have a different malloc. Allocating the >>> buffer in one namespace and freeing it another results in >>> heap corruption. Fix this by using a static string (potentially >>> translated) in secondary namespaces. It would also be possible >>> to use the malloc from the initial namespace to manage the >>> buffer, but these functions would still not be safe to use in >>> auditors etc. because a call to strerror could still free a >>> buffer while it is used by the application. Another approach >>> could use proper initial-exec TLS, duplicated in secondary >>> namespaces, but that would need a callback interface for freeing >>> libc resources in namespaces on thread exit, which does not exist >>> today. >>> >>> Tested on x86_64-linux-gnu. >> >> Looks okay, although it also means that all interfaces that potentially >> use malloc/free might be broken on auditors. > > The issue here is that this kind of internal TLS isn't replicated along > with libc. Most other uses are. We did have a similar issue with > dlerror, which I fixed a while back, by making sure that the right > malloc and free were called. Yeah, that's what I referring meaning that we will need extra care on some interfaces to get them right for auditors. > >> I also think it needs a regression check. > > I tried to write a regression test, but it's difficult to get a reliable > crash because tcache, fastbins etc. tend to be interoperable between the > mallocs. The test didn't look very valuable to me. > Fair enough. > Should I posted a v2 with the whitespace removal glitch fixed? No need. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
* Florian Weimer: > diff --git a/string/strsignal.c b/string/strsignal.c > index 3114601564..808e474e15 100644 > --- a/string/strsignal.c > +++ b/string/strsignal.c > @@ -21,6 +21,7 @@ > #include <string.h> > #include <libintl.h> > #include <tls-internal.h> > +#include <libc-internal.h> > > /* Return a string describing the meaning of the signal number SIGNUM. */ > char * > @@ -30,21 +31,26 @@ strsignal (int signum) > if (desc != NULL) > return _(desc); > > - struct tls_internal_t *tls_internal = __glibc_tls_internal (); > - free (tls_internal->strsignal_buf); > + if (__libc_initial) > + { > + struct tls_internal_t *tls_internal = __glibc_tls_internal (); > + free (tls_internal->strsignal_buf); > > - int r; > + int r; > #ifdef SIGRTMIN > - if (signum >= SIGRTMIN && signum <= SIGRTMAX) > - r = __asprintf (&tls_internal->strsignal_buf, _("Real-time signal %d"), > - signum - SIGRTMIN); > - else > + if (signum >= SIGRTMIN && signum <= SIGRTMAX) > + r = __asprintf (&tls_internal->strsignal_buf, _("Real-time signal %d"), > + signum - SIGRTMIN); > + else > #endif > - r = __asprintf (&tls_internal->strsignal_buf, _("Unknown signal %d"), > - signum); > - > - if (r == -1) > - tls_internal->strsignal_buf = NULL; > - > - return tls_internal->strsignal_buf; > + r = __asprintf (&tls_internal->strsignal_buf, _("Unknown signal %d"), > + signum); > + > + if (r >= 0) > + return tls_internal->strsignal_buf; > + } > + /* Fall through on asprintf error, and for !__libc_initial: > + secondary namespaces use a different malloc and cannot > + participate in the buffer management. */ > + return _("Unknown signal"); > } Sadly this introduces a double-free bug if asprintf fails. The “if (r == -1) tls_internal->strsignal_buf = NULL;” part was required. Will repost a fixed version. Florian
diff --git a/string/strerror_l.c b/string/strerror_l.c index 15cce261e6..70456e5bb4 100644 --- a/string/strerror_l.c +++ b/string/strerror_l.c @@ -20,7 +20,7 @@ #include <stdio.h> #include <string.h> #include <tls-internal.h> - +#include <libc-internal.h> static const char * translate (const char *str, locale_t loc) @@ -31,6 +31,12 @@ translate (const char *str, locale_t loc) return res; } +static char * +unknown_error (locale_t loc) +{ + return (char *) translate ("Unknown error", loc); +} + /* Return a string describing the errno code in ERRNUM. */ char * @@ -40,18 +46,25 @@ __strerror_l (int errnum, locale_t loc) char *err = (char *) __get_errlist (errnum); if (__glibc_unlikely (err == NULL)) { - struct tls_internal_t *tls_internal = __glibc_tls_internal (); - free (tls_internal->strerror_l_buf); - if (__asprintf (&tls_internal->strerror_l_buf, "%s%d", - translate ("Unknown error ", loc), errnum) > 0) - err = tls_internal->strerror_l_buf; - else + if (__libc_initial) { - /* The memory was freed above. */ - tls_internal->strerror_l_buf = NULL; - /* Provide a fallback translation. */ - err = (char *) translate ("Unknown error", loc); + struct tls_internal_t *tls_internal = __glibc_tls_internal (); + free (tls_internal->strerror_l_buf); + if (__asprintf (&tls_internal->strerror_l_buf, "%s%d", + translate ("Unknown error ", loc), errnum) > 0) + err = tls_internal->strerror_l_buf; + else + { + /* The memory was freed above. */ + tls_internal->strerror_l_buf = NULL; + /* Provide a fallback translation. */ + err = unknown_error (loc); + } } + else + /* Secondary namespaces use a different malloc, so cannot + participate in the buffer management. */ + err = unknown_error (loc); } else err = (char *) translate (err, loc); diff --git a/string/strsignal.c b/string/strsignal.c index 3114601564..808e474e15 100644 --- a/string/strsignal.c +++ b/string/strsignal.c @@ -21,6 +21,7 @@ #include <string.h> #include <libintl.h> #include <tls-internal.h> +#include <libc-internal.h> /* Return a string describing the meaning of the signal number SIGNUM. */ char * @@ -30,21 +31,26 @@ strsignal (int signum) if (desc != NULL) return _(desc); - struct tls_internal_t *tls_internal = __glibc_tls_internal (); - free (tls_internal->strsignal_buf); + if (__libc_initial) + { + struct tls_internal_t *tls_internal = __glibc_tls_internal (); + free (tls_internal->strsignal_buf); - int r; + int r; #ifdef SIGRTMIN - if (signum >= SIGRTMIN && signum <= SIGRTMAX) - r = __asprintf (&tls_internal->strsignal_buf, _("Real-time signal %d"), - signum - SIGRTMIN); - else + if (signum >= SIGRTMIN && signum <= SIGRTMAX) + r = __asprintf (&tls_internal->strsignal_buf, _("Real-time signal %d"), + signum - SIGRTMIN); + else #endif - r = __asprintf (&tls_internal->strsignal_buf, _("Unknown signal %d"), - signum); - - if (r == -1) - tls_internal->strsignal_buf = NULL; - - return tls_internal->strsignal_buf; + r = __asprintf (&tls_internal->strsignal_buf, _("Unknown signal %d"), + signum); + + if (r >= 0) + return tls_internal->strsignal_buf; + } + /* Fall through on asprintf error, and for !__libc_initial: + secondary namespaces use a different malloc and cannot + participate in the buffer management. */ + return _("Unknown signal"); }