On Wed, Feb 17, 2021 at 6:18 PM Jason A. Donenfeld <ja...@zx2c4.com> wrote: > > On 2/18/21, Willem de Bruijn <willemdebruijn.ker...@gmail.com> wrote: > > On Wed, Feb 17, 2021 at 5:56 PM Jason A. Donenfeld <ja...@zx2c4.com> wrote: > >> > >> Hi Willem, > >> > >> On Wed, Feb 17, 2021 at 11:27 PM Willem de Bruijn > >> <willemdebruijn.ker...@gmail.com> wrote: > >> > A vmlinux image might help. I couldn't find one for this kernel. > >> > >> https://data.zx2c4.com/icmp_send-crash-e03b4a42-706a-43bf-bc40-1f15966b3216.tar.xz > >> has .debs with vmlinuz in there, which you can extract to vmlinux, as > >> well as my own vmlinux elf construction with the symbols added back in > >> by extracting them from kallsyms. That's the best I've been able to > >> do, as all of this is coming from somebody random emailing me. > >> > >> > But could it be > >> > that the forwarded packet is not sensible IPv4? The skb->protocol is > >> > inferred in wg_packet_consume_data_done->ip_tunnel_parse_protocol. > >> > >> The wg calls to icmp_ndo_send are gated by checking skb->protocol: > >> > >> if (skb->protocol == htons(ETH_P_IP)) > >> icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_UNREACH, > >> 0); > >> else if (skb->protocol == htons(ETH_P_IPV6)) > >> icmpv6_ndo_send(skb, ICMPV6_DEST_UNREACH, > >> ICMPV6_ADDR_UNREACH, 0); > >> > >> On the other hand, that code is hit on an error path when > >> wg_check_packet_protocol returns false: > >> > >> static inline bool wg_check_packet_protocol(struct sk_buff *skb) > >> { > >> __be16 real_protocol = ip_tunnel_parse_protocol(skb); > >> return real_protocol && skb->protocol == real_protocol; > >> } > >> > >> So that means, at least in theory, icmp_ndo_send could be called with > >> skb->protocol != ip_tunnel_parse_protocol(skb). I guess I can address > >> that. But... is it actually a problem? > > > > For this forwarded packet that arrived on a wireguard tunnel, > > skb->protocol was originally also set by ip_tunnel_parse_protocol. > > So likely not. > > > > The other issue seems more like a real bug. wg_xmit calling > > icmp_ndo_send without clearing IPCB first. > > > > Bingo! Nice eye! I confirmed the crash by just memsetting 0x41 to cb > before the call. Clearly this should be zeroed by icmp_ndo_xmit. Will > send a patch for icmp_ndo_xmit momentarily and will CC you.
Great, let's hope that's it. gtp_build_skb_ip4 zeroes before calling. The fix will be most obviously correct if wg_xmit does the same. But it is quite likely that the other callers, xfrmi_xmit2 and sunvnet_start_xmit_common should zero, too. If so, then icmp_ndo_xmit is the more robust location to do this. Then the Fixes tag will likely go quite a bit farther back, too. Whichever variant of the patch you prefer.
