Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CAHmME9oX+4Ek81xy0nBOegqABH0xYqyONAqinsu7GZ7AaQaqYQ@mail.gmail.com>
 <20220119100615.5059-1-miles.chen@mediatek.com> <CAMj1kXE_6WboUK0VPbTwzTbMNxv8b4XUp7USQUp=YqcCRMTZig@mail.gmail.com>
 <CAHmME9o-=q6jT6mF3XsfoA17756YVEWdXGHukP=rxWS1gs5irg@mail.gmail.com>
In-Reply-To: <CAHmME9o-=q6jT6mF3XsfoA17756YVEWdXGHukP=rxWS1gs5irg@mail.gmail.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Wed, 19 Jan 2022 11:35:32 +0100
Message-ID: <CAMj1kXFdgPj7Ky-361QNAOLccr1mq5m9LSsPfbgpwV58fLAxJw@mail.gmail.com>
Subject: Re: [PATCH] lib/crypto: blake2s: fix a CFI failure
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     Miles Chen <miles.chen@mediatek.com>,
        "David S. Miller" <davem@davemloft.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-mediatek@lists.infradead.org,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 19 Jan 2022 at 11:20, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On 1/19/22, Ard Biesheuvel <ardb@kernel.org> wrote:
> > On Wed, 19 Jan 2022 at 11:06, Miles Chen <miles.chen@mediatek.com> wrote:
> >>
> >> Hi,
> >>
> >> >Hi Miles,
> >> >
> >> >I'm actually not able to reproduce your oops. I'm using vanilla clang
> >> >13, cross compiling for arm64, with thin LTO enabled and CFI enabled.
> >> >Kernel seems to run fine.
> >> >
> >> >
> >> >Are there other settings that are needed to trigger this? Do you see
> >> >it in upstream clang or just the Android fork of clang?
> >> >
> >> I will try another clang (the previous version I use).
> >> I am using Android fork of clang and there is a clang upgrade in this
> >> merge.
> >>
> >
> > One thing that could be worth a try is to make __blake2s_update() and
> > __blake2s_final() __always_inline rather than just inline, which by
> > itself does not appear to be sufficient for the code to get inlined.
> > (If it were, the indirect call should have disappeared as well)
> >
> > Given that indirect calls suck on x86, we should probably apply that
> > change in any case, regardless of CFI.
> >
>
> Had the same thought at first, but then looking at the original stack
> trace, it looks like the __ function is inlined:
>
> [    0.000000][    T0]  __cfi_slowpath_diag+0x354/0x4b0
> [    0.000000][    T0]  blake2s_update+0x14c/0x178
> [    0.000000][    T0]  _extract_entropy+0xf4/0x29c
>

Indeed. How odd. I hope this doesn't happen with the x86 backend
because that would be plain silly. On arm64, it doesn't actually
matter in terms of performance, it just needs one additional callee
save register to preserve the function pointer across calls.