Re: [PATCH net v2 1/2] ipv4: recompile IP options on a private copy 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] ipv4_send_dest_unreach() (net/ipv4/route.c) now re-reads the
  IPv4 header from the private copy (nskb = pskb_copy(skb, GFP_ATOMIC))…

--

ipv4: recompile IP options on a private copy in ipv4_send_dest_unreach()

This patch changes ipv4_send_dest_unreach() so that it recompiles and
echoes the IP options on a private pskb_copy() instead of the caller's
skb. __ip_options_compile() rewrites RR/TS options in place, and the skb
may share data with clones. If the copy fails, no ICMP error is sent.

> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 3a90c8d28b940..9af72cb892034 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c

[ ... ]

> @@ -1271,6 +1272,16 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
>  	if (ip_hdr(skb)->ihl > 5) {
>  		if (!pskb_network_may_pull(skb, ip_hdr(skb)->ihl * 4))
>  			return;
> +
> +		/* __ip_options_compile() writes into the options, and
> +		 * callers of dst_link_failure() may cache pointers into
> +		 * skb->head: work on a private copy.
> +		 */
> +		nskb = pskb_copy(skb, GFP_ATOMIC);
> +		if (!nskb)
> +			return;
> +		skb = nskb;
> +
>  		parm.opt.optlen = ip_hdr(skb)->ihl * 4 - sizeof(struct iphdr);

[Severity: High]
What happens here if skb_network_offset(skb) is negative when we get
here?

__pskb_copy_fclone() reserves the headroom but copies only the linear
data that starts at skb->data:

net/core/skbuff.c:__pskb_copy_fclone() {
    ...
	skb_reserve(n, headroom);
	skb_put(n, skb_headlen(skb));
	skb_copy_from_linear_data(skb, n->data, n->len);
    ...
	skb_copy_header(n, skb);
}

skb_copy_header() keeps network_header at the same offset from head. If
the network header sits before skb->data, ip_hdr(nskb) points into
headroom that was never initialized.

The earlier checks don't catch this. pskb_network_may_pull() is
pskb_may_pull(skb, skb_network_offset(skb) + len), and that passes with
an offset of -20. The version and ihl checks ran on the original skb and
are not repeated on the copy.

One way to get a negative offset looks like this. A tc mirred (or
bpf_redirect) egress redirect of a routed IPv4 packet goes to an
NBMA/multicast ipgre device that has header_ops. That device is
ARPHRD_IPGRE, so tcf_mirred_to_dev() hands it over with data at the
network header and keeps the IPv4 dst:

tcf_mirred_to_dev()
  ipgre_xmit()             /* header_ops branch: skb_pull(hlen + 20) */
    __gre_xmit()           /* gre_build_header() pushes tun_hlen */
      ip_tunnel_xmit()     /* err_count > 0 */
        dst_link_failure()
          ipv4_link_failure()
            ipv4_send_dest_unreach()   /* skb_network_offset() == -20 */

Before this patch, the function read real header bytes from the original
skb's headroom. Now parm.opt.optlen is computed from uninitialized
memory. optlen is an unsigned char, so ihl < 5 wraps to 236..252.

Can __ip_options_compile(..., nskb, NULL) then parse and write past
nskb's tail? The writes include the IPOPT_END zero fill and the RR/TS
pointer and address writes. They could reach skb_shared_info (nr_frags,
frags[], frag_list), which pskb_copy() filled with references to the
original skb's frags.

__icmp_send() would also use the garbage saddr/daddr/tos/frag_off/
protocol. icmp_glue_bits() copies from skb_network_offset(nskb), so
uninitialized headroom bytes could go out in the ICMP payload.

Would it make sense to bail out when skb_network_offset(skb) < 0, use a
copy that includes the headroom, or copy only the IP header and options
into a private buffer?

>  
>  		rcu_read_lock();

[ ... ]

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