Messages in this thread | | | From | Nick Desaulniers <> | Date | Tue, 18 Apr 2023 11:09:41 -0700 | Subject | Re: [PATCH v2 06/10] fortify: strcat: Move definition to use fortified strlcat() |
| |
On Fri, Apr 7, 2023 at 12:27 PM Kees Cook <keescook@chromium.org> wrote: > > Move the definition of fortified strcat() to after strlcat() to use it > for bounds checking. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > include/linux/fortify-string.h | 53 +++++++++++++++++----------------- > 1 file changed, 26 insertions(+), 27 deletions(-) > > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h > index 8cf17ef81905..ab058d092817 100644 > --- a/include/linux/fortify-string.h > +++ b/include/linux/fortify-string.h > @@ -151,33 +151,6 @@ char *strncpy(char * const POS p, const char *q, __kernel_size_t size) > return __underlying_strncpy(p, q, size); > } > > -/** > - * strcat - Append a string to an existing string > - * > - * @p: pointer to NUL-terminated string to append to > - * @q: pointer to NUL-terminated source string to append from > - * > - * Do not use this function. While FORTIFY_SOURCE tries to avoid > - * read and write overflows, this is only possible when the > - * destination buffer size is known to the compiler. Prefer > - * building the string with formatting, via scnprintf() or similar. > - * At the very least, use strncat(). > - * > - * Returns @p. > - * > - */ > -__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); > - > - if (p_size == SIZE_MAX) > - return __underlying_strcat(p, q); > - if (strlcat(p, q, p_size) >= p_size) > - fortify_panic(__func__); > - return p; > -} > - > extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen); > /** > * strnlen - Return bounded count of characters in a NUL-terminated string > @@ -435,6 +408,32 @@ size_t strlcat(char * const POS p, const char * const POS q, size_t avail) > return wanted; > } > > +/* Defined after fortified strlcat() to reuse it. */
I don't follow; the previous location was already defined in terms of calls to strlcat. Why is this patch necessary?
Could this be fixed in 5/10 https://lore.kernel.org/linux-hardening/20230407192717.636137-5-keescook@chromium.org/ by just putting strlcat in the expected place in the first place?
> +/** > + * strcat - Append a string to an existing string > + * > + * @p: pointer to NUL-terminated string to append to > + * @q: pointer to NUL-terminated source string to append from > + * > + * Do not use this function. While FORTIFY_SOURCE tries to avoid > + * read and write overflows, this is only possible when the > + * destination buffer size is known to the compiler. Prefer > + * building the string with formatting, via scnprintf() or similar. > + * At the very least, use strncat(). > + * > + * Returns @p. > + * > + */ > +__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); > +
This drops the `p_size == SIZE_MAX` guard. Might it be faster at runtime to dispatch to __underlying_strcat rather than __real_strlcat in such cases?
What's the convention for __underlying_ vs __real_ prefixes in include/linux/fortify-string.h?
> + if (strlcat(p, q, p_size) >= p_size) > + fortify_panic(__func__); > + return p; > +} > + > /** > * strncat - Append a string to an existing string > * > -- > 2.34.1 >
-- Thanks, ~Nick Desaulniers
| |