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