Message ID | 20220411174956.2657622-1-joanbrugueram@gmail.com |
---|---|
State | New |
Headers | show |
Series | misc: Fix rare fortify crash on wchar funcs. [BZ 29030] | expand |
On 11/04/2022 23:19, Joan Bruguera wrote: > If `__glibc_objsize (__o) == (size_t) -1` (i.e. `__o` is unknown size), fortify > checks should pass, and `__whatever_alias` should be called. > > Previously, `__glibc_objsize (__o) == (size_t) -1` was explicitly checked, but > on commit a643f60c53876b, this was moved into `__glibc_safe_or_unknown_len`. > > A comment says the -1 case should work as: "The -1 check is redundant because > since it implies that __glibc_safe_len_cond is true.". But this fails when: > * `__s > 1` > * `__osz == -1` (i.e. unknown size at compile time) > * `__l` is big enough > * `__l * __s <= __osz` can be folded to a constant > (I only found this to be true for `mbsrtowcs` and other functions in wchar2.h) > > In this case `__l * __s <= __osz` is false, and `__whatever_chk_warn` will be > called by `__glibc_fortify` or `__glibc_fortify_n` and crash the program. > > This commit adds the explicit `__osz == -1` check again. > moc crashes on startup due to this, see: https://bugs.archlinux.org/task/74041 > > Minimal test case (test.c): > #include <wchar.h> > > int main (void) > { > const char *hw = "HelloWorld"; > mbsrtowcs (NULL, &hw, (size_t)-1, NULL); > return 0; > } > > Build with: > gcc -O2 -Wp,-D_FORTIFY_SOURCE=2 test.c -o test && ./test > > Output: > *** buffer overflow detected ***: terminated Thank you Joan, just a couple of nits, please send a v2 with those fixed and I'll push it for you: > > Fixes: a643f60c53876b ("Make sure that the fortified function conditionals are constant") > Fixes: https://sourceware.org/bugzilla/show_bug.cgi?id=29030 Please mention only the bug number, i.e.: Fixes: BZ #29030 > Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com> > --- > debug/tst-fortify.c | 5 +++++ > misc/sys/cdefs.h | 6 +++--- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/debug/tst-fortify.c b/debug/tst-fortify.c > index d65a2fe6e1..03c9867714 100644 > --- a/debug/tst-fortify.c > +++ b/debug/tst-fortify.c > @@ -1504,6 +1504,11 @@ do_test (void) > CHK_FAIL_END > #endif > > + /* Bug 29030 regresion check */ > + cp = "HelloWorld"; > + if (mbsrtowcs (NULL, &cp, (size_t)-1, &s) != 10) > + FAIL (); > + Test case, perfect! > cp = "A"; > if (mbstowcs (wenough, cp, 10) != 1 > || wcscmp (wenough, L"A") != 0) > diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h > index 44d3826bca..b8419e7e6c 100644 > --- a/misc/sys/cdefs.h > +++ b/misc/sys/cdefs.h > @@ -156,14 +156,14 @@ > variants. These conditions should get evaluated to constant and optimized > away. */ > > -#define __glibc_safe_len_cond(__l, __s, __osz) ((__l) <= (__osz) / (__s)) > +#define __glibc_safe_len_cond(__l, __s, __osz) \ > + ((__osz) == (size_t) -1 || ((__l) <= (__osz) / (__s))) This should go... > #define __glibc_unsigned_or_positive(__l) \ > ((__typeof (__l)) 0 < (__typeof (__l)) -1 \ > || (__builtin_constant_p (__l) && (__l) > 0)) > > /* Length is known to be safe at compile time if the __L * __S <= __OBJSZ > - condition can be folded to a constant and if it is true. The -1 check is > - redundant because since it implies that __glibc_safe_len_cond is true. */ > + condition can be folded to a constant and if it is true, or unknown (-1) */ > #define __glibc_safe_or_unknown_len(__l, __s, __osz) \ here since the above macro is "safe length condition" and this one is "safe or unknown length", so the unknown length check ideally belongs here. > (__glibc_unsigned_or_positive (__l) \ > && __builtin_constant_p (__glibc_safe_len_cond ((__SIZE_TYPE__) (__l), \ Thanks, Siddhesh
On 18/04/2022 10:14, Siddhesh Poyarekar wrote: >> cp = "A"; >> if (mbstowcs (wenough, cp, 10) != 1 >> || wcscmp (wenough, L"A") != 0) >> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h >> index 44d3826bca..b8419e7e6c 100644 >> --- a/misc/sys/cdefs.h >> +++ b/misc/sys/cdefs.h >> @@ -156,14 +156,14 @@ >> variants. These conditions should get evaluated to constant and >> optimized >> away. */ >> -#define __glibc_safe_len_cond(__l, __s, __osz) ((__l) <= (__osz) / >> (__s)) >> +#define __glibc_safe_len_cond(__l, __s, __osz) \ >> + ((__osz) == (size_t) -1 || ((__l) <= (__osz) / (__s))) > > This should go... > >> #define __glibc_unsigned_or_positive(__l) \ >> ((__typeof (__l)) 0 < (__typeof (__l)) -1 \ >> || (__builtin_constant_p (__l) && (__l) > 0)) >> /* Length is known to be safe at compile time if the __L * __S <= >> __OBJSZ >> - condition can be folded to a constant and if it is true. The -1 >> check is >> - redundant because since it implies that __glibc_safe_len_cond is >> true. */ >> + condition can be folded to a constant and if it is true, or >> unknown (-1) */ >> #define __glibc_safe_or_unknown_len(__l, __s, __osz) \ > > here since the above macro is "safe length condition" and this one is > "safe or unknown length", so the unknown length check ideally belongs here. Sorry I missed another note, please use __SIZE_TYPE__ instead of size_t to avoid having to include stddef.h. Thanks, Siddhesh
diff --git a/debug/tst-fortify.c b/debug/tst-fortify.c index d65a2fe6e1..03c9867714 100644 --- a/debug/tst-fortify.c +++ b/debug/tst-fortify.c @@ -1504,6 +1504,11 @@ do_test (void) CHK_FAIL_END #endif + /* Bug 29030 regresion check */ + cp = "HelloWorld"; + if (mbsrtowcs (NULL, &cp, (size_t)-1, &s) != 10) + FAIL (); + cp = "A"; if (mbstowcs (wenough, cp, 10) != 1 || wcscmp (wenough, L"A") != 0) diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h index 44d3826bca..b8419e7e6c 100644 --- a/misc/sys/cdefs.h +++ b/misc/sys/cdefs.h @@ -156,14 +156,14 @@ variants. These conditions should get evaluated to constant and optimized away. */ -#define __glibc_safe_len_cond(__l, __s, __osz) ((__l) <= (__osz) / (__s)) +#define __glibc_safe_len_cond(__l, __s, __osz) \ + ((__osz) == (size_t) -1 || ((__l) <= (__osz) / (__s))) #define __glibc_unsigned_or_positive(__l) \ ((__typeof (__l)) 0 < (__typeof (__l)) -1 \ || (__builtin_constant_p (__l) && (__l) > 0)) /* Length is known to be safe at compile time if the __L * __S <= __OBJSZ - condition can be folded to a constant and if it is true. The -1 check is - redundant because since it implies that __glibc_safe_len_cond is true. */ + condition can be folded to a constant and if it is true, or unknown (-1) */ #define __glibc_safe_or_unknown_len(__l, __s, __osz) \ (__glibc_unsigned_or_positive (__l) \ && __builtin_constant_p (__glibc_safe_len_cond ((__SIZE_TYPE__) (__l), \
If `__glibc_objsize (__o) == (size_t) -1` (i.e. `__o` is unknown size), fortify checks should pass, and `__whatever_alias` should be called. Previously, `__glibc_objsize (__o) == (size_t) -1` was explicitly checked, but on commit a643f60c53876b, this was moved into `__glibc_safe_or_unknown_len`. A comment says the -1 case should work as: "The -1 check is redundant because since it implies that __glibc_safe_len_cond is true.". But this fails when: * `__s > 1` * `__osz == -1` (i.e. unknown size at compile time) * `__l` is big enough * `__l * __s <= __osz` can be folded to a constant (I only found this to be true for `mbsrtowcs` and other functions in wchar2.h) In this case `__l * __s <= __osz` is false, and `__whatever_chk_warn` will be called by `__glibc_fortify` or `__glibc_fortify_n` and crash the program. This commit adds the explicit `__osz == -1` check again. moc crashes on startup due to this, see: https://bugs.archlinux.org/task/74041 Minimal test case (test.c): #include <wchar.h> int main (void) { const char *hw = "HelloWorld"; mbsrtowcs (NULL, &hw, (size_t)-1, NULL); return 0; } Build with: gcc -O2 -Wp,-D_FORTIFY_SOURCE=2 test.c -o test && ./test Output: *** buffer overflow detected ***: terminated Fixes: a643f60c53876b ("Make sure that the fortified function conditionals are constant") Fixes: https://sourceware.org/bugzilla/show_bug.cgi?id=29030 Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com> --- debug/tst-fortify.c | 5 +++++ misc/sys/cdefs.h | 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-)