Re: [PATCH] lib/crypto: blake2s: fix a CFI failure

[Jason A. Donenfeld]

Hi David,

On Wed, Jan 19, 2022 at 3:41 PM David Laight <David.Laight@xxxxxxxxxx> wrote:
>
> From: Ard Biesheuvel
> > Sent: 19 January 2022 12:19
> ...
> > -               (*compress)(state, in, nblocks - 1, BLAKE2S_BLOCK_SIZE);
> > +               if (IS_ENABLED(CONFIG_CRYPTO_ARCH_HAVE_LIB_BLAKE2S))
> > +                       (*compress)(state, in, nblocks - 1, BLAKE2S_BLOCK_SIZE);
> > +               else
> > +                       blake2s_compress_generic(state, in, nblocks - 1,
> > +                                                BLAKE2S_BLOCK_SIZE);
>
> Isn't that a candidate for a 'static call' ?
>
> And, maybe all these inlined functions should be real functions?
> No point having all the bloat on every call site.
> Much better to call a real function and used the cached instructions.

Not a good candidate for static call, as this doesn't actually need to
change at runtime ever. It's using a function pointer here out of
laziness to keep the same body of the function, like a compile-time
template. You can sort of squint and imagine the C++. Unfortunately,
CFI felt differently and still treats it as an indirect call.

https://lore.kernel.org/linux-crypto/20220119135450.564115-1-Jason@xxxxxxxxx/
fixes it up to use a boolean instead, which will certainly be inlined
away. So that's definitely an improvement on what's there now.

For 5.18, I think it's probable that all of this stuff goes away
anyway, and we don't need the templated helpers at all. So perhaps my
patch will serve as an okay stop gap. Alternatively, maybe the clang
people will say, "oh no, our bug" and then fix it in their
neighborhood. According to
https://github.com/ClangBuiltLinux/linux/issues/1567 it looks like
that could be the case.

> There are clearly optimisations for the top/bottom of the loop.
> But they can be done to the generic C version.

Optimizing the generic C version would be quite nice, as it'd help all
platforms.

Jason