linux-kernel - Re: [PATCH] fortify: Improve buffer overflow reporting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKwvOdkfCQJcpx3zh0pwHfnBP2v2ieqxqTUzOUyW14s3Xsaf-w@mail.gmail.com>
Date:   Thu, 2 Mar 2023 15:21:11 -0800
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Andy Shevchenko <andy@...nel.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Nicolas Schier <nicolas@...sle.eu>, Tom Rix <trix@...hat.com>,
        Josh Poimboeuf <jpoimboe@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>,
        Marco Elver <elver@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Cezary Rojewski <cezary.rojewski@...el.com>,
        Mark Brown <broonie@...nel.org>, Puyou Lu <puyou.lu@...il.com>,
        linux-hardening@...r.kernel.org, linux-kbuild@...r.kernel.org,
        llvm@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fortify: Improve buffer overflow reporting

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