Re: [PATCH net 1/1] bpf: reject unsafe pointers in TCP congestion control access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: Commit message describes impossible exploit vector via bpf_rdonly_cast
--

commit fc4f1a419dfb22513647d5927643a1274a3968ac
Author: Zhiling Zou <zhilinz@nebusec.ai>

bpf: reject unsafe pointers in TCP congestion control access

This commit adds a check in bpf_tcp_ca_btf_struct_access() to reject
pointers with unsafe modifiers. This is intended to prevent BPF programs
from using bpf_rdonly_cast() to create untrusted tcp_sock pointers and
modifying allowlisted members.

> bpf_tcp_ca_btf_struct_access() authorizes writes based on the BTF type
> and member offset, but does not check pointer modifiers. A BPF program
> can use bpf_rdonly_cast() to create an untrusted tcp_sock pointer and
> write through an allowlisted member, bypassing the read-only cast.

[Severity: Medium]
Does bpf_rdonly_cast() actually allow this bypass?

bpf_rdonly_cast() produces a pointer with the PTR_UNTRUSTED modifier for
structs. Looking at check_ptr_to_btf_access() in kernel/bpf/verifier.c, the
verifier natively rejects BPF_WRITE access for PTR_UNTRUSTED pointers before
the bpf_tcp_ca_btf_struct_access() callback is ever reached:

kernel/bpf/verifier.c:check_ptr_to_btf_access() {
    ...
    if (atype != BPF_READ && bpf_may_fault_on_deref(reg->type)) {
        verbose(env, "only read is supported\n");
        return -EACCES;
    }
    ...
}

Because bpf_may_fault_on_deref() returns true for PTR_UNTRUSTED, wouldn't
this write already be rejected by the verifier with -EACCES?

> diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> index 791e15063237c..252cbfe11e694 100644
> --- a/net/ipv4/bpf_tcp_ca.c
> +++ b/net/ipv4/bpf_tcp_ca.c
> @@ -65,6 +65,11 @@ static int bpf_tcp_ca_btf_struct_access(struct bpf_verifier_log *log,
>  	const struct btf_type *t;
>  	size_t end;
>  
> +	if (bpf_type_has_unsafe_modifiers(reg->type)) {
> +		bpf_log(log, "write through an unsafe tcp_sock pointer\n");
> +		return -EACCES;
> +	}

[Severity: Medium]
Since PTR_UNTRUSTED is blocked earlier, is this code unreachable for the
bpf_rdonly_cast() vector described in the commit message?

It appears this change is still valuable because it prevents writes from other
unsafe modifiers (like MEM_RCU and MEM_RDONLY) that don't trigger
bpf_may_fault_on_deref() and would otherwise be improperly permitted by this
callback.

Could the commit message be updated to describe the actual vectors this change
protects against, rather than the impossible bpf_rdonly_cast() vector?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791978740.git.zhilinz@nebusec.ai?part=1