Re: [PATCH 12/28] fortify: Convert strlcat() to snprintf() in strcat()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] fortify: strcat using snprintf causes major performance regression
- [High] fortify: snprintf in strcat bypasses FORTIFY_READ bounds on source
--

commit d5360c3f44eeb8b6f9cd8cc195950107cbfe6556
Author: Bill Wendling <morbo@google.com>

fortify: Convert strlcat() to snprintf() in strcat()

Replaces the strlcat() implementation inside strcat() with
__builtin_snprintf() when the destination size is known, and falls back to
__underlying_strcat() otherwise. This prepares for the removal of the
deprecated strlcat() API.

Link: https://github.com/KSPP/linux/issues/370 [1]

> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index cf841dc71feff..0b489124bfcb8 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -363,7 +363,12 @@ __FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2)
>  char *strcat(char * const POS p, const char *q)
>  {
>  	const size_t p_size = __member_size(p);
> -	const size_t wanted = strlcat(p, q, p_size);
> +
> +	if (p_size == SIZE_MAX)
> +		return __underlying_strcat(p, q);

[Severity: High]
Does falling back to the unfortified __underlying_strcat() directly here skip
the FORTIFY_READ bounds checks on the source string q?

The previous strlcat() fallback delegated to __fortify_strlen(q), which safely
bounded the read and triggered a panic if q was not NUL-terminated within its
allocation. Could this bypass allow unbounded reads if q lacks a NUL
terminator?

> +
> +	const size_t p_len = __fortify_strlen(p);
> +	const size_t wanted = p_len + __builtin_snprintf(p + p_len, p_size - p_len, "%s", q);

[Severity: High]
Will injecting __builtin_snprintf() for a fundamental string primitive like
strcat() cause a significant performance regression and stack bloat?

Because the size parameter is dynamic, the compiler cannot optimize this away,
meaning this will call into the heavy vsnprintf() format parsing engine.

Given that vsnprintf() has a notoriously large stack footprint, could this
increase the risk of kernel stack overflows in deep call chains?

Additionally, does passing q as an opaque variadic argument to snprintf()
defeat the compiler's __builtin_object_size() tracking?

Since the vsnprintf() engine will process "%s" using an unbounded strlen(q),
could this bypass the intended FORTIFY_READ mitigation and leak information
or crash if q is missing a NUL terminator?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915081910.4142719-1-morbo@google.com?part=12