Re: [PATCH bpf-next v4] bpf: crypto: Use AES-CBC and AES-ECB libraries

[bot+bpf-ci]

> diff --git a/kernel/bpf/crypto.c b/kernel/bpf/crypto.c
> index 51f89cecefb4..784e2bd5ca92 100644
> --- a/kernel/bpf/crypto.c
> +++ b/kernel/bpf/crypto.c

[ ... ]

> @@ -36,94 +30,53 @@ struct bpf_crypto_params {
>  	u32 authsize;
>  };
>
> +enum bpf_crypto_algo_id {
> +	BPF_ALGO_AES_CBC,
> +	BPF_ALGO_AES_ECB,
> +};
> +
> +/**
> + * struct bpf_crypto_ctx - refcounted BPF crypto context structure
> + * @algo:	The crypto algorithm ID
> + * @key:	The crypto key
> + * @rcu:	The RCU head used to free the crypto context with RCU safety.
> + * @usage:	Object reference counter. When the refcount goes to 0, the
> + *		memory is released back to the BPF allocator, which provides
> + *		RCU safety.
> + */
> +struct bpf_crypto_ctx {
> +	enum bpf_crypto_algo_id algo;
> +	union {
> +		struct aes_key aes;
> +	} key;
> +	struct rcu_head rcu;
> +	refcount_t usage;
> +};

This isn't a bug, but struct aes_enckey pads itself so the round keys
can be 16-byte aligned for better performance (as documented in
include/crypto/aes.h). Putting the 4-byte algo enum before the key union
means key.aes lands at offset 4, so the round keys can never hit that
alignment. Would putting key first (or aligning it) be worth considering
here?

[ ... ]

> +static int bpf_aes_cbc_crypt(u8 *dst, u32 dst_len, const u8 *src, u32 src_len,
> +			     u8 *iv, u32 iv_len,
> +			     const struct bpf_crypto_ctx *ctx, bool decrypt)
> +{
> +	if (iv_len != AES_BLOCK_SIZE)
> +		return -EINVAL;
> +	if (src_len % AES_BLOCK_SIZE || dst_len < src_len)
> +		return -EINVAL;
> +	if (decrypt)
> +		aes_cbc_decrypt(dst, src, src_len, iv, &ctx->key.aes);
> +	else
> +		aes_cbc_encrypt(dst, src, src_len, iv, &ctx->key.aes);
> +	return 0;
> +}
> +
> +static int bpf_aes_ecb_crypt(u8 *dst, u32 dst_len, const u8 *src, u32 src_len,
> +			     u8 *iv, u32 iv_len,
> +			     const struct bpf_crypto_ctx *ctx, bool decrypt)
> +{
> +	if (iv_len != 0)
> +		return -EINVAL;
> +	if (src_len % AES_BLOCK_SIZE || dst_len < src_len)
> +		return -EINVAL;
> +	if (decrypt)
> +		aes_ecb_decrypt(dst, src, src_len, &ctx->key.aes);
> +	else
> +		aes_ecb_encrypt(dst, src, src_len, &ctx->key.aes);
> +	return 0;
> +}

This isn't a bug, but both helpers repeat the same src_len %
AES_BLOCK_SIZE and dst_len < src_len checks. Could that validation stay
in bpf_crypto_crypt() next to the existing length checks, leaving only
the iv_len check per mode?

Also, bpf_aes_ecb_crypt() takes an iv parameter but never uses it (only
iv_len is checked). Would it read more clearly to drop the iv argument
from this function, or drop both iv and iv_len and do the iv_len != 0
check in the ECB arm of bpf_crypto_crypt()?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35815287581