Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0ff0548e-7348-d6cb-75a5-838a110b7d82@intel.com>
Date:   Thu, 6 Apr 2023 17:23:54 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH 6/9] fortify: Split reporting and avoid passing string
 pointer
Content-Language: en-US
To:     Kees Cook <keescook@chromium.org>
CC:     <linux-hardening@vger.kernel.org>,
        Andy Shevchenko <andy@kernel.org>,
        Cezary Rojewski <cezary.rojewski@intel.com>,
        Puyou Lu <puyou.lu@gmail.com>, Mark Brown <broonie@kernel.org>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        "Peter Zijlstra" <peterz@infradead.org>,
        Brendan Higgins <brendan.higgins@linux.dev>,
        David Gow <davidgow@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Nathan Chancellor <nathan@kernel.org>,
        "Alexander Potapenko" <glider@google.com>,
        Zhaoyang Huang <zhaoyang.huang@unisoc.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Miguel Ojeda <ojeda@kernel.org>,
        "Nick Desaulniers" <ndesaulniers@google.com>,
        Liam Howlett <liam.howlett@oracle.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Dan Williams <dan.j.williams@intel.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        "Yury Norov" <yury.norov@gmail.com>,
        "Jason A. Donenfeld" <Jason@zx2c4.com>,
        "Sander Vanheule" <sander@svanheule.net>,
        Eric Biggers <ebiggers@google.com>,
        "Masami Hiramatsu (Google)" <mhiramat@kernel.org>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        "Daniel Latypov" <dlatypov@google.com>,
        =?UTF-8?B?Sm9zw6kgRXhww7NzaXRv?= <jose.exposito89@gmail.com>,
        <linux-kernel@vger.kernel.org>, <kunit-dev@googlegroups.com>
