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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) vulnerability during BPF program load allows bypassing map signature verification.
- [High] Unbounded memory allocation with user-provided `attr->signature_size` leads to denial of service.
- [High] Arbitrary user keyring injection allows unprivileged users to bypass system-wide signature verification.
- [High] Map verification logic allows map substitution by improperly tracking matched hashes.
- [High] Programs signed with an empty map hash SET bypass map verification entirely while receiving an OK verdict.
- [High] Runtime map verification only hooks `bpf(BPF_PROG_RUN)`, leaving normal event-driven eBPF execution unprotected against runtime map tampering.
- [Medium] Skipping a map in the signature causes valid test runs to be denied with `-EPERM` if the map is frozen.
- [Medium] Off-by-one bounds check in `hornet_next_map` incorrectly rejects valid signatures containing exactly 64 maps.
--

commit 33400a9d1c16d89f1fcfeabbd6450005c549df6e
Author: Blaise Boscaccy <bboscaccy@linux.microsoft.com>

security: Hornet LSM

This commit adds the Hornet Linux Security Module to provide eBPF signature
verification and map validation. It uses PKCS#7 signed attributes to verify
map hashes at load time and 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);
> +
> +	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));

Is this susceptible to a Time-of-Check to Time-of-Use race?

Since maps->fd_array is read from userspace here, and then read again later
by the core BPF verifier, could a malicious application swap the map file
descriptor to point to an unsigned map after this verification succeeds?

[ ... ]
> +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;

Does this restrict the maximum number of maps to 63 instead of the intended 64?

If a valid signature contains exactly 64 maps, processing the 64th map will
increment hash_count to 64, causing this check to evaluate to true and reject
the signature.

[ ... ]
> +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);

Can a user provide an extremely large signature_size to trigger an unbounded
memory allocation?

Without a bounds check on attr->signature_size, an attacker could request a
size large enough to cause the slab allocator to fail, potentially triggering
warnings or a system panic if panic_on_warn is enabled.

> +	if (!sig) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
[ ... ]
> +	if (system_keyring_id_check(attr->keyring_id) == 0)
> +		key = (struct key *)(unsigned long)attr->keyring_id;
> +	else {
> +		user_key = lookup_user_key(attr->keyring_id, 0, KEY_DEFER_PERM_CHECK);

Does this allow an unprivileged user to specify their own user keyring for
signature verification?

If an attacker signs a malicious program with their own private key and loads
it with their own user keyring ID, will Hornet successfully verify it and
return an OK verdict to downstream LSMs?

[ ... ]
> +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)
> +		return 0;

If a program is signed without including any map hashes, will this return
success but allow an attacker to attach arbitrary unverified maps?

Since an empty map hash SET would produce an OK verdict at load time,
returning early here bypasses runtime verification completely.

> +
> +	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 (!READ_ONCE(map->frozen) || !map->ops->map_get_hash)
> +			continue;
> +
> +		if (map->ops->map_get_hash(map, SHA256_DIGEST_SIZE, hash))
> +			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;
> +			}
> +		}

Does this allow map substitution if multiple maps share the same hash?

Because the matched hash isn't marked as consumed, an attacker could supply
multiple copies of the same map to satisfy the covered_count check, entirely
omitting another required map.

> +		if (!found) {
> +			mutex_unlock(&prog->aux->used_maps_mutex);
> +			return -EPERM;

If a valid signature explicitly skips a map using a zero-length SHA, but the
map happens to be frozen, will this incorrectly reject the execution?

Skipped maps are not added to signed_hashes at load time, so this loop will
fail to find the hash and return -EPERM, breaking execution for valid programs.

[ ... ]
> +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);
> +}

Does hooking only the BPF_PROG_RUN command leave normal event-driven eBPF
executions unprotected?

Programs like XDP, TC, or kprobes execute asynchronously in response to kernel
events and bypass the bpf() syscall entirely. Will this allow runtime map
tampering for these workloads?

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