Re: [PATCH v2 security-next 1/4] security: Hornet LSM

Tyler Hicks code@tyhicks.com writes:

On 2025-04-04 14:54:50, Blaise Boscaccy wrote:

...

+static int hornet_verify_lskel(struct bpf_prog *prog, struct hornet_maps *maps,

• 	       void *sig, size_t sig_len)
  

+{

• int fd;
• u32 i;
• void *buf;
• void *new;
• size_t buf_sz;
• struct bpf_map *map;
• int err = 0;
• int key = 0;
• union bpf_attr attr = {0};
• buf = kmalloc_array(prog->len, sizeof(struct bpf_insn), GFP_KERNEL);
• if (!buf)

• return -ENOMEM;
  

• buf_sz = prog->len * sizeof(struct bpf_insn);
• memcpy(buf, prog->insnsi, buf_sz);
• for (i = 0; i < maps->used_map_cnt; i++) {

• err = copy_from_bpfptr_offset(&fd, maps->fd_array,
  

• 			      maps->used_idx[i] * sizeof(fd),
  

• 			      sizeof(fd));
  

• if (err < 0)
  

• 	continue;
  

• if (fd < 1)
  

• 	continue;
  

• map = bpf_map_get(fd);
  

I'm not very familiar with BPF map lifetimes but I'd assume we need to have a corresponding bpf_map_put(map) before returning.

...

• if (IS_ERR(map))
  

• 	continue;
  

• /* don't allow userspace to change map data used for signature verification */
  

• if (!map->frozen) {
  

• 	attr.map_fd = fd;
  

• 	err = kern_sys_bpf(BPF_MAP_FREEZE, &attr, sizeof(attr));
  

• 	if (err < 0)
  

• 		goto out;
  

• }
  

• new = krealloc(buf, buf_sz + map->value_size, GFP_KERNEL);
  

• if (!new) {
  

• 	err = -ENOMEM;
  

• 	goto out;
  

• }
  

• buf = new;
  

• new = map->ops->map_lookup_elem(map, &key);
  

• if (!new) {
  

• 	err = -ENOENT;
  

• 	goto out;
  

• }
  

• memcpy(buf + buf_sz, new, map->value_size);
  

• buf_sz += map->value_size;
  

• }
• err = verify_pkcs7_signature(buf, buf_sz, sig, sig_len,

• 		     VERIFY_USE_SECONDARY_KEYRING,
  

• 		     VERIFYING_EBPF_SIGNATURE,
  

• 		     NULL, NULL);
  

+out:

• kfree(buf);
• return err;

+}

+static int hornet_check_binary(struct bpf_prog *prog, union bpf_attr *attr,

• 	       struct hornet_maps *maps)
  

+{

• struct file *file = get_task_exe_file(current);

We should handle get_task_exe_file() returning NULL. I don't think it is likely to happen when passing `current` but kernel_read_file() doesn't protect against it and we'll have a NULL pointer dereference when it calls file_inode(NULL).

...

• const unsigned long markerlen = sizeof(EBPF_SIG_STRING) - 1;
• void *buf = NULL;
• size_t sz = 0, sig_len, prog_len, buf_sz;
• int err = 0;
• struct module_signature sig;
• buf_sz = kernel_read_file(file, 0, &buf, INT_MAX, &sz, READING_EBPF);

We are leaking buf in this function. kernel_read_file() allocates the memory for us but we never kfree(buf).

...

• fput(file);
• if (!buf_sz)

• return -1;
  

• prog_len = buf_sz;
• if (prog_len > markerlen &&

•    memcmp(buf + prog_len - markerlen, EBPF_SIG_STRING, markerlen) == 0)
  

• prog_len -= markerlen;
  

Why is the marker optional? Looking at module_sig_check(), which verifies the signature on kernel modules, I see that it refuses to proceed if the marker is not found. Should we do the same and refuse to operate on any unexpected input?

Looking at this again, there doesn't seem to be a good reason to have an optional marker. I'll get that fixed in v3 along with the rest of these suggestions.

...

• memcpy(&sig, buf + (prog_len - sizeof(sig)), sizeof(sig));

We should make sure that prog_len is larger than sizeof(sig) prior to this memcpy(). It is probably not a real issue in practice but it would be good to ensure that we can't be tricked to copy and operate on any bytes proceeding buf.

Tyler

...

• sig_len = be32_to_cpu(sig.sig_len);
• prog_len -= sig_len + sizeof(sig);
• err = mod_check_sig(&sig, prog->len * sizeof(struct bpf_insn), "ebpf");
• if (err)

• return err;
  

• return hornet_verify_lskel(prog, maps, buf + prog_len, sig_len);

+}