Skip to content

bpf: Keep fault protection when merging pointer types - #13310

Closed
kernel-patches-daemon-bpf[bot] wants to merge 7 commits into
bpf-next_basefrom
series/1146311=>bpf-next
Closed

bpf: Keep fault protection when merging pointer types#13310
kernel-patches-daemon-bpf[bot] wants to merge 7 commits into
bpf-next_basefrom
series/1146311=>bpf-next

Conversation

@kernel-patches-daemon-bpf

Copy link
Copy Markdown

Pull request for series with
subject: bpf: Keep fault protection when merging pointer types
version: 2
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 90bd032
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311
version: 2

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: a2b83a8
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311
version: 2

@kernel-patches-review-bot

Copy link
Copy Markdown
commit bcfd0d6a160f158cc5a0311b54efcee59212c10d
Author: Daniel Borkmann <daniel@iogearbox.net>

bpf: Reject a store through a fault prone pointer

This commit prevents kernel panics from BPF stores through fault-prone
pointers by widening the check from PTR_UNTRUSTED to
bpf_may_fault_on_deref(), covering bare PTR_TO_BTF_ID pointers that may
fault but lack BPF_PROBE_MEM protection.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 814096ed835d..cbbb0c82d22c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5788,7 +5788,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
>  		return -EACCES;
>  	}
>
> -	if (atype != BPF_READ && (type_flag(reg->type) & PTR_UNTRUSTED)) {
> +	if (atype != BPF_READ && bpf_may_fault_on_deref(reg->type)) {
>  		verbose(env, "only read is supported\n");
>  		return -EACCES;
>  	}

Does this change also reject HID-BPF writes to struct hid_device fields?

The HID struct_ops hid_bpf_ops_btf_struct_access() whitelists writes to
hid_device::name, ::uniq, and ::phys via WRITE_RANGE entries in
drivers/hid/bpf/hid_bpf_struct_ops.c. A HID program receives a
'struct hid_bpf_ctx *' argument that is PTR_TO_BTF_ID | PTR_TRUSTED, but
walking hctx->hid produces a bare PTR_TO_BTF_ID because
struct hid_bpf_ctx appears in no BTF_TYPE_SAFE_TRUSTED list in
check_ptr_to_btf_access().

Before this commit, 'hctx->hid->name[i] = c' passed the PTR_UNTRUSTED
test and reached env->ops->btf_struct_access(), where
hid_bpf_ops_btf_struct_access() returned 0 for offsets in the whitelisted
ranges and the store succeeded. After this commit,
bpf_may_fault_on_deref(PTR_TO_BTF_ID) is true and the store is rejected
with 'only read is supported' before the btf_struct_access callback runs.

The three WRITE_RANGE(hid_device, ...) entries become unreachable. The
commit message notes the bpf_qdisc Qdisc::next_sched case but does not
mention HID-BPF. If the intent is to preserve HID-BPF write capability,
hid_bpf_ctx::hid would need a trusted annotation
(BTF_TYPE_SAFE_TRUSTED(struct hid_bpf_ctx) { struct hid_device *hid; })
so the walked register stays PTR_TRUSTED. Otherwise, should the removal
be documented?



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

In-Reply-To-Subject: bpf: Reject a store through a fault prone pointer
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31845722500

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: fdd4fad
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311
version: 2

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-review-bot

Copy link
Copy Markdown
commit 6708731b00b8c6bc2aa916fb57f2004eac94caf7
Author: Daniel Borkmann <daniel@iogearbox.net>

selftests/bpf: Add tests for pointer type merge at a shared load

This commit adds six new test cases to cover scenarios where the type
recorded for a shared load used to lose the BPF_PROBE_MEM rewrite,
which would trigger a NULL deref if not handled properly.

> diff --git a/tools/testing/selftests/bpf/progs/mem_rdonly_untrusted.c b/tools/testing/selftests/bpf/progs/mem_rdonly_untrusted.c
> index b91271d4caa4..ac8bba3e7d02 100644
> --- a/tools/testing/selftests/bpf/progs/mem_rdonly_untrusted.c
> +++ b/tools/testing/selftests/bpf/progs/mem_rdonly_untrusted.c

[ ... ]

> +SEC("socket")
> +__success
> +__log_level(2)
> +__msg("= *(u64 *)(r{{[0-9]}} +0){{.*}}=untrusted_ptr_sock")
> +__msg("= *(u64 *)(r{{[0-9]}} +0){{.*}}=ringbuf_mem")
> +__retval(0)
> +int mixed_mem_untrusted_btf_id_type(void *ctx)

[ ... ]

> +SEC("socket")
> +__success
> +__log_level(2)
> +__msg("= *(u32 *)(r{{[0-9]}} +0){{.*}}=ptr_nameidata")
> +__msg("= *(u32 *)(r{{[0-9]}} +0){{.*}}=ringbuf_mem")
> +__retval(0)
> +int mixed_mem_btf_id_type(void *ctx)

[ ... ]

> +SEC("socket")
> +__success
> +__log_level(2)
> +__msg("r8 = *(u32 *)(r7 +0){{.*}}R7=ptr_nameidata")
> +__msg("r8 = *(u32 *)(r7 +0){{.*}}R7=rdonly_mem")
> +__retval(0)
> +int mixed_rdonly_mem_btf_id_type(void *ctx)

[ ... ]

> +SEC("socket")
> +__success
> +__log_level(2)
> +__msg("= *(u64 *)(r{{[0-9]}} +0){{.*}}=ringbuf_mem")
> +__msg("= *(u64 *)(r{{[0-9]}} +0){{.*}}=rdonly_untrusted_mem")
> +__retval(0)
> +int mixed_mem_mem_type(void *ctx)

[ ... ]

> +SEC("socket")
> +__failure
> +__msg("same insn cannot be used with different pointers")
> +int mixed_map_value_mem_type(void *ctx)

[ ... ]

> +SEC("socket")
> +__failure
> +__msg("same insn cannot be used with different pointers")
> +int mixed_stack_mem_type(void *ctx)

[ ... ]

A subsystem pattern flags this as potentially concerning: these six
tests cover only two of the four commits in the series. The three merge
shapes listed in dea41ba6e67d's changelog map 1:1 onto the
mixed_mem_untrusted_btf_id_type, mixed_mem_btf_id_type, and
mixed_rdonly_mem_btf_id_type tests, and the mixed_mem_mem_type,
mixed_map_value_mem_type, and mixed_stack_mem_type tests cover the new
PTR_TO_MEM mismatch rule from fbffa57329b8.

However, the other two commits appear untested:

bcfd0d6a160f changed check_ptr_to_btf_access() from checking
type_flag(reg->type) & PTR_UNTRUSTED to calling
bpf_may_fault_on_deref(), so a BPF_STX through a bare PTR_TO_BTF_ID is
now rejected with "only read is supported". The changelog documents a
reachable bpf_qdisc panic (next = sch->next_sched; next->limit = 1000;),
but no __failure / __msg("only read is supported") case is added here or
in progs/bpf_qdisc_*.c. Only verifier_global_ptr_args.c and
verifier_netfilter_ctx.c assert that message, and both do so for
PTR_UNTRUSTED pointers that the old check already caught, so the
newly-closed hole is untested.

bef47cf76444 states that PTR_TO_BTF_ID|PTR_UNTRUSTED|MEM_ALLOC|NON_OWN_REF
and PTR_TO_BTF_ID|PTR_UNTRUSTED|MEM_RCU were reachable and left as plain
loads (two Fixes: tags). The added tests only produce
ringbuf/rdonly/bare-BTF-ID types, all of which the old exact-match list
in bpf_convert_ctx_accesses() already handled or which the merge path
covers.

Would it make sense to add tests for the bcfd0d6a160f store-rejection path
and the bef47cf76444 MEM_ALLOC|NON_OWN_REF and MEM_RCU cases here?



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

In-Reply-To-Subject: selftests/bpf: Add tests for pointer type merge at a shared load
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31845722500

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: f5b57e9
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311
version: 2

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: f5b57e9
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311
version: 2

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: d82ebfc
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311
version: 2

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: ce7c9f6
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311
version: 2

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: c93cbdb
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311
version: 2

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: c93cbdb
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311
version: 2

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: c93cbdb
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311
version: 2

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: c93cbdb
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311
version: 2

When the same BPF_LDX instruction is reached through paths that yield
different pointer types, save_aux_ptr_type() merges them into a single
type which is later used by bpf_convert_ctx_accesses() to decide whether
the load has to be rewritten into a BPF_PROBE_MEM one.

Before f2362a5 ("bpf: allow void* cast using bpf_rdonly_cast()")
the merge only accepted two PTR_TO_BTF_ID pointers and unconditionally
fell back to PTR_TO_BTF_ID | PTR_UNTRUSTED, so the merged type was always
one that gets the BPF_PROBE_MEM rewrite. However, the mentioned commit
widened the merge to also cover a PTR_TO_MEM base and replaced the
fallback by a union of the PTR_UNTRUSTED and MEM_RDONLY flags.

A union of flags though cannot express the property the later rewrite
is built upon, some examples:

  - PTR_TO_MEM merged with PTR_TO_BTF_ID | PTR_UNTRUSTED gets
    PTR_TO_MEM | PTR_UNTRUSTED but only the MEM_RDONLY variant is valid
  - PTR_TO_MEM merged with a plain PTR_TO_BTF_ID gets PTR_TO_MEM
    dropping the rewrite the latter type would have gotten
  - PTR_TO_MEM | MEM_RDONLY merged with a plain PTR_TO_BTF_ID gets
    PTR_TO_MEM | MEM_RDONLY which is not rewritten either since only
    its PTR_UNTRUSTED variant is

In all three cases a program can take the unsafe path at runtime with a
NULL or otherwise bad pointer and panic the kernel on the faulting load:

  BUG: kernel NULL pointer dereference, address: 0000000000000038
  RIP: 0010:bpf_prog_77531a87032eeaf1_mixed_mem_btf_id_type+0x4b/0x65
  Call Trace:
   <TASK>
   bpf_test_run+0x20b/0x460
   bpf_prog_test_run_skb+0x650/0xbe0
   __sys_bpf+0xb96/0x3140
   __x64_sys_bpf+0x2c/0x40
   do_syscall_64+0xba/0x590
  Kernel panic - not syncing: Fatal exception in interrupt

Note that the last two shapes have to be fixed right here, otherwise
the merged type retains nothing which marks the load as fault prone,
thus no rule in bpf_convert_ctx_accesses() can recover it. Fix it by
normalizing the merged type instead.

Reuse it in is_load_acq_unsafe() to avoid open coding, and trim the
overly verbose comment which is more of an implementation detail of
bpf_convert_ctx_accesses() anyway.

Fixes: f2362a5 ("bpf: allow void* cast using bpf_rdonly_cast()")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
reg_type_mismatch_ok() enumerates the pointer types which must not
silently share a BPF_LDX with a different one, since the type recorded
for the insn drives a rewrite in bpf_convert_ctx_accesses().

f2362a5 ("bpf: allow void* cast using bpf_rdonly_cast()") added
PTR_TO_MEM | MEM_RDONLY | PTR_UNTRUSTED as another type in need of one,
namely the BPF_PROBE_MEM rewrite, but did not add it there. Fix it by
adding the missing case to reg_type_mismatch_ok(), so that a PTR_TO_MEM
which may fault on deref is not mismatch ok anymore. The triage in
save_aux_ptr_type() then merges them.

Fixes: f2362a5 ("bpf: allow void* cast using bpf_rdonly_cast()")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
check_ptr_to_btf_access() allows the program to store before the default
BTF access path gets to reject a non read access. ac65c71 ("bpf:
Reject writes through untrusted BTF pointers") closed that for a
PTR_UNTRUSTED pointer, but a bare PTR_TO_BTF_ID may fault on a dereference
just the same and is let through.

A BPF_LDX gets the BPF_PROBE_MEM rewrite in bpf_convert_ctx_accesses()
and a bad address is handled, but a BPF_STX does not and cannot, there
is no probed store to rewrite. The store is emitted as a plain one without
an exception table entry and a bad address panics the kernel.

A bpf_qdisc program can reach this, bpf_qdisc_btf_struct_access() permits a
write to Qdisc::limit and Qdisc::next_sched is a plain struct Qdisc pointer
which the walk turns into the compat type:

  struct Qdisc *next = sch->next_sched;

  next->limit = 1000;

  BUG: kernel NULL pointer dereference, address: 0000000000000014
  RIP: 0010:bpf_prog_c6e14e7f32c8e325_bpf_fifo_enqueue+0x3a/0x12b
  Code: [...] bf e8 03 00 00 <89> 7e 14 41 8b 7f 14 [...]
  Kernel panic - not syncing: Fatal exception in interrupt

Fix by widen the check to bpf_may_fault_on_deref() so that it covers both.

Fixes: 27ae799 ("bpf: Introduce BPF_PROG_TYPE_STRUCT_OPS")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: c93cbdb
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311
version: 2

bpf_convert_ctx_accesses() turns a BPF_LDX into a BPF_PROBE_MEM one by
matching the type recorded for the insn against a list of exact pointer
types. The list cannot keep up with the flag combinations the verifier
produces, and a type which is missing from it ends up as a plain load
without an exception table entry, so a bad address panics the kernel
instead of being handled.

Two such types exist today and are reachable:

  - PTR_TO_BTF_ID | PTR_UNTRUSTED | MEM_ALLOC | NON_OWN_REF
  - PTR_TO_BTF_ID | PTR_UNTRUSTED | MEM_RCU

Rather than adding the two, just drop the list and state the property
itself in the default case of the switch. This is a superset of what
the list matched, the untrusted PTR_TO_MEM does not have to carry
MEM_RDONLY for it anymore, and it stays in sync with the verifier side
which uses the same match in save_aux_ptr_type() and reg_type_mismatch_ok().

Assert that a fault prone type which does not get the rewrite for whatever
reason is rejected at load time rather than left to fault at runtime to
catch any future cases.

Fixes: 1b12171 ("bpf: Mark direct ld of stashed bpf_{rb,list}_node as non-owning ref")
Fixes: 6fcd486 ("bpf: Refactor RCU enforcement in the verifier.")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Cover the ways in which the type recorded for a shared load used to lose
the BPF_PROBE_MEM rewrite which would then trigger a NULL deref if not
handled properly.

  # LDLIBS=-static PKG_CONFIG='pkg-config --static' ./vmtest.sh -- ./test_progs -t mem_rdonly_untrusted
  [...]
  #238/1   mem_rdonly_untrusted/btf_id_to_ptr_mem:OK
  #238/2   mem_rdonly_untrusted/ldx_is_ok_bad_addr:OK
  #238/3   mem_rdonly_untrusted/ldx_is_ok_good_addr:OK
  #238/4   mem_rdonly_untrusted/offset_not_tracked:OK
  #238/5   mem_rdonly_untrusted/stx_not_ok:OK
  #238/6   mem_rdonly_untrusted/atomic_not_ok:OK
  #238/7   mem_rdonly_untrusted/atomic_rmw_not_ok:OK
  #238/8   mem_rdonly_untrusted/kfunc_param_not_ok:OK
  #238/9   mem_rdonly_untrusted/mixed_mem_type:OK
  #238/10  mem_rdonly_untrusted/mixed_mem_untrusted_btf_id_type:OK
  #238/11  mem_rdonly_untrusted/mixed_mem_btf_id_type:OK
  #238/12  mem_rdonly_untrusted/mixed_rdonly_mem_btf_id_type:OK
  #238/13  mem_rdonly_untrusted/mixed_mem_mem_type:OK
  #238/14  mem_rdonly_untrusted/mixed_map_value_mem_type:OK
  #238/15  mem_rdonly_untrusted/mixed_stack_mem_type:OK
  #238/16  mem_rdonly_untrusted/diff_size_access:OK
  #238/17  mem_rdonly_untrusted/misaligned_access:OK
  #238/18  mem_rdonly_untrusted/null_check:OK
  #238/19  mem_rdonly_untrusted/ldx_is_ok_commuted_addr:OK
  #238/20  mem_rdonly_untrusted/helper_param_not_ok:OK
  #238     mem_rdonly_untrusted:OK
  Summary: 1/20 PASSED, 0 SKIPPED, 0/0 FAILED

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Cover the two loads which used to lose the BPF_PROBE_MEM rewrite, both
reached from an RCU read-side critical section.

  # LDLIBS=-static PKG_CONFIG='pkg-config --static' ./vmtest.sh -- ./test_progs -t rcu_read_lock
  [...]
  #332/1   rcu_read_lock/success:OK
  #332/2   rcu_read_lock/rcuptr_acquire:OK
  #332/3   rcu_read_lock/negative_tests_inproper_region:OK
  #332/4   rcu_read_lock/negative_tests_rcuptr_misuse:OK
  #332     rcu_read_lock:OK
  Summary: 1/4 PASSED, 0 SKIPPED, 0/0 FAILED

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=1146311 expired. Closing PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant