| Message ID | 20230407192717.636137-6-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fortify: Add KUnit tests for runtime overflows | expand |
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 >
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).
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. */ +/** + * 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 (strlcat(p, q, p_size) >= p_size) + fortify_panic(__func__); + return p; +} + /** * strncat - Append a string to an existing string *
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(-)