Re: [PATCH net 1/2] psp: drop transport mode packets with extension headers

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] The new check in psp_dev_encapsulate()
  (net/psp/psp_main.c:260-262) only looks for IPv6 extension headers…

--

psp: drop transport mode packets with extension headers

This adds a check to psp_dev_encapsulate() that rejects IPv6 packets
whose network header length is not sizeof(struct ipv6hdr). Without it,
the nexthdr of the fixed IPv6 header gets rewritten to UDP even when
extension headers sit between it and the TCP header. That produced
malformed packets, and on mlx5 unencrypted ones.

> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index c9c1a8826b7f9..0d276e2aceff4 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c

[ ... ]

> @@ -256,6 +256,11 @@ bool psp_dev_encapsulate(struct net *net, struct sk_buff *skb, __be32 spi,
>  	    skb->protocol != htons(ETH_P_IPV6))
>  		return false;
>  
> +	/* extension headers not supported */
> +	if (skb->protocol == htons(ETH_P_IPV6) &&
> +	    network_len != sizeof(struct ipv6hdr))
> +		return false;
> +

[Severity: High]
Does this check catch the case where a route inserts the extension
headers? The commit message gives that case as the reason for a datapath
check:

  "extension headers can also come from a route, so the datapath check
  is needed either way."

network_len comes from skb_network_header_len(), which is only
transport_header - network_header. The lwtunnel encapsulators that add
IPv6 extension headers on egress insert the extension header and then set
the transport header to the offset right after the fixed header:

net/ipv6/seg6_iptunnel.c:seg6_do_srh() {
    ...
	skb_set_transport_header(skb, sizeof(struct ipv6hdr));
	nf_reset_ct(skb);
    ...
}

rpl_do_srh_inline() in net/ipv6/rpl_iptunnel.c does the same. So do the
ioam6 inline and encap helpers in net/ipv6/ioam6_iptunnel.c, which call
skb_set_transport_header(skb, sizeof(*hdr)).

Take a PSP TCP flow that leaves the PSP netdev through a seg6, rpl or
ioam6 lwtunnel route. Its network_len is exactly sizeof(struct ipv6hdr),
even though a Routing or Hop-by-Hop header sits right after the fixed
header:

seg6_output_core()
  seg6_do_srh()
    skb_set_transport_header(skb, sizeof(struct ipv6hdr))
dev_queue_xmit()
  psp_validate_xmit()
  mlx5e_psp_handle_tx_skb() (or nsim_do_psp())
    psp_dev_encapsulate()
      network_len == sizeof(struct ipv6hdr), so the new check passes

After that, psp_dev_encapsulate() still does:

		ipv6_hdr(skb)->nexthdr = IPPROTO_UDP;

This overwrites NEXTHDR_ROUTING or NEXTHDR_HOP. The UDP and PSP headers
end up in front of the extension header. psp_write_headers() also sets
psph->nexthdr = IPPROTO_TCP, but the next bytes are an SRH or HBH header.

Wouldn't this produce the same kind of malformed packet that this patch
is meant to drop?

The follow-up selftest patch in the series doesn't touch psp_main.c, so
this path still seems reachable at the end of the series.

Could the check look at the header chain itself instead? For example,
rejecting the packet when ipv6_hdr(skb)->nexthdr != IPPROTO_TCP would
cover both the socket option case and the route case.

>  	if (skb_cow_head(skb, PSP_ENCAP_HLEN))
>  		return false;
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021210200.788191-1-toddfreed%40meta.com