linux-kernel - Re: [PATCH 04/12] RISC-V: crypto: add Zvkned accelerated AES implementation

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]

Message-ID: <20231102045154.GE1498@sol.localdomain>
Date:   Wed, 1 Nov 2023 21:51:54 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Jerry Shih <jerry.shih@...ive.com>
Cc:     paul.walmsley@...ive.com, palmer@...belt.com,
        aou@...s.berkeley.edu, herbert@...dor.apana.org.au,
        davem@...emloft.net, andy.chiu@...ive.com, greentime.hu@...ive.com,
        conor.dooley@...rochip.com, guoren@...nel.org, bjorn@...osinc.com,
        heiko@...ech.de, ardb@...nel.org, 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 04/12] RISC-V: crypto: add Zvkned accelerated AES
 implementation

On Thu, Oct 26, 2023 at 02:36:36AM +0800, Jerry Shih wrote:
> diff --git a/arch/riscv/crypto/Kconfig b/arch/riscv/crypto/Kconfig
> index 10d60edc0110..500938317e71 100644
> --- a/arch/riscv/crypto/Kconfig
> +++ b/arch/riscv/crypto/Kconfig
> @@ -2,4 +2,16 @@
>  
>  menu "Accelerated Cryptographic Algorithms for CPU (riscv)"
>  
> +config CRYPTO_AES_RISCV64
> +	default y if RISCV_ISA_V
> +	tristate "Ciphers: AES"
> +	depends on 64BIT && RISCV_ISA_V
> +	select CRYPTO_AES
> +	select CRYPTO_ALGAPI
> +	help
> +	  Block ciphers: AES cipher algorithms (FIPS-197)
> +
> +	  Architecture: riscv64 using:
> +	  - Zvkned vector crypto extension

kconfig options should default to off.

I.e., remove the line "default y if RISCV_ISA_V"

> + *
> + * All zvkned-based functions use encryption expending keys for both encryption
> + * and decryption.
> + */

The above comment is a bit confusing.  It's describing the 'key' field of struct
aes_key; maybe there should be a comment there instead:

    struct aes_key {
            u32 key[AES_MAX_KEYLENGTH_U32]; /* round keys in encryption order */
            u32 rounds;
    };

> +int riscv64_aes_setkey(struct riscv64_aes_ctx *ctx, const u8 *key,
> +		       unsigned int keylen)
> +{
> +	/*
> +	 * The RISC-V AES vector crypto key expending doesn't support AES-192.
> +	 * We just use the generic software key expending here to simplify the key
> +	 * expending flow.
> +	 */

expending => expanding

> +	u32 aes_rounds;
> +	u32 key_length;
> +	int ret;
> +
> +	ret = aes_expandkey(&ctx->fallback_ctx, key, keylen);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	/*
> +	 * Copy the key from `crypto_aes_ctx` to `aes_key` for zvkned-based AES
> +	 * implementations.
> +	 */
> +	aes_rounds = aes_round_num(keylen);
> +	ctx->key.rounds = aes_rounds;
> +	key_length = AES_BLOCK_SIZE * (aes_rounds + 1);
> +	memcpy(ctx->key.key, ctx->fallback_ctx.key_enc, key_length);
> +
> +	return 0;
> +}

Ideally this would use the same crypto_aes_ctx for both the fallback and the
assembly code.  I suppose we don't want to diverge from the OpenSSL code (unless
it gets rewritten), though.  So I guess this is fine for now.

> void riscv64_aes_encrypt_zvkned(const struct riscv64_aes_ctx *ctx, u8 *dst,
>                                const u8 *src)

These functions can be called from a different module (aes-block-riscv64), so
they need EXPORT_SYMBOL_GPL.

> +static inline bool check_aes_ext(void)
> +{
> +	return riscv_isa_extension_available(NULL, ZVKNED) &&
> +	       riscv_vector_vlen() >= 128;
> +}
> +
> +static int __init riscv64_aes_mod_init(void)
> +{
> +	if (check_aes_ext())
> +		return crypto_register_alg(&riscv64_aes_alg_zvkned);
> +
> +	return -ENODEV;
> +}
> +
> +static void __exit riscv64_aes_mod_fini(void)
> +{
> +	if (check_aes_ext())
> +		crypto_unregister_alg(&riscv64_aes_alg_zvkned);
> +}
> +
> +module_init(riscv64_aes_mod_init);
> +module_exit(riscv64_aes_mod_fini);

module_exit can only run if module_init succeeded.  So, in cases like this it's
not necessary to check for CPU features before unregistering the algorithm.

- Eric