Re: [PATCH net v3 2/4] psp: support ipv6 extension headers in psp_dev_rcv() decap

[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 (1)

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] psp_dev_rcv() now decrypts and decaps PSP packets that carry
  IPv6 extension headers.

Pre-existing issues:
- [Low] TX/RX asymmetry for IPv6 extension headers.

--

psp: support ipv6 extension headers in psp_dev_rcv() decap

This patch makes psp_dev_rcv() skip IPv6 extension headers to find the
UDP and PSP headers. On decap it writes the inner nexthdr into the last
extension header instead of the fixed header. Packets that carry fragment
or authentication headers are dropped.

> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index 273b010d2355c..206ab7c3e3b51 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c

[ ... ]

> @@ -188,6 +189,39 @@ unsigned int psp_key_size(u32 version)
>  }
>  EXPORT_SYMBOL(psp_key_size);
>  

[ ... ]

> +static int psp_ipv6_nexthdr_off(const u8 *hdr, unsigned int len)
> +{
> +	unsigned int nh = offsetof(struct ipv6hdr, nexthdr);
> +	unsigned int off = sizeof(struct ipv6hdr);
> +	const struct ipv6_opt_hdr *eh;
> +	unsigned int rem;
> +	u8 type;
> +
> +	while (off < len) {
> +		eh = (const void *)(hdr + off);
> +		type = hdr[nh];
> +
> +		if (!ipv6_ext_hdr(type) || type == NEXTHDR_NONE ||
> +		    type == NEXTHDR_FRAGMENT || type == NEXTHDR_AUTH)
> +			return -EINVAL;

[Severity: High]
This filter only rejects NEXTHDR_FRAGMENT, NEXTHDR_AUTH and NEXTHDR_NONE.
That means routing headers of any type and any segments_left value are
accepted. So are hop-by-hop and destination options headers with any
option content. Could this expose decrypted plaintext through ICMPv6
errors?

The PSP ICV doesn't cover the outer IPv6 header or its extension headers.
An on-path attacker could take a captured PSP packet, insert one of these
headers, and set saddr to an address they control. This assumes the
device authenticates such packets, which this patch's premise implies.

Once psp_dev_rcv() succeeds, the driver marks the skb as decrypted.
For example, mlx5e_psp_offload_handle_rx_skb() sets skb->decrypted = 1.
The IPv6 stack then processes the extension headers before tcp_v6_rcv()
reaches psp_sk_rx_policy_check().

Several of those handlers reply with an ICMPv6 error that quotes the
packet. With default sysctls, this happens for an unrecognized routing
header type (type 0, for example) with segments_left != 0:

net/ipv6/exthdrs.c:ipv6_rthdr_rcv() {
    ...
unknown_rh:
	__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
	icmpv6_param_prob(skb, ICMPV6_HDR_FIELD,
			  (&hdr->type) - skb_network_header(skb));
    ...
}

It also happens for an unknown destination or hop-by-hop option whose
action bits request a parameter problem, through
ip6_parse_tlv()->ip6_tlvopt_unknown():

	case 2: /* send ICMP PARM PROB regardless and drop packet */
		icmpv6_param_prob_reason(skb, ICMPV6_UNK_OPTION, optoff,
					 SKB_DROP_REASON_UNHANDLED_PROTO);

When seg6_enabled is set, ipv6_srh_rcv() calls icmpv6_param_prob() if
segments_left >= (hdrlen >> 1). It also sends ICMPV6_TIME_EXCEED in the
hop_limit <= 1 loopback case.

icmp6_send() copies up to 1232 bytes of the original packet, starting at
the network header, into a new skb. That limit is IPV6_MIN_MTU minus the
IPv6 and ICMPv6 headers. The new skb doesn't have decrypted set, so
sk_validate_xmit_skb() doesn't drop it. It is sent in cleartext to the
attacker-chosen saddr, carrying the decrypted TCP header and payload.

Before this patch, all of these packets were dropped at the is_udp check.
Forwarding the decrypted skb itself looks blocked by
sk_validate_xmit_skb() ("unencrypted skb with no associated socket -
dropping"). That leaves the ICMP error path as the open channel.

Should psp_ipv6_nexthdr_off() be stricter about which extension headers
and options it accepts? Rejecting only routing headers with
segments_left != 0 would not cover the destination or hop-by-hop options
case. Later patches in the series don't change psp_ipv6_nexthdr_off() or
psp_dev_rcv().

[ ... ]

> @@ -390,10 +445,11 @@ int psp_dev_rcv(struct sk_buff *skb, u16 dev_id, u8 generation, bool strip_icv)
>  	} else {
>  		struct ipv6hdr *ipv6h = (struct ipv6hdr *)(skb->data + l2_hlen);
>  
> -		if (unlikely(ntohs(ipv6h->payload_len) < encap))
> +		if (unlikely(ntohs(ipv6h->payload_len) <
> +			     l3_hlen - sizeof(*ipv6h) + encap))
>  			return -EINVAL;
>  
> -		ipv6h->nexthdr = psph->nexthdr;
> +		((u8 *)ipv6h)[nh_off] = psph->nexthdr;

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch. The TX
side in psp_dev_encapsulate() still writes IPPROTO_UDP into the fixed
header:

	} else {
		ipv6_hdr(skb)->nexthdr = IPPROTO_UDP;
		be16_add_cpu(&ipv6_hdr(skb)->payload_len, PSP_ENCAP_HLEN);
	}

The UDP and PSP headers are inserted after skb_network_header_len()
bytes, which include any extension headers. A TCPv6 socket with
IPV6_HOPOPTS, IPV6_DSTOPTS or IPV6_RTHDR set would therefore emit:

  fixed(nexthdr=UDP) | EXT(nexthdr=TCP) | UDP | PSP | TCP

The new psp_dev_rcv() code expects the UDP nexthdr in the last extension
header. Here it would treat the first extension header as the UDP header
and drop the packet on the dport check.

In netdevsim, nsim_do_psp() calls psp_dev_encapsulate() and then
psp_dev_rcv(). So at this commit, the new RX path can't be tested end to
end with extension headers.

The next patch in the series, "psp: support ipv6 extension headers in
psp_dev_encapsulate()", seems to fix this. It reuses
psp_ipv6_nexthdr_off() and writes IPPROTO_UDP at nh_off. Would it make
sense to land the TX change before this one, or together with it?

>  		ipv6h->payload_len = htons(ntohs(ipv6h->payload_len) - encap);
>  	}
>

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