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

[Eric Biggers <ebiggers@kernel.org>]

On Mon, Aug 31, 2026 at 08:29:02PM +0000, bot+bpf-ci@kernel.org wrote:
> The claim that lskcipher supports only these three specific algorithm
> names is not accurate. The cbc and ecb templates in crypto/cbc.c and
> crypto/ecb.c instantiate lskcipher interfaces for any plain cipher
> algorithm, not just AES:
> 
> crypto/cbc.c:crypto_cbc_create() calls lskcipher_alloc_instance_simple()
> crypto/ecb.c:crypto_ecb_create() calls lskcipher_alloc_instance_simple()
> 
> So cbc(<any-cipher>) and ecb(<any-cipher>) are valid lskcipher
> algorithms. The kernel includes about 14 plain cipher algorithms besides
> AES, including sm4, des, des3_ede, camellia, twofish, aria, blowfish,
> cast5, cast6, serpent, seed, khazad, anubis, tea, and xtea.
> 
> The pre-patch code passed params->algo directly to
> crypto_alloc_lskcipher() with no whitelist, so a BPF program could use
> cbc(sm4), cbc(des3_ede), ecb(camellia), or ecb(twofish).
> 
> After this patch, bpf_crypto_find_algo() only matches the two-entry
> table with cbc(aes) and ecb(aes), so all non-AES cbc/ecb modes now fail.
> 
> The changelog justifies dropping ARC4 but does not mention that this
> also drops support for all non-AES block ciphers. Should the commit
> message explicitly state that non-AES cbc()/ecb() support is being
> removed, or should the characterization of lskcipher's scope be
> corrected?

I'll update the commit message to clarify that other block ciphers are
dropped as well.

> Additionally, the error code for an unsupported algorithm changes from
> -EOPNOTSUPP (old has_algo failure path) to -ENOENT (new
> bpf_crypto_find_algo() path), which is a visible behavior change for
> existing BPF programs that inspect errno.

I'll change it to EOPNOTSUPP.

> > +config BPF_CRYPTO
> > +	def_bool y
> > +	depends on BPF_SYSCALL
> > +	select CRYPTO_LIB_AES_CBC
> > +	select CRYPTO_LIB_AES_ECB
> > +	help
> > +	  Provide the kfuncs needed for BPF programs to encrypt and decrypt
> > +	  data. The supported algorithms are AES-CBC and AES-ECB.
> > +
> 
> The def_bool y makes BPF_CRYPTO invisible and unconditionally enabled
> whenever CONFIG_BPF_SYSCALL=y, so the AES library code is always linked
> with no opt-out.
> 
> Before this patch, kernel/bpf/Makefile gated crypto.o on:
> 
>   ifneq ($(CONFIG_CRYPTO),)
>   obj-$(CONFIG_BPF_SYSCALL) += crypto.o
>   endif
> 
> so a CONFIG_BPF_SYSCALL=y, CONFIG_CRYPTO=n kernel previously built
> neither the BPF crypto kfuncs nor any AES code. Now the AES
> key-schedule, CBC, and ECB library code is unconditionally present.
> 
> This is particularly notable given that the commit message says AES-ECB
> "isn't appropriate in new systems and should be dropped" yet it is now
> unconditionally linked into every BPF_SYSCALL kernel.
> 
> Should this be a user-selectable option with a prompt like "bool 'BPF
> crypto kfuncs'" so it can be disabled, or is the intent really to make
> it always-on?

Almost every kernel has CRYPTO and CRYPTO_SKCIPHER2 enabled anyway, and
CRYPTO_SKCIPHER2 also enables ECB support.  So the change isn't actually
as drastic as the bot implies, but I'd be glad to give BPF_CRYPTO a
prompt if the BPF folks are okay with that.

> Deleting this file removes the last in-tree caller of
> crypto_alloc_lskcipher(). After this patch, the symbol only exists at
> its definition and export in crypto/lskcipher.c, with no remaining
> callers anywhere in the tree.
> 
> Given the premise that the lskcipher API "doesn't seem to be going
> anywhere," should crypto_alloc_lskcipher() be removed in a follow-up, or
> left as an exported API with zero users?

Yes, lskcipher should be deleted in a follow-up series.

> Does this code have a double-fetch issue? The function now uses the
> caller-supplied 'int *err' out-parameter as its own internal success
> flag: every failure path stores into *err, then the 'if (*err)' at the
> 'out:' label re-reads it to decide whether to free ctx and fail.
> 
> The 'err' parameter is classified by the verifier as KF_ARG_PTR_TO_MEM,
> so a BPF program can legally pass a pointer into a shared map value
> rather than its own stack. That makes the store followed by the reload a
> double fetch of concurrently-writable memory.
> 
> The pre-patch version never read *err - it used distinct error labels
> and returned NULL unconditionally on error, so the failure decision
> could not be influenced by the caller.
> 
> If the reload observes 0 because another CPU zeroed the map value
> between the store and the load, the function takes the success path for
> a create that actually failed. For instance, if aes_preparekey() returns
> -EINVAL from aes_check_keylen() before assigning enc_key->len and
> enc_key->nrounds, the kzalloc'ed key stays all-zero with nrounds == 0.
> 
> A later bpf_crypto_encrypt()/bpf_crypto_decrypt() on that ctx reaches
> aes_encrypt_generic()/aes_decrypt_generic(), which compute 'int n =
> nrounds - 1' (= -1) and then run 'do { ... } while (--n);', reading 16
> bytes past the round-key pointer per iteration for roughly 2^32
> iterations - an unbounded out-of-bounds read and unrecoverable soft
> lockup.
> 
> The reload can also mis-handle the -ENOENT path from
> bpf_crypto_find_algo(), returning a ctx whose algo is uninitialized
> (BPF_ALGO_AES_CBC == 0) with no key.
> 
> Could this be fixed by using a local 'int ret' for the internal flow and
> assigning '*err = ret' exactly once?

I'll change it to use a local variable.

> > @@ -86,23 +83,17 @@ struct bpf_crypto_ctx {
> >   * As crypto API functions use GFP_KERNEL allocations, this function can
> >   * only be used in sleepable BPF programs.
> 
> The kernel-doc for bpf_crypto_ctx_create() still justifies the
> KF_SLEEPABLE requirement with "As crypto API functions use GFP_KERNEL
> allocations."
> 
> This patch removes every crypto API call from the function
> (crypto_alloc_lskcipher()/crypto_lskcipher_setkey() are gone). The body
> now only does kzalloc_obj(), bpf_crypto_find_algo(), and
> aes_preparekey(), and the AES library is documented as usable in "Any
> context".
> 
> The conclusion still holds because kzalloc_obj() uses GFP_KERNEL, not
> because of the crypto API. Could the comment be updated to reflect the
> actual reason for the sleepable restriction after this refactoring?

I'll update the comment.

- Eric