Re: [PATCH 6/9] fortify: Split reporting and avoid passing string pointer

[Kees Cook <keescook@chromium.org>]

On Thu, Apr 06, 2023 at 05:23:54PM +0200, Alexander Lobakin wrote:
> From: Kees Cook <keescook@chromium.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@kernel.org>
> > Cc: Cezary Rojewski <cezary.rojewski@intel.com>
> > Cc: Puyou Lu <puyou.lu@gmail.com>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: linux-hardening@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.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