Messages in this thread | | | Date | Tue, 16 May 2023 14:15:01 -0700 | From | Kees Cook <> | Subject | Re: [PATCH v2 06/10] fortify: strcat: Move definition to use fortified strlcat() |
| |
On Tue, Apr 18, 2023 at 11:09:41AM -0700, Nick Desaulniers wrote: > 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?
I wanted to collect all the str*cat functions together.
> > +/** > > + * 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?
I wanted to avoid repeating the same checks, so since strlcat() already does the right checking, I avoided repeating it here.
> What's the convention for __underlying_ vs __real_ prefixes in > include/linux/fortify-string.h?
__underlying may be wrapped by K*SAN before being implemented via __builtin, where as __real is used for things that aren't wrapped and/or aren't available with a __builtin (e.g. strscpy).
-- Kees Cook
| |