netdev - Re: [PATCH bpf-next v6 9/9] selftests/bpf: Cover metadata access from a modified skb clone

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: <ff37dd97-dde7-48a3-9bb6-7d424f94e345@linux.dev>
Date: Tue, 12 Aug 2025 11:20:35 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Jakub Sitnicki <jakub@...udflare.com>
Cc: Alexei Starovoitov <ast@...nel.org>, Andrii Nakryiko <andrii@...nel.org>,
 Arthur Fabre <arthur@...hurfabre.com>, Daniel Borkmann
 <daniel@...earbox.net>, Eduard Zingerman <eddyz87@...il.com>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Jesper Dangaard Brouer <hawk@...nel.org>,
 Jesse Brandeburg <jbrandeburg@...udflare.com>,
 Joanne Koong <joannelkoong@...il.com>, Lorenzo Bianconi
 <lorenzo@...nel.org>, Toke Høiland-Jørgensen
 <thoiland@...hat.com>, Yan Zhai <yan@...udflare.com>,
 kernel-team@...udflare.com, netdev@...r.kernel.org, bpf@...r.kernel.org,
 Stanislav Fomichev <sdf@...ichev.me>
Subject: Re: [PATCH bpf-next v6 9/9] selftests/bpf: Cover metadata access from
 a modified skb clone

On 8/12/25 6:12 AM, Jakub Sitnicki wrote:
>> No strong opinion to either copy the metadata on a clone or set the dynptr
>> rdonly for a clone. I am ok with either way.
>>
>> A brain dump:
>> On one hand, it is hard to comment without visibility on how will it look like
>> when data_meta can be preserved in the future, e.g. what may be the overhead but
>> there is flags in bpf_dynptr_from_skb_meta and bpf_dynptr_write, so there is
>> some flexibility. On the other hand, having a copy will be less surprise on the
>> clone skb like what we have discovered in this and the earlier email thread but
>> I suspect there is actually no write use case on the skb data_meta now.
> 
> All makes sense.
> 
> To keep things simple and consistent, it would be best to have a single
> unclone (bpf_try_make_writable) point caused by a write to metadata
> through an skb clone.
> 
> Today, the unclone in the prologue can already be triggered by a write
> to data_meta from a dead branch. Despite being useless, since
> pskb_expand_head resets meta_len.
> 
> We also need the prologue unclone for bpf_dynptr_slice_rdwr created from
> an skb_meta dynptr, because creating a slice does not invalidate packet
> pointers by contract.
> 
> So I'm thinking it makes sense to unclone in the prologue if we see a
> potential bpf_dynptr_write to skb_meta dynptr as well. This could be
> done by tweaking check_helper_call to set the seen_direct_write flag:
> 
> static int check_helper_call(...)
> {
>          // ...
>         	switch (func_id) {
>          // ...
> 	case BPF_FUNC_dynptr_write:
> 	{
>                  // ...
> 		dynptr_type = dynptr_get_type(env, reg);
>                  // ...
> 		if (dynptr_type == BPF_DYNPTR_TYPE_SKB ||
> 		    dynptr_type == BPF_DYNPTR_TYPE_SKB_META)
> 			changes_data = true;

This looks ok.

> 		if (dynptr_type == BPF_DYNPTR_TYPE_SKB_META)
> 			env->seen_direct_write = true;

The "seen_direct_write = true;" addition will be gone from the verifier 
eventually when pskb_expand_head can keep the data_meta (?). Right, there are 
existing cases that the prologue call might be unnecessary. However, I don't 
think it should be the reason that it can set "seen_direct_write" on top of the 
"changes_data". I think it is confusing.

> 
> 		break;
> 	}
>          // ...
> }
> 
> That would my the plan for the next iteration, if it sounds sensible.
> 
> As for keeping metadata intact past a pskb_expand_head call, on second
> thought, I'd leave that for the next patch set, to keep the patch count
> within single digits.

If the plan is to make pskb_expand_head support the data_meta later, just set 
the rdonly bit in the bpf_dynptr_from_skb_meta now. Then the future 
pskb_expand_head change will be a clean change in netdev and filter.c, and no 
need to revert the "seen_direct_write" changes from the verifier.