References: <20230405235832.never.487-kees@kernel.org>
 <20230406000212.3442647-6-keescook@chromium.org>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20230406000212.3442647-6-keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bXY4R0JZR294S3VyQ3grVXRwZmIzNjBZQXowT0QzR09POXhuS3IvNmtkUTQz?=
 =?utf-8?B?bVVSejcxeHNxWWZKbTRJY3BCcjkweG0zbDl3MDIwOXE1QlJ2VWJzbjRaRHhB?=
 =?utf-8?B?RjhiZlV6ZXVhM1E2am4vUnBvY053Z3FhRnNuL2Y3ejl2MTQ3MDdkK0pJZ21D?=
 =?utf-8?B?YWszeTdIMnQ3Nm51dzJZWlV6NWVFZVhnT2xSUHlYaUxTcGNPRXlFZHNnWStX?=
 =?utf-8?B?Um5vY1oyQ3N5UXkvM3NrTHRLSW9rVXBiSGJ0MGNsR2cvbDlTdjYzZUhIZ21h?=
 =?utf-8?B?RUcrNVpreDc1aHl5aTl4aVZZSHJ2MWh0ODhJRTRuQ3pqK0RJNDAzVG1uK1dK?=
 =?utf-8?B?RnU0b2YrNUlsOEFPUGsrbkpnZ1JaNEdUUTRseXdpS3JpejFTaWZxclo2YWlZ?=
 =?utf-8?B?d0RIMktaMjBFa3hiRVhOU1E1ZmF3VXR6RldMSWN5MXNwRG9jejZuQkhvbDh2?=
 =?utf-8?B?S3BYYzNXNXlEbnF3aE5NNG1odmtaVUxPR2ppVzlVZnZjTTFpUkdtUWtqa0pX?=
 =?utf-8?B?eW5weklZYS8rRlFWY0s0cnlOeksrSFpDaG51azZHY2hOS1U2clpvYXl6V0g4?=
 =?utf-8?B?OC9NeWl4ZnFWNkpROTFYVDZxMXdWYTdqUVVPSUovYTd4VGRrVnordi9Jekcx?=
 =?utf-8?B?ZldTYnovZDM3L0Z6UExCb0swalYzYS82UHBncFZuUnFadVNDelMvbDBFdkk4?=
 =?utf-8?B?eGxubFVxdm1EYkwvZXprOGlvVmFWTGQ1VnlCeVZ6dWdxV3J0Mi8rd1RwRFE1?=
 =?utf-8?B?elY2U0E5TlBQVWkwV0lYWGdGTUZ3eUE2eThRekRaT0pWbmVyYmxacnZJaG1q?=
 =?utf-8?B?Y3RmOFJESVg4c0xaSVBDZ0FhNlJreGloZEdwcXNIeGs3QzFLalZHWkpIVDdQ?=
 =?utf-8?B?a0VWUEdVcFFId244aW5ybEFFeTR3SEJHMnNHWmtBU09FMysvakpDaFRjZ25U?=
 =?utf-8?B?NTBLSVhwY2MxbDBHc041N0E4cEFyNk1IL0YrUnNVelZNR3dEck5vd0YvYXEy?=
 =?utf-8?B?SWhBWVVoemZWTEltSVhXd0hlenJ4dkQ3WlRZTHdhLzJYSm51bHYzOHRzaHZH?=
 =?utf-8?B?eWJXaHhGUVpjNWxuaUVkQk5oMHZUSFEzVW45dFp2alpIdmlXSjB4VHZxVWZW?=
 =?utf-8?B?RUpsVmFrTGpmNS95NXVaS2YxYjRSRFVTSktRVTZranhESWxhS0E0c3kzY1dI?=
 =?utf-8?B?L2Rqd2ZjUlRxOUJjdUs3Z2o0bVIzczhHc09tUExIcDBEQ0ZCMjRGZHZJRGk0?=
 =?utf-8?B?bS8vK2o3aVdrbDU3YS91UVVyaUxkTStmekUvTGRzbnM5NGlqZTdQeXNIbVE0?=
 =?utf-8?B?UzFYdml6SnVvbmIwaVZ2RWJBa3BCdkNxMDRUZHAzd2tJdXNpY0R2ZVpYNEgv?=
 =?utf-8?B?d1o5TDhoSjdiTEl3WnMxdStMTFNSSVZHb0UwcWtCREc4WWR4ODZ5YTczTmND?=
 =?utf-8?B?NjB1Vk1hamFmTmIwVFdteUlRYmo4SVFRMkxpQlJHVEl6TUhiSzZRcWtUODk2?=
 =?utf-8?B?QXFEbEROOERyRVZ2K0lkVENMVEVOVFVHKys3Ykw3QXNXbjNuWVB0a2U0bGRz?=
 =?utf-8?B?SWkyV0w5T3F4RHpyYzhoNkhObExqWVJNKzY2TENxQW9oeEhyNlRxUllrMFow?=
 =?utf-8?B?VjRYTldwcUx4VHpsVTEwNXhxMDg2STlvYzVHaUFuMHp6b0xtNmkzWEtFNmlQ?=
 =?utf-8?B?VW93Zk1YbE90UXUrOWoxbWRFOVR1V0lzNkY4ZlBLeUVWbExYanhuclBQZW5q?=
 =?utf-8?B?NjlOSS9XVXFNY3BlYVVqTEFhcnMwUkRKczBMdXB0K2ZMVFlrTElMK25JaDF4?=
 =?utf-8?B?Y0llamxHMit0TTg2TE9LdlZCcXVEaGhJVk0vU2xyZW9NclE3RkZ3SklWcmp5?=
 =?utf-8?B?MGpoY1Q4WFFNU1VPRHRjSTU0Um9CSzJKTk85Mk4ybXFkOVFHTlUrL1RIN3d5?=
 =?utf-8?B?K0lNYU0yK1dVV3Yrd09KUGl4ejd2ekdiT0VONG5QNVEwd2pWT2hoZXc2SzZZ?=
 =?utf-8?B?UDZ0d3FNcGwwUmRlemVDalV5Z0xNVW5lTVNQV0h3bWI4TFZ0OUhvK1QwdGR0?=
 =?utf-8?B?QzQ5VVJiZDZ4MVROYmx2Wk9nbldsdUwrOUJuWTQ4VkVuTmh1VFhaZmZ3d0Ro?=
 =?utf-8?B?RFVqZWFSdnZWVVFUQ0x4aXQ2dHlWcjJKNmVXb1RDWWhQK2NLeXh3aENNTjdm?=
 =?utf-8?B?cGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 998b5b28-9f8d-4484-2710-08db36b2f50d
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Apr 2023 15:24:04.9487
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: adqAtdpNzpZMa8dsZXjeY0faGf1Tx6Ejm060+PosOg7u8GihuRPDMnChAMiKbjYX8Nm3JezWE/pkRhyOBF+JBZ89u8fsfclLkaY8nnX/kII=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4764
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.7 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=unavailable autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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?

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