Re: [PATCH v3 net 1/3] flow_dissector: avoid u16 truncation of skb->len when computing thoff

[Eric Dumazet <edumazet@kernel.org>]

On 10/5/26 17:30, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The fix is incomplete: __skb_flow_dissect() still has a second
>    path that truncates a length to u16 when it computes thoff, the BPF…
> 
> --
> 
> flow_dissector: avoid u16 truncation of skb->len when computing thoff
> 
> This patch changes the thoff computation at the end of
> __skb_flow_dissect() to compare in u32 instead of u16. If the result
> does not fit in key_control->thoff, it is capped to U16_MAX and the
> dissection is reported as failed.
> 
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index 8aa4f9b4df810..27d8a01bc9230 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
> 
> [ ... ]
> 
>> @@ -1692,7 +1693,13 @@ bool __skb_flow_dissect(const struct net *net,
>>   	ret = true;
>>   
>>   out:
>> -	key_control->thoff = min_t(u16, nhoff, skb ? skb->len : hlen);
>> +	thoff = min_t(u32, nhoff, skb ? skb->len : hlen);
>> +	if (unlikely(thoff > U16_MAX)) {
>> +		/* Cannot be represented in key_control->thoff. */
>> +		thoff = U16_MAX;
>> +		ret = false;
>> +	}
>> +	key_control->thoff = thoff;
> 
> [Severity: Medium]
> Does the BPF flow dissector path in the same function still have the
> same u16 truncation?


Seems legit, I will take care of this in a separate and stand alone 
patch next week (after LPC)

> 
> When a BPF flow dissector is attached to init_net or the netns,
> __skb_flow_dissect() returns early and never reaches this out: label:
> 
> net/core/flow_dissector.c:__skb_flow_dissect() {
>      ...
> 	result = bpf_flow_dissect(prog, &ctx, n_proto, nhoff,
> 				  hlen, flags);
> 	if (result != BPF_FLOW_DISSECTOR_CONTINUE) {
> 		__skb_flow_bpf_to_target(&flow_keys, flow_dissector,
> 					 target_container);
> 		rcu_read_unlock();
> 		return result == BPF_OK;
> 	}
>      ...
> }
> 
> bpf_flow_dissect() still clamps as u16, so the int hlen (skb_headlen())
> is truncated before the comparison:
> 
> net/core/flow_dissector.c:bpf_flow_dissect() {
>      ...
> 	flow_keys->nhoff = clamp_t(u16, flow_keys->nhoff, nhoff, hlen);
> 	flow_keys->thoff = clamp_t(u16, flow_keys->thoff,
> 				   flow_keys->nhoff, hlen);
>      ...
> }
> 
> Then __skb_flow_bpf_to_target() copies the value as is:
> 
> 	key_control->thoff = flow_keys->thoff;
> 
> Take a linear head of 65540 bytes and a program that reports nhoff=14,
> thoff=34 and returns BPF_OK. (u16)hlen is 4, so both offsets collapse
> to 4, and __skb_flow_dissect() returns true.
> 
> That seems to be the case the commit message describes:
> 
>    "Whenever skb->len (or hlen) modulo 65536 is smaller than nhoff, the
>    truncated length wins and thoff is set to a bogus small value, even
>    when the dissection succeeded."
> 
> and:
> 
>    "report the dissection as failed rather than silently returning a
>    wrong transport offset."
> 
> skb_headlen() above 65535 looks reachable. packet_snd() skips the MTU
> check when vnet_hdr.gso_type is set. Both packet_alloc_skb() and
> tun_alloc_skb() grow the linear part when len exceeds the fragment
> capacity:
> 
> 	linear = len - MAX_SKB_FRAGS * (PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER);
> 
> So a GSO write of roughly 622KB on 4K pages gives a linear head over
> 64KB. Attaching the BPF dissector does require privilege.
> 
> Callers that trust a successful return would then put the transport
> header inside the L2/L3 header. Examples are skb_probe_transport_header()
> calling skb_set_transport_header(skb, keys.control.thoff). Another is
> __virtio_net_hdr_to_skb(), after "net: always dissect GSO packets in
> __virtio_net_hdr_to_skb()" later in the series. The later patches in
> the series leave bpf_flow_dissect() unchanged.
> 
> Should bpf_flow_dissect() also compare in u32, or cap hlen at U16_MAX,
> and report failure when the offset cannot be represented?
> 
>>   	key_basic->n_proto = proto;
>>   	key_basic->ip_proto = ip_proto;
>>
>