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

[sashiko-bot@kernel.org]

Sashiko AI review found 11 potential issue(s):
- [Critical] Improper error handling in userspace copies leads to security bypass and uninitialized variable use.
- [High] Unchecked `lookup_user_key` return value causes kernel panic.
- [High] Memory and resource leak of key references.
- [High] NULL pointer dereference when invoking `map_get_hash`.
- [High] State corruption via shared BPF map security blob causes denial of service.
- [High] Runtime map verification loop logic allows integrity check bypass.
- [High] File descriptor TOCTOU race condition in `hornet_bpf` hook.
- [Medium] Subsystem guideline violation: Mixing scope-based cleanup with goto.
- [Medium] Out-of-bounds read in `hornet_map_index()` when `vlen == 0`.
- [Medium] Off-by-one error in `hornet_next_map()` rejects valid signatures.
- [Low] Unchecked allocation size causes memory allocation warnings.
--

commit 6fbe5a3d726790559c6e3d3f0367735ef6beae27
Author: Blaise Boscaccy <bboscaccy@linux.microsoft.com>

security: Hornet LSM

This commit adds the Hornet Linux Security Module to provide enhanced
signature verification and data validation for eBPF programs. It verifies
PKCS#7 signatures on eBPF bytecode and associated map hashes at load time,
and re-verifies map hashes before execution.

> diff --git a/security/hornet/hornet_lsm.c b/security/hornet/hornet_lsm.c
> new file mode 100644
> --- /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)
> +{
> +	int map_fd;
> +	u32 i;
> +	struct bpf_map *map;
> +	int err = 0;
> +	unsigned char hash[SHA256_DIGEST_SIZE];
> +	struct hornet_prog_security_struct *security = hornet_bpf_prog_security(prog);
> +	struct hornet_map_security_struct *map_security;
> +
> +	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)
> +			return LSM_INT_VERDICT_FAULT;

Because copy_from_bpfptr_offset() returns the number of uncopied bytes
(a positive integer) upon failure, does this if statement fail to catch
a failed copy? If the copy fails, it seems map_fd could remain
uninitialized when passed to CLASS(fd, f)(map_fd) below, resulting in
an arbitrary file descriptor lookup.

> +
> +		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);

Does this code safely handle map types that do not implement the
map_get_hash callback? Many standard BPF map types (like
BPF_MAP_TYPE_PROG_ARRAY) do not implement this. Providing a signature
for such a map type might result in a NULL pointer dereference.

> +
> +		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 possible for state corruption to occur here since BPF maps can be
shared across multiple eBPF programs?

If a second program loads and verifies the same map, it appears it would
overwrite map_security->index with its own signature index. When the
first program is later executed, hornet_verify_map() would see a mismatch,
potentially causing a denial of service regression for the first program.

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

Could this unintentionally reject valid signatures that use exactly 64
maps? Because MAX_USED_MAPS is 64, incrementing the count for the 64th
map makes the condition true, returning -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;

Are we missing a check for vlen == 0 here? If a malformed signature
specifies an integer length of 0, it looks like this could read one
byte out of bounds from the value pointer.

