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

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

[Naveed Khan]

bpf_object__collect_st_ops_relos() replaces each function pointer in a
struct_ops value with a pointer to the corresponding struct bpf_program
in the map's shadow data:

	*((struct bpf_program **)(st_ops->data + moff)) = prog;

st_ops->data is allocated with malloc(type->size), so it is exactly
map->def.value_size bytes long, and moff is the byte offset of the
member being relocated. The only bound placed on moff comes from
find_struct_ops_map_by_offset():

	offset - map->sec_offset < map->def.value_size

so moff can be as large as value_size - 1. The store above is
sizeof(struct bpf_program *) bytes wide, and nothing checks that
moff + sizeof(struct bpf_program *) stays within value_size.

libbpf's BTF sanity check does not validate that a struct member's
offset plus its size fits within the struct's declared size (see
btf_validate_type()), and this relocation runs at bpf_object__open()
time, before the kernel validates the struct_ops type. A crafted object
with a struct_ops struct whose declared size is small (e.g. 1) but which
contains a function-pointer member near the end of (or beyond) that size
therefore makes libbpf write up to sizeof(void *) - 1 bytes past the heap
allocation - a controlled heap out-of-bounds write reachable purely from
opening an untrusted object file.

Reject such relocations by requiring the whole pointer store to fit
within the map value.

Signed-off-by: Naveed Khan <naveed@digiscrypt.com>
---
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 1368752aa1..1aa2a1d098 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.
+		 */
+		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);
 		if (!prog) {
 			pr_warn("struct_ops reloc %s: cannot find prog at shdr_idx %u to relocate func ptr %s\n",
-- 
2.52.0

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

[sashiko-bot]

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

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 2194 bytes --]

> bpf_object__collect_st_ops_relos() replaces each function pointer in a
> struct_ops value with a pointer to the corresponding struct bpf_program
> in the map's shadow data:
>
> 	*((struct bpf_program **)(st_ops->data + moff)) = prog;
>
> st_ops->data is allocated with malloc(type->size), so it is exactly
> map->def.value_size bytes long, and moff is the byte offset of the
> member being relocated. The only bound placed on moff comes from
> find_struct_ops_map_by_offset():
>
> 	offset - map->sec_offset < map->def.value_size
>
> so moff can be as large as value_size - 1. The store above is
> sizeof(struct bpf_program *) bytes wide, and nothing checks that
> moff + sizeof(struct bpf_program *) stays within value_size.
>
> libbpf's BTF sanity check does not validate that a struct member's
> offset plus its size fits within the struct's declared size (see
> btf_validate_type()), and this relocation runs at bpf_object__open()
> time, before the kernel validates the struct_ops type. A crafted object
> with a struct_ops struct whose declared size is small (e.g. 1) but which
> contains a function-pointer member near the end of (or beyond) that size
> therefore makes libbpf write up to sizeof(void *) - 1 bytes past the heap
> allocation - a controlled heap out-of-bounds write reachable purely from
> opening an untrusted object file.
>
> Reject such relocations by requiring the whole pointer store to fit
> within the map value.
>
> Signed-off-by: Naveed Khan <naveed@digiscrypt.com>

This fixes a heap out-of-bounds write, but there is no Fixes: tag.

The out-of-bounds store here:

	*((struct bpf_program **)(st_ops->data + moff)) = prog;

looks like it was introduced when this function started writing into the
shadow data.  Before that, the relocation only wrote to
st_ops->progs[member_idx], so the heap write past value_size did not
exist.

Should this carry:

  Fixes: 69e4a9d2b3f5 ("libbpf: Convert st_ops->data to shadow type.")


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/29987958246