Re: [PATCH net 1/2] ipv4: unshare headers before recompiling IP options in ipv4_send_dest_unreach()

[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] This patch adds skb_unclone(skb, GFP_ATOMIC) to
  ipv4_send_dest_unreach() (net/ipv4/route.c:1272-1274).

--

ipv4: unshare headers before recompiling IP options in ipv4_send_dest_unreach()

The patch makes the IPv4 header private with skb_unclone() before
__ip_options_compile() rewrites Record Route and Timestamp options in
place in ipv4_send_dest_unreach(). The goal is to stop concurrent
link-failure processing of clones from changing an option pointer after
it has been checked. If the copy fails, no ICMP error is sent.

> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 3a90c8d28b940..4d8fcc95c2f46 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1269,7 +1269,8 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
>  
>  	memset(&parm, 0, sizeof(parm));
>  	if (ip_hdr(skb)->ihl > 5) {
> -		if (!pskb_network_may_pull(skb, ip_hdr(skb)->ihl * 4))
> +		if (!pskb_network_may_pull(skb, ip_hdr(skb)->ihl * 4) ||
> +		    skb_unclone(skb, GFP_ATOMIC))

[Severity: High]
Can this skb_unclone() leave stale header pointers in callers of
dst_link_failure()?

ipv4_send_dest_unreach() runs as the ipv4 dst_ops->link_failure
callback. Before this patch, a header that was already linear was never
reallocated on this path, because pskb_network_may_pull() returns early.
Now, when the skb is cloned and has options (ihl > 5),
pskb_expand_head() swaps skb->head and calls skb_release_data() on the
old head, which drops this skb's reference.

ip_tunnel_xmit() in net/ipv4/ip_tunnel.c saves a raw pointer into the
head before calling dst_link_failure(), and does not reload it
afterwards:

ip_tunnel_xmit() {
    ...
	inner_iph = (const struct iphdr *)skb_inner_network_header(skb);
    ...
			WRITE_ONCE(tunnel->err_count, err_count - 1);

			dst_link_failure(skb);
    ...
	tos = ip_tunnel_ecn_encap(tos, inner_iph, skb);
	ttl = tnl_params->ttl;
	if (ttl == 0) {
		if (payload_protocol == htons(ETH_P_IP))
			ttl = inner_iph->ttl;
    ...
}

For header_ops GRE devices, ipgre_xmit() in net/ipv4/ip_gre.c also
points tnl_params into the head:

	tnl_params = (const struct iphdr *)skb->data;

The call chain is:

ip_tunnel_xmit()->dst_link_failure()->ipv4_link_failure()->
ipv4_send_dest_unreach()->skb_unclone()->pskb_expand_head()

Cloned skbs do get here. TCP transmits nohdr clones, so
skb_header_cloned() is false and skb_cow_head() in
ipgre_xmit()/gre_tap_xmit() does not unclone them, but skb_cloned() is
still true. An unprivileged user can add RR/TS options with
setsockopt(IP_OPTIONS). err_count goes up on ICMP errors from remote
hosts in ipgre_err().

After the expand, the old head stays alive only while the other clone
holder, such as the TCP rtx queue skb or a tc-mirred original, keeps its
reference. If the tunnel has a qdisc, the clone can be sent without the
socket lock, and an ACK on another CPU could free the original. That
could happen during __icmp_send(), or even inside pskb_expand_head()
between the skb_cloned() check and skb_release_data().

In that case, are the inner_iph->tos, tnl_params->ttl and inner_iph->ttl
reads use-after-free reads, with the stale bytes written into the outer
header's TOS/TTL?

Would it be safer to compile and echo the options from a private copy
rather than uncloning the caller's skb inside a link_failure callback?
Another option would be to reload inner_iph and tnl_params in
ip_tunnel_xmit() after dst_link_failure(). Either way, it seems worth
checking the other dst_link_failure() callers for the same pattern.

>  			return;
>  		parm.opt.optlen = ip_hdr(skb)->ihl * 4 - sizeof(struct iphdr);
>

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