Re: [PATCH] libbpf: bounds-check struct_ops member offset before writing shadow pointer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] The new multi-line comment introduced in the patch violates the BPF subsystem comment style guidelines.

Pre-existing issues:
- [High] Integer overflows in bounds checks during BTF and ELF parsing allow heap out-of-bounds reads.
- [High] Incomplete Fix: Missing bounds check in `bpf_map__init_kern_struct_ops` allows heap out-of-bounds read leaking memory to the kernel.
--

--- Patch [1]: [PATCH] libbpf: bounds-check struct_ops member offset before writing shadow pointer ---
commit edbb05326b1be4436b6fa2f85e4c2b2097d884b7
Author: Naveed Khan <naveed@digiscrypt.com>

libbpf: bounds-check struct_ops member offset before writing shadow pointer

This commit adds a bounds check in bpf_object__collect_st_ops_relos() to
ensure that writing the shadow pointer for a struct_ops member doesn't
overflow the map's value size. It prevents a controlled heap out-of-bounds
write that can occur when parsing malformed BTF data.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 1368752aa13c3..1aa2a1d098927 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -10529,6 +10529,17 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
>  			return -EINVAL;
>  		}
>  
> +		/* the shadow pointer stored below is sizeof(struct bpf_program *)
> +		 * bytes wide, so the whole write must fit within st_ops->data,
> +		 * which is only map->def.value_size bytes long. A malformed BTF
> +		 * can place a member near the end of the value and overflow it.
> +		 */

[Severity: Low]
Is this multi-line comment using the correct BPF subsystem style?

The guidelines indicate that multi-line comments must have the opening /* on
its own line, with the comment text starting on the next line.

> +		if (moff + sizeof(struct bpf_program *) > map->def.value_size) {
> +			pr_warn("struct_ops reloc %s: member %s at moff %u overflows map value size %u\n",
> +				map->name, name, moff, map->def.value_size);
> +			return -EINVAL;
> +		}
> +
>  		prog = find_prog_by_sec_insn(obj, shdr_idx, insn_idx);

[Severity: High]
This is a pre-existing issue, but since this patch fixes a missing bounds
check for struct_ops offsets, could a similar missing bounds check exist in
bpf_map__init_kern_struct_ops()?

tools/lib/bpf/libbpf.c:bpf_map__init_kern_struct_ops() {
    ...
		moff = member->offset / 8;
		mdata = data + moff;
		msize = btf__resolve_size(btf, member->type);
    ...
		memcpy(kern_mdata, mdata, msize);
    ...
}

If a malformed BTF provides a struct_ops member with a large offset, could
this result in an out-of-bounds heap read from mdata that is then copied
into the kernel-bound buffer kern_mdata?


[Severity: High]
This is also a pre-existing issue, but can the 32-bit arithmetic used in
the bounds checks for BTF and ELF parsing overflow?

tools/lib/bpf/libbpf.c:init_struct_ops_maps() {
    ...
		if (vsi->offset + type->size > data->d_size) {
			pr_warn("struct_ops init: var %s is beyond the end of DATASEC %s\n",
				var_name, sec_name);
			return -EINVAL;
		}
    ...
}

Since vsi->offset and type->size are 32-bit integers, could a maliciously
crafted large vsi->offset (e.g., 0xFFFFFFF0) cause the addition to wrap
around and bypass the check against the 64-bit data->d_size, leading to an
out-of-bounds read during the subsequent operations?

A similar 32-bit overflow appears to exist in create_jt_map() when parsing
the .jumptables section:

tools/lib/bpf/libbpf.c:create_jt_map() {
    ...
	if (sym_off + jt_size > obj->jumptables_data_sz) {
		pr_warn("map '.jumptables': jumptables_data size is %zd, trying to access %d\n",
			obj->jumptables_data_sz, sym_off + jt_size);
		err = -EINVAL;
		goto err_close;
	}
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178478913040.2.14225985431891987705@digiscrypt.com?part=1