netdev - Re: [PATCH v13 bpf-next 09/10] bpf: Add bpf_dynptr_slice and bpf_dynptr_slice

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: <20230306071006.73t5vtmxrsykw4zu@apollo>
Date:   Mon, 6 Mar 2023 08:10:06 +0100
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     Joanne Koong <joannelkoong@...il.com>
Cc:     bpf@...r.kernel.org, martin.lau@...nel.org, andrii@...nel.org,
        ast@...nel.org, daniel@...earbox.net, netdev@...r.kernel.org,
        toke@...nel.org
Subject: Re: [PATCH v13 bpf-next 09/10] bpf: Add bpf_dynptr_slice and
 bpf_dynptr_slice_rdwr

On Wed, Mar 01, 2023 at 04:49:52PM CET, Joanne Koong wrote:
> Two new kfuncs are added, bpf_dynptr_slice and bpf_dynptr_slice_rdwr.
> The user must pass in a buffer to store the contents of the data slice
> if a direct pointer to the data cannot be obtained.
>
> For skb and xdp type dynptrs, these two APIs are the only way to obtain
> a data slice. However, for other types of dynptrs, there is no
> difference between bpf_dynptr_slice(_rdwr) and bpf_dynptr_data.
>
> For skb type dynptrs, the data is copied into the user provided buffer
> if any of the data is not in the linear portion of the skb. For xdp type
> dynptrs, the data is copied into the user provided buffer if the data is
> between xdp frags.
>
> If the skb is cloned and a call to bpf_dynptr_data_rdwr is made, then
> the skb will be uncloned (see bpf_unclone_prologue()).
>
> Please note that any bpf_dynptr_write() automatically invalidates any prior
> data slices of the skb dynptr. This is because the skb may be cloned or
> may need to pull its paged buffer into the head. As such, any
> bpf_dynptr_write() will automatically have its prior data slices
> invalidated, even if the write is to data in the skb head of an uncloned
> skb. Please note as well that any other helper calls that change the
> underlying packet buffer (eg bpf_skb_pull_data()) invalidates any data
> slices of the skb dynptr as well, for the same reasons.
>
> Signed-off-by: Joanne Koong <joannelkoong@...il.com>
> ---

Sorry for chiming in late.

I see one potential hole in bpf_dynptr_slice_rdwr. If the returned pointer is
actually pointing to the stack (but verified as a PTR_TO_MEM in verifier state),
we won't reflect changes to the stack state in the verifier for writes happening
through it.

For the worst case scenario, this will basically allow overwriting values of
spilled pointers and doing arbitrary kernel memory reads/writes. This is only an
issue when bpf_dynptr_slice_rdwr at runtime returns a pointer to the supplied
buffer residing on program stack. To verify, by forcing the memcpy to buffer for
skb_header_pointer I was able to make it dereference a garbage value for
l4lb_all selftest.

--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2253,7 +2253,13 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
 	case BPF_DYNPTR_TYPE_RINGBUF:
 		return ptr->data + ptr->offset + offset;
 	case BPF_DYNPTR_TYPE_SKB:
-		return skb_header_pointer(ptr->data, ptr->offset + offset, len, buffer);
+	{
+		void *p = skb_header_pointer(ptr->data, ptr->offset + offset, len, buffer);
+		if (p == buffer)
+			return p;
+		memcpy(buffer, p, len);
+		return buffer;
+	}

--- a/tools/testing/selftests/bpf/progs/test_l4lb_noinline_dynptr.c
+++ b/tools/testing/selftests/bpf/progs/test_l4lb_noinline_dynptr.c
@@ -470,7 +470,10 @@ int balancer_ingress(struct __sk_buff *ctx)
 	eth = bpf_dynptr_slice_rdwr(&ptr, 0, buffer, sizeof(buffer));
 	if (!eth)
 		return TC_ACT_SHOT;
-	eth_proto = eth->eth_proto;
+	*(void **)buffer = ctx;
+	*(void **)eth = (void *)0xdeadbeef;
+	ctx = *(void **)buffer;
+	eth_proto = eth->eth_proto + ctx->len;
 	if (eth_proto == bpf_htons(ETH_P_IP))
 		err = process_packet(&ptr, eth, nh_off, false, ctx);

I think the proper fix is to treat it as a separate return type distinct from
PTR_TO_MEM like PTR_TO_MEM_OR_PKT (or handle PTR_TO_MEM | DYNPTR_* specially),
fork verifier state whenever there is a write, so that one path verifies it as
PTR_TO_PACKET, while another as PTR_TO_STACK (if buffer was a stack ptr). I
think for the rest it's not a problem, but there are allow_ptr_leak checks
applied to PTR_TO_STACK and PTR_TO_MAP_VALUE, so that needs to be rechecked.
Then we ensure that program is safe in either path.

Also we need to fix regsafe to not consider other PTR_TO_MEMs equivalent to such
a pointer. We could also fork verifier states on return, to verify either path
separately right from the point following the call instruction.

Any other ideas welcome.