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

[Blaise Boscaccy <bboscaccy@linux.microsoft.com>]

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