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

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.

[ ... ]

[Severity: Medium]
Does the BPF flow dissector path in the same function still have the
same u16 truncation?

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;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001191140.2818991-1-edumazet%40kernel.org