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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z/lo3iVcJgB2pfQX@redbud>
Date: Fri, 11 Apr 2025 14:09:18 -0500
From: Tyler Hicks <code@...icks.com>
To: Blaise Boscaccy <bboscaccy@...ux.microsoft.com>
Cc: Jonathan Corbet <corbet@....net>, David Howells <dhowells@...hat.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	"David S. Miller" <davem@...emloft.net>,
	Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	Masahiro Yamada <masahiroy@...nel.org>,
	Nathan Chancellor <nathan@...nel.org>,
	Nicolas Schier <nicolas@...sle.eu>, Shuah Khan <shuah@...nel.org>,
	Mickaël Salaün <mic@...ikod.net>,
	Günther Noack <gnoack@...gle.com>,
	Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
	Bill Wendling <morbo@...gle.com>,	Justin Stitt <justinstitt@...gle.com>,
	Jarkko Sakkinen <jarkko@...nel.org>,	Jan Stancek <jstancek@...hat.com>,
 Neal Gompa <neal@...pa.dev>,	linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org,	keyrings@...r.kernel.org,
 linux-crypto@...r.kernel.org,	linux-security-module@...r.kernel.org,
 linux-kbuild@...r.kernel.org,	linux-kselftest@...r.kernel.org,
 bpf@...r.kernel.org,	llvm@...ts.linux.dev, nkapron@...gle.com,
 teknoraver@...a.com,	roberto.sassu@...wei.com, xiyou.wangcong@...il.com
Subject: 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);
> +}