[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <51190E7A-25BD-4D9A-AADF-02FE2A280508@sifive.com>
Date: Tue, 28 Nov 2023 15:16:53 +0800
From: Jerry Shih <jerry.shih@...ive.com>
To: Eric Biggers <ebiggers@...nel.org>
Cc: Paul Walmsley <paul.walmsley@...ive.com>, palmer@...belt.com,
Albert Ou <aou@...s.berkeley.edu>, herbert@...dor.apana.org.au,
davem@...emloft.net, conor.dooley@...rochip.com, ardb@...nel.org,
heiko@...ech.de, phoebe.chen@...ive.com, hongrong.hsu@...ive.com,
linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-crypto@...r.kernel.org
Subject: Re: [PATCH v2 09/13] RISC-V: crypto: add Zvknha/b accelerated
SHA224/256 implementations
On Nov 28, 2023, at 12:12, Eric Biggers <ebiggers@...nel.org> wrote:
> On Mon, Nov 27, 2023 at 03:06:59PM +0800, Jerry Shih wrote:
>> +/*
>> + * sha256 using zvkb and zvknha/b vector crypto extension
>> + *
>> + * This asm function will just take the first 256-bit as the sha256 state from
>> + * the pointer to `struct sha256_state`.
>> + */
>> +asmlinkage void
>> +sha256_block_data_order_zvkb_zvknha_or_zvknhb(struct sha256_state *digest,
>> + const u8 *data, int num_blks);
>
> The SHA-2 and SM3 assembly functions are potentially being called using indirect
> calls, depending on whether the compiler optimizes out the indirect call that
> exists in the code or not. These assembly functions also are not defined using
> SYM_TYPED_FUNC_START. This is not compatible with Control Flow Integrity
> (CONFIG_CFI_CLANG); these indirect calls might generate CFI failures.
>
> I recommend using wrapper functions to avoid this issue, like what is done in
> arch/arm64/crypto/sha2-ce-glue.c.
>
> - Eric
Here is the previous review comment for the assembly function wrapper:
> > +asmlinkage void sha256_block_data_order_zvbb_zvknha(u32 *digest, const void *data,
> > + unsigned int num_blks);
> > +
> > +static void __sha256_block_data_order(struct sha256_state *sst, u8 const *src,
> > + int blocks)
> > +{
> > + sha256_block_data_order_zvbb_zvknha(sst->state, src, blocks);
> > +}
> Having a double-underscored function wrap around a non-underscored one like this
> isn't conventional for Linux kernel code. IIRC some of the other crypto code
> happens to do this, but it really is supposed to be the other way around.
>
> I think you should just declare the assembly function to take a 'struct
> sha256_state', with a comment mentioning that only the 'u32 state[8]' at the
> beginning is actually used. That's what arch/x86/crypto/sha256_ssse3_glue.c
> does, for example. Then, __sha256_block_data_order() would be unneeded.
Do you mean that we need the wrapper functions back for both SHA-* and SM3?
If yes, we also don't need to check the state offset like:
BUILD_BUG_ON(offsetof(struct sha256_state, state) != 0);
Could we just use the `SYM_TYPED_FUNC_START` in asm directly without the
wrappers?
-Jerry
Powered by blists - more mailing lists