Re: [v6 06/10] security: Hornet LSM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] A Time-of-Check to Time-of-Use (TOCTOU) race condition in the `bpf` LSM hook allows attackers to bypass runtime map verification.
- [Critical] TOCTOU race condition on `attr->fd_array` allows loading unsigned maps.
- [High] Unbounded memory allocation for the signature buffer based on user input allows local Denial of Service.
- [High] Synchronous map hashing over potentially massive BPF maps causes severe CPU stalls.
- [High] Lockless read of `map->frozen` lacks memory barriers, allowing race condition bypass.
- [High] Hardcoded fail-open logic for unsigned programs overrides downstream LSM policy.
- [Medium] Off-by-one error in ASN.1 decoder rejects valid signatures with exactly 64 maps.
- [Medium] Legitimately skipped frozen maps are unconditionally rejected at runtime.
- [Medium] Deduplication of `used_maps` causes state mismatch, rejecting valid programs.
--

commit 88fd67d9d38369a17372ee4205b99a1b455d1487
Author: Blaise Boscaccy <bboscaccy@linux.microsoft.com>

security: Hornet LSM

This adds the Hornet Linux Security Module which provides enhanced
signature verification and data validation for eBPF programs. It
verifies PKCS#7 signatures and map hashes to solidify auditing and
prevent tampering.

