On Nov 28, 2023, at 12:12, Eric Biggers <ebiggers@xxxxxxxxxx> 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