Re: [PATCH] fortify: Improve buffer overflow reporting

[Nick Desaulniers]

On Thu, Mar 2, 2023 at 2:58 PM Kees Cook <keescook@xxxxxxxxxxxx> 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