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

sashiko-bot
2026-04-16T21:24:54Z

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