> diff --git a/security/hornet/hornet_lsm.c b/security/hornet/hornet_lsm.c
> new file mode 100644
> index 000000000000..4656457657cc
> --- /dev/null
> +++ b/security/hornet/hornet_lsm.c
[ ... ]
> +static int hornet_verify_hashes(struct hornet_maps *maps,
> +				struct hornet_parse_context *ctx,
> +				struct bpf_prog *prog)
> +{
[ ... ]
> +	for (i = 0; i < ctx->hash_count; i++) {
> +		if (ctx->skips[i])
> +			continue;
> +
> +		err = copy_from_bpfptr_offset(&map_fd, maps->fd_array,
> +					      ctx->indexes[i] * sizeof(map_fd),
> +					      sizeof(map_fd));

Can this lead to a Time-of-Check to Time-of-Use race condition?
The map file descriptors are read directly from the user-provided fd_array.
After Hornet successfully verifies these maps against the signature, the BPF
verifier reads from the exact same user-space memory to bind the maps to the
program. It appears an attacker could swap the map file descriptors in user
memory with malicious, unsigned maps between the LSM check and the verifier
binding.

> +		if (err != 0)
> +			return LSM_INT_VERDICT_FAULT;
> +
> +		CLASS(fd, f)(map_fd);
> +		if (fd_empty(f))
> +			return LSM_INT_VERDICT_FAULT;
> +		if (unlikely(fd_file(f)->f_op != &bpf_map_fops))
> +			return LSM_INT_VERDICT_FAULT;
> +
> +		map = fd_file(f)->private_data;
> +		if (!map->frozen)

Could this check bypass the integrity verification due to missing memory
barriers? map->frozen is read locklessly here without smp_load_acquire().
While bpf_map_freeze uses a mutex, memory reordering could allow Hornet to
read benign map data while seeing map->frozen as true from a concurrent
freeze. An attacker could potentially update the map with malicious data
right before freezing it.

> +			return LSM_INT_VERDICT_FAULT;
[ ... ]
> +int hornet_next_map(void *context, size_t hdrlen,
> +		     unsigned char tag,
> +		     const void *value, size_t vlen)
> +{
> +	struct hornet_parse_context *ctx = (struct hornet_parse_context *)context;
> +
> +	if (++ctx->hash_count >= MAX_USED_MAPS)

Does this reject valid signatures with exactly 64 maps? Since hash_count is
incremented before the comparison, processing the 64th map will trigger the
condition and cause the function to return -EINVAL.

> +		return -EINVAL;
> +	return 0;
> +}
[ ... ]
> +static int hornet_check_program(struct bpf_prog *prog, union bpf_attr *attr,
> +				struct bpf_token *token, bool is_kernel,
> +				enum lsm_integrity_verdict *verdict)
> +{
[ ... ]
> +	maps.fd_array = make_bpfptr(attr->fd_array, is_kernel);
> +	sig = kzalloc(attr->signature_size, GFP_KERNEL);

Is there a risk of memory pressure or Denial of Service here?
The allocation size relies directly on the user-controlled
attr->signature_size without any upper bound check. An excessively large
size could trigger a loud warning from the page allocator and spam the
kernel log.

> +	if (!sig) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
[ ... ]
> +static int hornet_check_prog_maps(u32 ufd)
> +{
> +	CLASS(fd, f)(ufd);
> +	struct bpf_prog *prog;
> +	struct hornet_prog_security_struct *security;
> +	unsigned char hash[SHA256_DIGEST_SIZE];
> +	struct bpf_map *map;
> +	int i, j;
> +	bool found;
> +	int covered_count = 0;
> +
> +	if (fd_empty(f))
> +		return -EBADF;
> +	if (fd_file(f)->f_op != &bpf_prog_fops)
> +		return -EINVAL;
> +
> +	prog = fd_file(f)->private_data;
> +	security = hornet_bpf_prog_security(prog);
> +
> +	if (!security->signed_hash_count)

Does this hardcoded fail-open logic override downstream LSM policy?
If a downstream LSM allows unsigned programs, Hornet returns 0 here and
bypasses map checks. However, if it allows partially signed programs, Hornet
still enforces that all frozen maps match, breaking LSM composition.

> +		return 0;
> +
> +	mutex_lock(&prog->aux->used_maps_mutex);
> +
> +	/* Verify every used_map has a matching signed hash */
> +	for (j = 0; j < prog->aux->used_map_cnt; j++) {
> +		map = prog->aux->used_maps[j];
> +
> +		if (!map->frozen || !map->ops->map_get_hash)
> +			continue;
> +
> +		if (map->ops->map_get_hash(map, SHA256_DIGEST_SIZE, hash))

Could this cause CPU stalls or local Denial of Service?
BPF maps can be extremely large. Computing the hash over the entire map
memory synchronously without yielding the CPU could cause multi-second RCU
stalls.

> +			continue;
> +
> +		found = false;
> +		for (i = 0; i < security->signed_hash_count; i++) {
> +			if (memcmp(hash,
> +				   &security->signed_hashes[i * SHA256_DIGEST_SIZE],
> +				   SHA256_DIGEST_SIZE) == 0) {
> +				found = true;
> +				break;
> +			}
> +		}
> +		if (!found) {

Does this incorrectly reject legitimately skipped maps?
If a signature specifies a zero-length hash for a map, hornet_map_hash sets
skips to true and excludes it from signed_hashes. However, if the map is
frozen, this loop will fail to find it in signed_hashes and return -EPERM.

> +			mutex_unlock(&prog->aux->used_maps_mutex);
> +			return -EPERM;
> +		}
> +		covered_count++;
> +	}
> +
> +	mutex_unlock(&prog->aux->used_maps_mutex);
> +
> +	/* Ensure all signed hashes were accounted for */
> +	if (covered_count != security->signed_hash_count)

Can this fail due to map deduplication?
If a program's signature contains multiple index entries for the same map
file descriptor, signed_hash_count is incremented for each entry. The BPF
core deduplicates maps in prog->aux->used_maps, so covered_count is only
incremented once per unique map, causing this check to reject valid programs.

> +		return -EPERM;
> +
> +	return 0;
> +}
> +
> +static int hornet_bpf(int cmd, union bpf_attr *attr, unsigned int size, bool kernel)
> +{
[ ... ]
> +	if (cmd != BPF_PROG_RUN)
> +		return 0;
> +	if (kernel)
> +		return 0;
> +
> +	return hornet_check_prog_maps(attr->test.prog_fd);

Is there a Time-of-Check to Time-of-Use race condition here?
attr->test.prog_fd is resolved and checked inside hornet_check_prog_maps, but
the BPF core will resolve the same file descriptor again via bpf_prog_get()
before executing it. Because the file descriptor table is shared across
threads, an attacker could use dup2() to swap the file descriptor to point
to a malicious BPF program after Hornet's check but before execution.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429191431.2345448-1-bboscaccy@linux.microsoft.com?part=6