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

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?

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

+}