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>
In-Reply-To: <CAHmME9pv4WWATjdqZgwrtHDmq3sX4ABfB9PoNT9Z4tSEduR2Lw@mail.gmail.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Wed, 19 Jan 2022 10:09:59 +0100
Message-ID: <CAMj1kXEuWRUbCqDBnxiWRaERt6OGL8ufQ1Q7naAGHqKK1oQB1w@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

(+ 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);