linux-kernel - RE: [PATCH] lib/crypto: blake2s: fix a CFI failure

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]

Date:   Wed, 19 Jan 2022 14:40:58 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Ard Biesheuvel' <ardb@...nel.org>,
        "Jason A. Donenfeld" <Jason@...c4.com>
CC:     Miles Chen <miles.chen@...iatek.com>,
        "David S. Miller" <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Nathan Chancellor <nathan@...nel.org>,
        "Nick Desaulniers" <ndesaulniers@...gle.com>
Subject: RE: [PATCH] lib/crypto: blake2s: fix a CFI failure

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.

Although, having looked at the source and the generated code for
x86-64 and arm64 I'm not sure I'd want to try to generate
optimised assembler for it.
(Unless you can a instruction that does exactly what the code wants.)

Basically the compiler can merge the instructions for 4 of the
G() expansions so that they can execute in parallel on a multi-issue
cpu. Doing that by hand will be error prone.
Each G() expansion is pretty much a register dependency chain,
not much chance of parallel execution.

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

The real problem is lack of registers - the code needs 16 for the
v[] array plus a few extras.
So some have to spill to stack.

The unrolled code is about 1200 instructions on arm64 and x86-64.
Each of the 10 rounds reads all 16 of the u32 input values.
So that is about 8 (1200/160) instructions for each read.
Which means there is plenty of memory bandwidth for other
reads.

So 'rolling up' the rounds - which adds in the blake2s_sigma[]
reads could easily be 'almost free'.
Certainly on x86 where you are just (well should be just) adding an
extra memory uop for each input buffer reads.

I'm not sure the 8 G() calls can be folded into two sets of 4
while still getting the compiler to interleave the generated code.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)