Re: [PATCH v5 06/10] security: Hornet LSM

[sashiko-bot@kernel.org]

Sashiko AI review found 10 potential issue(s):
- [High] Invalid Pointer Dereference due to missing IS_ERR check on lookup_user_key().
- [High] Resource Leak due to missing key_put() on user keyring reference.
- [High] NULL Pointer Dereference when verifying map types that do not implement map_get_hash.
- [High] Security Bypass due to uninitialized verdict variable on copy_from_bpfptr() failure.
- [High] Logic Flaw allowing verification of arbitrary file descriptors on copy_from_bpfptr_offset() failure.
- [High] State Corruption and Data Race in globally shared map_security object.
- [High] Logic Error bypassing runtime map verification due to incorrect loop index.
- [High] False Positives and Logic Error due to matching unverified maps to signature index 0.
- [Medium] Out-of-bounds Read in hornet_map_index() due to missing length check.
- [Medium] Off-by-One Error limits tracked maps to 63 instead of the intended 64.
--

commit 59054f5223757f6e86fcfcc7b0a3aa6b97918bab
Author: Blaise Boscaccy <bboscaccy@linux.microsoft.com>

security: Hornet LSM

This commit adds the Hornet Linux Security Module. It verifies PKCS#7
signatures attached to eBPF programs and checks SHA-256 hashes of BPF maps
at load and run time.

> diff --git a/security/hornet/hornet_lsm.c b/security/hornet/hornet_lsm.c
> new file mode 100644
> index 0000000000000..f7d62fe6229c9
> --- /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));
> +		if (err < 0)

Does this correctly handle copy_from_bpfptr_offset() failures?

The copy_from_bpfptr_offset() function returns the positive number of
uncopied bytes on failure, so err < 0 will not catch the error.

Since the destination buffer is zero-filled on failure, could this allow
inspecting file descriptor 0 instead of the intended map?

> +			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)
> +			return LSM_INT_VERDICT_FAULT;
> +
> +		map->ops->map_get_hash(map, SHA256_DIGEST_SIZE, hash);

Are we sure map_get_hash is implemented for all map types being checked?

Many standard BPF maps (such as hash maps) do not implement this callback.
Could calling it unconditionally trigger a NULL pointer dereference if an
unsupported map type is encountered?

> +
> +		err = memcmp(hash, &ctx->hashes[i * SHA256_DIGEST_SIZE],
> +			      SHA256_DIGEST_SIZE);
> +		if (err)
> +			return LSM_INT_VERDICT_UNEXPECTED;
> +
> +		security->checked[i] = true;
> +		memcpy(&security->hashes[i * SHA256_DIGEST_SIZE], hash, SHA256_DIGEST_SIZE);
> +		map_security = hornet_bpf_map_security(map);
> +		map_security->checked = true;
> +		map_security->index = i;

Is it safe to write the program-specific signature index into the globally
shared map_security object?

If multiple programs are concurrently loaded and attach the same map at
different signature indices, this could result in a data race. When the
first program runs, hornet_verify_map() expects the original index, which
might cause legitimate executions to fail.

> +	}
> +	return LSM_INT_VERDICT_OK;
> +}
> +
> +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)

Should this condition be > instead of >=?

Since MAX_USED_MAPS is 64, returning an error when the count reaches 64
artificially restricts the maximum number of tracked maps to 63.

> +		return -EINVAL;
> +	return 0;
> +}
> +
> +int hornet_map_index(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 (vlen > 1)
> +		return -EINVAL;
> +
> +	ctx->indexes[ctx->hash_count] = *(u8 *)value;

Is it possible for vlen to be 0 here?

If a crafted ASN.1 signature provides a zero-length integer, this might
result in an out-of-bounds read when accessing *(u8 *)value.

> +	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)
> +{
[ ... ]
> +	err = copy_from_bpfptr(sig, usig, attr->signature_size);
> +	if (err != 0)
> +		goto cleanup_sig;

Does this properly handle a copy_from_bpfptr() failure?

The copy_from_bpfptr() function returns the positive number of uncopied
bytes on failure, which is then propagated and returned by
hornet_check_program().

In hornet_bpf_prog_load_integrity(), the failure check is if (result < 0).
That check will pass on a positive return value, which could lead to
security_bpf_prog_load_post_integrity() being called with an uninitialized
verdict variable.

> +
> +	msg = pkcs7_parse_message(sig, attr->signature_size);
> +	if (IS_ERR(msg)) {
> +		*verdict = LSM_INT_VERDICT_BADSIG;
> +		err = 0;
> +		goto cleanup_sig;
> +	}
> +
> +	if (system_keyring_id_check(attr->keyring_id) == 0)
> +		key = (struct key*)(unsigned long)attr->keyring_id;
> +	else
> +		key = key_ref_to_ptr(lookup_user_key(attr->keyring_id, 0, KEY_DEFER_PERM_CHECK));
> +
> +	if (verify_pkcs7_message_sig(prog->insnsi, prog->len * sizeof(struct bpf_insn), msg,
> +				     key,

Can lookup_user_key() return an error pointer here?

If lookup_user_key() fails (for instance, due to an invalid keyring ID),
key_ref_to_ptr() masks the reference but does not check for IS_ERR(). This
could lead to an invalid pointer dereference when key is evaluated later.

Additionally, lookup_user_key() increments the reference count of the
returned key. Is there a missing key_put() to release this reference? It
appears the reference is never dropped on either the success or error
paths, which could cause a memory leak.

[ ... ]
> +static int hornet_verify_map(struct bpf_prog *prog, int index)
> +{
> +	unsigned char hash[SHA256_DIGEST_SIZE];
> +	int i;
> +	struct bpf_map *map;
> +	struct hornet_prog_security_struct *security = hornet_bpf_prog_security(prog);
> +	struct hornet_map_security_struct *map_security;
> +
> +	if (!security->checked[index])
> +		return 0;
> +
> +	for (i = 0; i < prog->aux->used_map_cnt; i++) {
> +		map = prog->aux->used_maps[i];
> +		map_security = hornet_bpf_map_security(map);
> +		if (map_security->index != index)

If a map has not been verified, its map_security structure is
zero-initialized, making map_security->index evaluate to 0.

When verifying signature index 0, could this wrongly match an unverified
map and attempt to compute its hash, leading to false positives?

> +			continue;
> +
> +		if (!map->frozen)
> +			return -EPERM;
> +
> +		map->ops->map_get_hash(map, SHA256_DIGEST_SIZE, hash);

Similar to hornet_verify_hashes(), does this safely handle map types that
do not implement map_get_hash?

Calling it unconditionally on unsupported maps might trigger a NULL pointer
dereference.

> +		if (memcmp(hash, &security->hashes[index * SHA256_DIGEST_SIZE],
> +			   SHA256_DIGEST_SIZE) != 0)
> +			return -EPERM;
> +		else
> +			return 0;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int hornet_check_prog_maps(u32 ufd)
> +{
[ ... ]
> +	for (i = 0; i < prog->aux->used_map_cnt; i++) {
> +		result = hornet_verify_map(prog, i);

Is i the correct value to pass as the signature index here?

The loop counter i iterates up to used_map_cnt. If a program uses maps
assigned to higher signature indices (e.g., indices 5 and 6) but only uses
2 maps overall, it appears this loop will only check indices 0 and 1,
skipping verification for the expected indices.

> +		if (result)
> +			goto out;
> +	}
> +out:
> +	mutex_unlock(&prog->aux->used_maps_mutex);
> +	return result;
> +}

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