Re: [PATCH bpf-next v8 1/4] bpf: Add hash kfunc for cryptographic hashing

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3896 bytes --]

> diff --git a/crypto/bpf_crypto_shash.c b/crypto/bpf_crypto_shash.c
> --- /dev/null
> +++ b/crypto/bpf_crypto_shash.c
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/bpf_crypto.h>
> +#include <crypto/hash.h>
> +
> +struct bpf_shash_ctx {
> +	struct crypto_shash *tfm;
> +	struct shash_desc desc;
> +};

[ ... ]

> +static void *bpf_crypto_shash_alloc_tfm(const char *algo)
> +{

[ ... ]

> +	ctx->tfm = tfm;
> +	ctx->desc.tfm = tfm;
> +
> +	return ctx;
> +}

[ ... ]

> +static int bpf_crypto_shash_hash(void *tfm, const u8 *data, u8 *out,
> +				 unsigned int len)
> +{
> +	struct bpf_shash_ctx *ctx = tfm;
> +
> +	return crypto_shash_digest(&ctx->desc, data, len, out);
> +}

bpf_crypto_shash_alloc_tfm() creates one bpf_shash_ctx per
bpf_crypto_ctx, with a single embedded shash_desc. Multiple BPF
programs can acquire references to the same bpf_crypto_ctx
concurrently via bpf_crypto_ctx_acquire() (which just does
refcount_inc_not_zero), and the hash kfunc is marked KF_RCU which
provides no mutual exclusion.

When two CPUs call bpf_crypto_hash() with the same context, both
end up in crypto_shash_digest() using the same desc:

  CPU0                           CPU1
  bpf_crypto_hash(ctx)           bpf_crypto_hash(ctx)
    bpf_crypto_shash_hash()        bpf_crypto_shash_hash()
      crypto_shash_digest(desc)      crypto_shash_digest(desc)
        alg->digest(desc, ...)         alg->digest(desc, ...)
        // writes desc->__ctx          // writes desc->__ctx
        memset(desc->__ctx, 0)         memset(desc->__ctx, 0)

crypto_shash_op_and_zero() in crypto/shash.c writes to
shash_desc_ctx(desc) via the algorithm callback and then zeroes it
with memset. Both CPUs race on this buffer, producing silently
corrupted hash output.

The existing encrypt/decrypt path does not have this problem because
crypto_lskcipher_encrypt() treats the tfm as read-only (the key
schedule is pre-computed) and the mutable IV state is passed
per-call through the siv dynptr.

Would it make sense to allocate a shash_desc per-call on the stack,
or use a per-cpu descriptor, to avoid this race?

> diff --git a/kernel/bpf/crypto.c b/kernel/bpf/crypto.c
> --- a/kernel/bpf/crypto.c
> +++ b/kernel/bpf/crypto.c

[ ... ]

> +#if IS_ENABLED(CONFIG_CRYPTO_HASH2)
> +/**
> + * bpf_crypto_hash() - Compute hash using configured context
> + * @ctx:	The crypto context being used. The ctx must be a trusted pointer.
> + * @data:	bpf_dynptr to the input data to hash. Must be a trusted pointer.
> + * @out:	bpf_dynptr to the output buffer. Must be a trusted pointer.
> + *
> + * Computes hash of the input data using the crypto context. The output buffer
> + * must be at least as large as the digest size of the hash algorithm.
> + */
> +__bpf_kfunc int bpf_crypto_hash(struct bpf_crypto_ctx *ctx,
> +				const struct bpf_dynptr *data,
> +				const struct bpf_dynptr *out)
> +{

[ ... ]

> +	return ctx->type->hash(ctx->tfm, data_ptr, out_ptr, data_len);
> +}
> +#endif /* CONFIG_CRYPTO_HASH2 */


---
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/22414975518

AI-authorship-score: low
AI-authorship-explanation: Eight revision iterations with active reviewer engagement on lore.kernel.org and incremental changes addressing feedback are consistent with human-driven development.
issues-found: 1
issue-severity-score: medium
issue-severity-explanation: Data race on shared shash_desc in bpf_crypto_hash() produces silently corrupted cryptographic hash output when two CPUs use the same context concurrently, which could affect integrity checking and authentication use cases.