| 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: <0ff0548e-7348-d6cb-75a5-838a110b7d82@intel.com>
Date: Thu, 6 Apr 2023 17:23:54 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Kees Cook <keescook@...omium.org>
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
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?
> --- 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
Powered by blists - more mailing lists