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 <[email protected]> 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/[email protected]?part=12
