Re: [PATCH net v2 1/2] ipv4: recompile IP options on a private copy in ipv4_send_dest_unreach()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] ipv4: skb->head reallocation and ihl TOCTOU in
  ipv4_send_dest_unreach()
- [High] ipv4: uninitialized headroom access after pskb_copy() when
  network_offset < 0
--

commit 5a814100bdfbbe07fb37410d0b393cb7deb2e38a
Author: Eric Dumazet <edumazet@kernel.org>

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

This commit uses pskb_copy() in ipv4_send_dest_unreach() so IP options are
recompiled and echoed on a private skb copy rather than modifying the
caller's skb.

> 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
> @@ -1256,6 +1256,7 @@ EXPORT_INDIRECT_CALLABLE(ipv4_dst_check);
>  
>  static void ipv4_send_dest_unreach(struct sk_buff *skb)
>  {
> +	struct sk_buff *nskb = NULL;
>  	struct inet_skb_parm parm;
>  	struct net_device *dev;
>  	int res;
> @@ -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;

[Severity: High]
Can calling pskb_network_may_pull() on the caller's skb before pskb_copy()
still reallocate skb->head and cause a use-after-free in callers of
dst_link_failure()?

Tunnel transmit paths such as ipip_tunnel_xmit() and ipgre_xmit() call
pskb_inet_may_pull(), which only pulls sizeof(struct iphdr) and leaves IPv4
options in nonlinear fragments.  When ip_tunnel_xmit() calls
dst_link_failure(skb) before skb_cow_head(), pskb_network_may_pull(skb,
ip_hdr(skb)->ihl * 4) pulls the option bytes via __pskb_pull_tail() ->
pskb_expand_head() on the caller's skb (and pskb_copy() itself can call
skb_orphan_frags() -> skb_unclone() on a cloned skb with zerocopy frags),
freeing the old skb->head that ip_tunnel_xmit()'s cached inner_iph and
tnl_params pointers still reference.

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

[Severity: High]
Does re-reading ip_hdr(skb)->ihl after pskb_copy() also allow a TOCTOU race
when skb->head is shared, or an uninitialized read when
skb_network_offset(skb) is negative?

If a concurrent clone modifies ip_hdr(skb)->ihl between the check on the
original skb and pskb_copy(), re-reading ip_hdr(nskb)->ihl without
re-validating it can underflow parm.opt.optlen or exceed the pulled length
in nskb, leading to out-of-bounds accesses in __ip_options_compile() and
__ip_options_echo().

Separately, when a packet is redirected (for example via act_mirred or
bpf_redirect) to an NBMA GRE device with header_ops set, ipgre_xmit() pulls
pull_len bytes before gre_build_header() pushes tun_hlen, entering
ip_tunnel_xmit() -> dst_link_failure() with a negative
skb_network_offset(skb).  pskb_network_may_pull() accepts negative network
offsets, but pskb_copy() only copies data starting at skb->data and leaves
the headroom [nskb->head .. nskb->data] uninitialized while preserving
nskb->network_header in that headroom.  ip_hdr(nskb) then points into
uninitialized heap memory, corrupting parm.opt.optlen and causing
out-of-bounds reads and writes in __ip_options_compile().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109131736.3060756-1-edumazet@kernel.org?part=1