Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220119082447.1675-1-miles.chen@mediatek.com>
 <CAHmME9pv4WWATjdqZgwrtHDmq3sX4ABfB9PoNT9Z4tSEduR2Lw@mail.gmail.com> <CAMj1kXEuWRUbCqDBnxiWRaERt6OGL8ufQ1Q7naAGHqKK1oQB1w@mail.gmail.com>
In-Reply-To: <CAMj1kXEuWRUbCqDBnxiWRaERt6OGL8ufQ1Q7naAGHqKK1oQB1w@mail.gmail.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Wed, 19 Jan 2022 10:13:51 +0100
Message-ID: <CAMj1kXGUtg1BOauCjxB+S7yMTPArQrPnHf5jnfhOihL-FHrFZA@mail.gmail.com>
Subject: Re: [PATCH] lib/crypto: blake2s: fix a CFI failure
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     =?UTF-8?B?TWlsZXMgQ2hlbiAo6Zmz5rCR5qi6KQ==?= 
        <miles.chen@mediatek.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        linux-mediatek@lists.infradead.org,
        Eric Biggers <ebiggers@google.com>,
        Sami Tolvanen <samitolvanen@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 19 Jan 2022 at 10:09, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> (+ Sami, Eric)
>
> On Wed, 19 Jan 2022 at 10:00, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > Hi Miles,
> >
> > Thanks for the patch. Could you let me know which architecture and
> > compiler this was broken on? If I had to guess, I'd wager arm32, and
> > you hit this by enabling optimized blake2s?
> >
> > If so, I'm not sure the problem is with weak symbols. Why should CFI
> > break weak symbols? Rather, perhaps the issue is that the function is
> > defined in blake2s-core.S? Are there some CFI macros we need for that
> > definition?
> >
>
> We should try to understand why CFI thinks the prototypes of the two
> symbols are different. There are still a number of issues with CFI, so
> papering over them by reverting stuff that we want for good reasons is
> not the way to go imo.
>
> In the short term, you can work around it by avoiding the indirect
> call to blake2s_compress, e.g.,
>
> diff --git a/lib/crypto/blake2s.c b/lib/crypto/blake2s.c
> index 93f2ae051370..fef2ff678431 100644
> --- a/lib/crypto/blake2s.c
> +++ b/lib/crypto/blake2s.c
> @@ -16,9 +16,15 @@
>  #include <linux/init.h>
>  #include <linux/bug.h>
>
> +static void __blake2s_compress(struct blake2s_state *state, const u8 *block,
> +                              size_t nblocks, const u32 inc)
> +{
> +       return blake2s_compress(state, block, nblocks, inc);
> +}
> +
>  void blake2s_update(struct blake2s_state *state, const u8 *in, size_t inlen)
>  {
> -       __blake2s_update(state, in, inlen, blake2s_compress);
> +       __blake2s_update(state, in, inlen, __blake2s_compress);
>  }
>  EXPORT_SYMBOL(blake2s_update);

Ehm, maybe not. As Jason points out, the typedef does not have quite
the right type, so that is most likely the culprit, and this
workaround would trigger CFI in exactly the same way.

Interestingly, the compiler does not seem to mind, right? Or are you
seeing any build time warnings on the reference to blake2s_compress in
the call to __blake2s_update() ?