> +	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)
> +{
> +	struct hornet_maps maps = {0};
> +	bpfptr_t usig = make_bpfptr(attr->signature, is_kernel);
> +	struct pkcs7_message *msg;
> +	struct hornet_parse_context *ctx;
> +	void *sig;
> +	int err;
> +	const void *authattrs;
> +	size_t authattrs_len;
> +	struct key *key;
> +
> +	if (!attr->signature) {
> +		*verdict = LSM_INT_VERDICT_UNSIGNED;
> +		return 0;
> +	}
> +
> +	ctx = kzalloc(sizeof(struct hornet_parse_context), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	maps.fd_array = make_bpfptr(attr->fd_array, is_kernel);
> +	sig = kzalloc(attr->signature_size, GFP_KERNEL);

Since attr->signature_size is a user-controlled value, could a very
large size intentionally trigger kernel memory allocation warnings
(OOM splats)? Should this use __GFP_NOWARN or enforce an upper limit?

> +	if (!sig) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +	err = copy_from_bpfptr(sig, usig, attr->signature_size);
> +	if (err != 0)
> +		goto cleanup_sig;

Similar to the map copy check earlier, copy_from_bpfptr() returns a positive
integer on failure. This causes hornet_check_program() to return a positive
value when the copy fails.

Looking at hornet_bpf_prog_load_integrity(), it checks if the result
is less than zero:

static int hornet_bpf_prog_load_integrity(...)
{
        enum lsm_integrity_verdict verdict;
        int result = hornet_check_program(prog, attr, token, is_kernel, &verdict);

        if (result < 0)
                return result;

        return security_bpf_prog_load_post_integrity(prog, attr, token, is_kernel,
                                                     &hornet_lsmid, verdict);
}

Could this result in proceeding to security_bpf_prog_load_post_integrity()
with an uninitialized verdict variable? If the stack memory happens to
hold LSM_INT_VERDICT_OK, it seems signature verification would be fully
bypassed.

> +
> +	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,

If lookup_user_key() fails (for example, due to an invalid ID or
permission denied), it returns an error-encoded key_ref_t.

Since IS_ERR() is not used to check the return value, could this pass an
invalid pointer through key_ref_to_ptr() and subsequently to
verify_pkcs7_message_sig(), potentially causing a panic?

Also, when lookup_user_key() successfully finds a key, it increments its
usage count. Is there a missing key_put(key) to drop this reference before
the function returns? Without it, repeated calls could leak struct key
objects.

[ ... ]
> +static int hornet_check_prog_maps(u32 ufd)
> +{
> +	CLASS(fd, f)(ufd);
> +	struct bpf_prog *prog;
> +	int i, result = 0;

This isn't a bug, but using CLASS(fd, f) alongside a goto-based error
handling pattern appears to violate the cleanup guidelines, which
suggest converting all resources to scope-based cleanup or none of them
to prevent inconsistent resource management.

> +
> +	if (fd_empty(f))
> +		return -EBADF;
> +	if (fd_file(f)->f_op != &bpf_prog_fops)
> +		return -EINVAL;
> +
> +	prog = fd_file(f)->private_data;
> +
> +	mutex_lock(&prog->aux->used_maps_mutex);
> +	if (!prog->aux->used_map_cnt)
> +		goto out;
> +
> +	for (i = 0; i < prog->aux->used_map_cnt; i++) {
> +		result = hornet_verify_map(prog, i);

Does using the loop counter i as the signature index for hornet_verify_map()
allow bypassing the integrity check for some maps?

The number of used maps doesn't seem to have a fixed relation to the
signature's map sequence length. If a map was verified at load time with a
signature index greater than or equal to used_map_cnt, it appears this loop
would never call hornet_verify_map() for that index, allowing the map
contents to be modified after loading.

> +		if (result)
> +			goto out;
> +	}
> +out:
> +	mutex_unlock(&prog->aux->used_maps_mutex);
> +	return result;
> +}
> +
> +static int hornet_bpf(int cmd, union bpf_attr *attr, unsigned int size, bool kernel)
> +{
> +	/* in horent_bpf(), anything that had originated from kernel space we assume
> +	   has already been checked, in some form or another, so we don't bother
> +	   checking the intergity of any maps. In hornet_bpf_prog_load_integrity(),
> +	   hornet doesn't make any opinion on that and delegates that to the downstream
> +	   policy enforcement. */
> +
> +	if (cmd != BPF_PROG_RUN)
> +		return 0;
> +	if (kernel)
> +		return 0;
> +
> +	return hornet_check_prog_maps(attr->test.prog_fd);

Is there a race condition here between the security check and the actual
execution in the syscall?

The hook resolves attr->test.prog_fd, performs checks, and then drops the
file descriptor reference before returning. The bpf() syscall will then
resolve the file descriptor again.

Could a concurrent thread use dup2() to overwrite attr->test.prog_fd
with an unsigned BPF program in the window between the check and the
execution?

> +}

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