| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <642f4d8a.630a0220.179b4.3576@mx.google.com>
Date: Thu, 6 Apr 2023 15:54:01 -0700
From: Kees Cook <keescook@...omium.org>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: linux-hardening@...r.kernel.org, Andy Shevchenko <andy@...nel.org>,
Cezary Rojewski <cezary.rojewski@...el.com>,
Puyou Lu <puyou.lu@...il.com>, Mark Brown <broonie@...nel.org>,
Josh Poimboeuf <jpoimboe@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Brendan Higgins <brendan.higgins@...ux.dev>,
David Gow <davidgow@...gle.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Nathan Chancellor <nathan@...nel.org>,
Alexander Potapenko <glider@...gle.com>,
Zhaoyang Huang <zhaoyang.huang@...soc.com>,
Randy Dunlap <rdunlap@...radead.org>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Miguel Ojeda <ojeda@...nel.org>,
Nick Desaulniers <ndesaulniers@...gle.com>,
Liam Howlett <liam.howlett@...cle.com>,
Vlastimil Babka <vbabka@...e.cz>,
Dan Williams <dan.j.williams@...el.com>,
Rasmus Villemoes <linux@...musvillemoes.dk>,
Yury Norov <yury.norov@...il.com>,
"Jason A. Donenfeld" <Jason@...c4.com>,
Sander Vanheule <sander@...nheule.net>,
Eric Biggers <ebiggers@...gle.com>,
"Masami Hiramatsu (Google)" <mhiramat@...nel.org>,
Andrey Konovalov <andreyknvl@...il.com>,
Linus Walleij <linus.walleij@...aro.org>,
Daniel Latypov <dlatypov@...gle.com>,
José Expósito <jose.exposito89@...il.com>,
linux-kernel@...r.kernel.org, kunit-dev@...glegroups.com
Subject: Re: [PATCH 6/9] fortify: Split reporting and avoid passing string
pointer
On Thu, Apr 06, 2023 at 05:23:54PM +0200, Alexander Lobakin wrote:
> From: Kees Cook <keescook@...omium.org>
> Date: Wed, 5 Apr 2023 17:02:05 -0700
>
> > In preparation for KUnit testing and further improvements in fortify
> > failure reporting, split out the report and encode the function and
> > access failure (read or write overflow) into a single int argument. This
> > mainly ends up saving some space in the data segment. For a defconfig
> > with FORTIFY_SOURCE enabled:
> >
> > $ size gcc/vmlinux.before gcc/vmlinux.after
> > text data bss dec hex filename
> > 26132309 9760658 2195460 38088427 2452eeb gcc/vmlinux.before
> > 26132386 9748382 2195460 38076228 244ff44 gcc/vmlinux.after
> >
> > Cc: Andy Shevchenko <andy@...nel.org>
> > Cc: Cezary Rojewski <cezary.rojewski@...el.com>
> > Cc: Puyou Lu <puyou.lu@...il.com>
> > Cc: Mark Brown <broonie@...nel.org>
> > Cc: linux-hardening@...r.kernel.org
> > Signed-off-by: Kees Cook <keescook@...omium.org>
> > ---
> > include/linux/fortify-string.h | 72 +++++++++++++++++++++++-----------
> > lib/string_helpers.c | 70 +++++++++++++++++++++++++++++++--
> > tools/objtool/check.c | 2 +-
> > 3 files changed, 118 insertions(+), 26 deletions(-)
> >
> > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> > index 41dbd641f55c..6db4052db459 100644
> > --- a/include/linux/fortify-string.h
> > +++ b/include/linux/fortify-string.h
> > @@ -9,7 +9,34 @@
> > #define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable
> > #define __RENAME(x) __asm__(#x)
> >
> > -void fortify_panic(const char *name) __noreturn __cold;
> > +#define fortify_reason(func, write) (((func) << 1) | !!(write))
> > +
> > +#define fortify_panic(func, write) \
> > + __fortify_panic(fortify_reason(func, write))
> > +
> > +#define FORTIFY_READ 0
> > +#define FORTIFY_WRITE 1
> > +
> > +#define FORTIFY_FUNC_strncpy 0
> > +#define FORTIFY_FUNC_strnlen 1
> > +#define FORTIFY_FUNC_strlen 2
> > +#define FORTIFY_FUNC_strlcpy 3
> > +#define FORTIFY_FUNC_strscpy 4
> > +#define FORTIFY_FUNC_strlcat 5
> > +#define FORTIFY_FUNC_strcat 6
> > +#define FORTIFY_FUNC_strncat 7
> > +#define FORTIFY_FUNC_memset 8
> > +#define FORTIFY_FUNC_memcpy 9
> > +#define FORTIFY_FUNC_memmove 10
> > +#define FORTIFY_FUNC_memscan 11
> > +#define FORTIFY_FUNC_memcmp 12
> > +#define FORTIFY_FUNC_memchr 13
> > +#define FORTIFY_FUNC_memchr_inv 14
> > +#define FORTIFY_FUNC_kmemdup 15
> > +#define FORTIFY_FUNC_strcpy 16
>
> enum?
I opted to avoid an enum due to the binary operations used in
"fortify_reason" to collapse it to a u8. It just seemed like more work
to put in enums, and then do u8 casts, etc, all for a strictly
"internal" set of magic numbers.
>
> > --- a/lib/string_helpers.c
> > +++ b/lib/string_helpers.c
> > @@ -1021,10 +1021,74 @@ EXPORT_SYMBOL(__read_overflow2_field);
> > void __write_overflow_field(size_t avail, size_t wanted) { }
> > EXPORT_SYMBOL(__write_overflow_field);
> >
> > -void fortify_panic(const char *name)
> > +void __fortify_report(u8 reason)
> > {
> > - pr_emerg("detected buffer overflow in %s\n", name);
> > + const char *name;
> > + const bool write = !!(reason & 0x1);
> > +
> > + switch (reason >> 1) {
>
> As already mentioned, I'd use bitfield helpers + couple definitions to
> not miss something when changing the way it's encoded
>
> #define FORTIFY_REASON_DIR(r) FIELD_GET(BIT(0), r)
> #define FORTIFY_REASON_FUNC(r) FIELD_GET(GENMASK(7, 1), r)
Yeah, good idea. Thanks for the FIELD_GET examples!
> [...]
> > + break;
> > + default:
> > + name = "unknown";
> > + }
>
> I know this is far from hotpath, but could we save some object code and
> do that via O(1) array lookup? Plus some macro to compress things:
>
> #define FORTIFY_ENTRY(name) \
> [FORTIFY_FUNC_##name] = #name
>
> static const char * const fortify_funcs[] = {
> FORTIFY_ENTRY(strncpy),
> ...
> }
>
> // array bounds check here if you wish, I wouldn't bother as
> // we're panicking anyway
>
> name = fortify_funcs[reason >> 1];
Fair enough. I had considered it and then forgot about it for some
reason. :)
--
Kees Cook
Powered by blists - more mailing lists