Re: [PATCH net] net: icmp: avoid invalid transport header access in icmp_send tracepoint

[David Ahern <dsahern@kernel.org>]

On 8/25/26 2:45 AM, Eric Dumazet wrote:
> syzbot reported a WARNING triggered by DEBUG_NET_WARN_ON_ONCE():
> 
>  WARNING: at skb_transport_header include/linux/skbuff.h:3087 [inline]
>  WARNING: at udp_hdr include/linux/udp.h:23 [inline]
>  WARNING: at do_trace_event_raw_event_icmp_send include/trace/events/icmp.h:30 [inline]
>  WARNING: at trace_event_raw_event_icmp_send+0x48c/0x6ec include/trace/events/icmp.h:11
>  Call trace:
>   skb_transport_header include/linux/skbuff.h:3087 [inline]
>   udp_hdr include/linux/udp.h:23 [inline]
>   do_trace_event_raw_event_icmp_send include/trace/events/icmp.h:30 [inline]
>   trace_event_raw_event_icmp_send+0x48c/0x6ec include/trace/events/icmp.h:11
>   __traceiter_icmp_send include/trace/events/icmp.h:11 [inline]
>   __do_trace_icmp_send include/trace/events/icmp.h:11 [inline]
>   trace_icmp_send+0x320/0x49c include/trace/events/icmp.h:11
>   __icmp_send+0xcfc/0x11d8 net/ipv4/icmp.c:1013
>   ipv4_send_dest_unreach net/ipv4/route.c:1280 [inline]
>   ipv4_link_failure+0x57c/0x8dc net/ipv4/route.c:1287
>   dst_link_failure include/net/dst.h:438 [inline]
>   vti_tunnel_xmit+0xe40/0x17a4 net/ipv4/ip_vti.c:307
> 
> TP_fast_assign() unconditionally calls udp_hdr(skb) before checking
> whether the packet is UDP. Furthermore, __icmp_send() can be invoked
> from paths (e.g., link failures, ARP errors, forwarding, AF_PACKET)
> where skb->transport_header was never initialized (~0U).
> 
> Under CONFIG_DEBUG_NET=y, calling skb_transport_header(skb) triggers
> DEBUG_NET_WARN_ON_ONCE(!skb_transport_header_was_set(skb)).
> 
> Fix this by:
> 1. Only parsing transport info when iph->protocol == IPPROTO_UDP.
> 2. Using skb_header_pointer() at skb_network_offset(skb) + (iph->ihl << 2)
>    to safely fetch the UDP header without assuming transport_header is set.
> 
> Fixes: db3efdcf70c7 ("net/ipv4: add tracepoint for icmp_send")
> Reported-by: syzbot+6d2762674103618994b0@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/6a8d5538.91706f20.ef82.0009.GAE@google.com/T/#u
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Peilin He <he.peilin@zte.com.cn>
> Cc: xu xin <xu.xin16@zte.com.cn>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> ---
>  include/trace/events/icmp.h | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/include/trace/events/icmp.h b/include/trace/events/icmp.h
> index 09ae115099dfe25616b6c74d5b92be1af4acff55..6937b778ae54dbf4442b127957a017da3551aaff 100644
> --- a/include/trace/events/icmp.h
> +++ b/include/trace/events/icmp.h
> @@ -27,17 +27,20 @@ TRACE_EVENT(icmp_send,
>  
>  		TP_fast_assign(
>  			struct iphdr *iph = ip_hdr(skb);
> -			struct udphdr *uh = udp_hdr(skb);
> -			int proto_4 = iph->protocol;
> +			struct udphdr _uh, *uh = NULL;
>  			__be32 *p32;
>  
>  			__entry->skbaddr = skb;
>  			__entry->type = type;
>  			__entry->code = code;
>  
> -			if (proto_4 != IPPROTO_UDP || (u8 *)uh < skb->head ||
> -				(u8 *)uh + sizeof(struct udphdr)
> -				> skb_tail_pointer(skb)) {
> +			if (iph->protocol == IPPROTO_UDP)
> +				uh = skb_header_pointer(skb,
> +							skb_network_offset(skb) +
> +							(iph->ihl << 2),
> +							sizeof(_uh), &_uh);
> +
> +			if (!uh) {
>  				__entry->sport = 0;
>  				__entry->dport = 0;
>  				__entry->ulen = 0;

This code change looks fine, so:
Reviewed-by: David Ahern <dsahern@kernel.org>


But really, why not drop the tracepoint entirely and have the author of
the original patch send a new version that acknowledges icmp's are sent
for other transport protocols?