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

[Alexander Lobakin]

From: Kees Cook <keescook@xxxxxxxxxxxx>
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@xxxxxxxxxx>
> Cc: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>
> Cc: Puyou Lu <puyou.lu@xxxxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxx>
> Cc: linux-hardening@xxxxxxxxxxxxxxx
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
>  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?

> --- 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)

(+ set pair)

> +	case FORTIFY_FUNC_strncpy:
> +		name = "strncpy";
> +		break;
> +	case FORTIFY_FUNC_strnlen:
> +		name = "strnlen";
> +		break;
> +	case FORTIFY_FUNC_strlen:
> +		name = "strlen";
> +		break;
> +	case FORTIFY_FUNC_strlcpy:
> +		name = "strlcpy";
> +		break;
> +	case FORTIFY_FUNC_strscpy:
> +		name = "strscpy";
> +		break;
> +	case FORTIFY_FUNC_strlcat:
> +		name = "strlcat";
> +		break;
> +	case FORTIFY_FUNC_strcat:
> +		name = "strcat";
> +		break;
> +	case FORTIFY_FUNC_strncat:
> +		name = "strncat";
> +		break;
> +	case FORTIFY_FUNC_memset:
> +		name = "memset";
> +		break;
> +	case FORTIFY_FUNC_memcpy:
> +		name = "memcpy";
> +		break;
> +	case FORTIFY_FUNC_memmove:
> +		name = "memmove";
> +		break;
> +	case FORTIFY_FUNC_memscan:
> +		name = "memscan";
> +		break;
> +	case FORTIFY_FUNC_memcmp:
> +		name = "memcmp";
> +		break;
> +	case FORTIFY_FUNC_memchr:
> +		name = "memchr";
> +		break;
> +	case FORTIFY_FUNC_memchr_inv:
> +		name = "memchr_inv";
> +		break;
> +	case FORTIFY_FUNC_kmemdup:
> +		name = "kmemdup";
> +		break;
> +	case FORTIFY_FUNC_strcpy:
> +		name = "strcpy";
> +		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];

> +	WARN(1, "%s: detected buffer %s overflow\n", name, write ? "write" : "read");
> +}
> +EXPORT_SYMBOL(__fortify_report);
> +
> +void __fortify_panic(const u8 reason)
> +{
> +	__fortify_report(reason);
>  	BUG();
>  }
> -EXPORT_SYMBOL(fortify_panic);
> +EXPORT_SYMBOL(__fortify_panic);
>  #endif /* CONFIG_FORTIFY_SOURCE */
Thanks,
Olek