Messages in this thread | ![/](/images/icornerl.gif) | | From | Nick Desaulniers <> | Date | Thu, 2 Mar 2023 15:21:11 -0800 | Subject | Re: [PATCH] fortify: Improve buffer overflow reporting |
| |
On Thu, Mar 2, 2023 at 2:58 PM Kees Cook <keescook@chromium.org> wrote: > > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h > index c9de1f59ee80..981e2838f99a 100644 > --- a/include/linux/fortify-string.h > +++ b/include/linux/fortify-string.h > @@ -170,11 +170,13 @@ __FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2) > char *strcat(char * const POS p, const char *q) > { > size_t p_size = __member_size(p); > + size_t size; > > if (p_size == SIZE_MAX) > return __underlying_strcat(p, q); > - if (strlcat(p, q, p_size) >= p_size) > - fortify_panic(__func__); > + size = strlcat(p, q, p_size); > + if (p_size < size)
What happens when they're equal? I think this patch changes behavior...? Intentional?
Did flipping this conditional drop what should be `<=`?
Was there an off by one, or is this version of this patch potentially introducing one? Or am I misremembering my boolean algebra?
> + fortify_panic(__func__, 1, p_size, size); > return p; > } > > @@ -205,7 +207,7 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size > /* Do not check characters beyond the end of p. */ > ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size); > if (p_size <= ret && maxlen != ret) > - fortify_panic(__func__); > + fortify_panic(__func__, 1, p_size, ret); > return ret; > } > > @@ -241,7 +243,7 @@ __kernel_size_t __fortify_strlen(const char * const POS p) > return __underlying_strlen(p); > ret = strnlen(p, p_size); > if (p_size <= ret) > - fortify_panic(__func__); > + fortify_panic(__func__, 1, p_size, ret); > return ret; > } > > @@ -282,8 +284,8 @@ __FORTIFY_INLINE size_t strlcpy(char * const POS p, const char * const POS q, si > __write_overflow(); > } > if (size) { > - if (len >= p_size) > - fortify_panic(__func__); > + if (p_size < len)
`<=` ? (This used to panic when they were equal)
> + fortify_panic(__func__, 1, p_size, len); > __underlying_memcpy(p, q, len); > p[len] = '\0'; > }
-- Thanks, ~Nick Desaulniers
| ![\](/images/icornerr.gif) |