Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] net: use-after-free read of iph->ihl after pskb_may_pull() in ip4_parse() - [High] virtio_net: failed drop of invalid non-GSO tunnel packets in virtio_net_hdr_tnl_to_skb() --
commit 932b3459605ba3055ae9042b1894bc6eb02c03c4 Author: Michael S. Tsirkin <[email protected]> virtio_net: cleanup protocol calculation This commit removes a duplicate assignment of inner_proto in virtio_net_hdr_tnl_to_skb() by computing it once before calling skb_set_inner_protocol(). > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > index 063cc116e51a1..e984e446ba44a 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h > @@ -613,8 +613,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, > return -EINVAL; > } > > - skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) : > - htons(ETH_P_IP)); > + inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP); > + skb_set_inner_protocol(skb, inner_proto); > if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) { > if (!tnl_csum_negotiated) > return -EINVAL; > @@ -638,7 +638,6 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, > * We know VIRTIO_NET_HDR_F_NEEDS_CSUM was set so > __virtio_net_hdr_to_skb > * set CHECKSUM_PARTIAL. > */ > - inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP); > if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh, > inner_proto)) > return 0; [Severity: High] When virtio_net_csum_partial_ok_at_offset() validates an IPv4 header via virtio_net_l4_start() -> ip4_parse() (introduced earlier in this series in commit f0109d51e8b0 ("net: add ip4_parse()")), can ip4_parse() in include/linux/ip.h perform a use-after-free read of iph->ihl? In include/linux/ip.h:ip4_parse(): if (!pskb_may_pull(skb, nhoff + sizeof(struct iphdr))) return NULL; iph = (struct iphdr *)(skb->data + nhoff); if (iph->version != 4 || iph->ihl < sizeof(struct iphdr) / sizeof(u32)) return NULL; /* Pull the options as well: the transport header offset * returned below must be within the linear data. */ if (!pskb_may_pull(skb, nhoff + iph->ihl * sizeof(u32))) return NULL; *thoff = nhoff + iph->ihl * sizeof(u32); return (struct iphdr *)(skb->data + nhoff); When an IPv4 packet has options (iph->ihl > 5) that reside in nonlinear paged data (so nhoff + iph->ihl * sizeof(u32) > skb_headlen(skb)), or when the skb is cloned, the second pskb_may_pull() call invokes __pskb_pull_tail() -> pskb_expand_head(), which allocates a new skb->head buffer and frees the old buffer still pointed to by iph. Dereferencing iph->ihl to compute *thoff after the second pskb_may_pull() then reads from the freed buffer before iph is reloaded from skb->data + nhoff. This affects all callers of ip4_parse(), including ip_rcv_core(), virtio_net_l4_start(), br_validate_ipv4(), and nf_reject_iphdr_validate(). Should ip4_parse() cache iph->ihl * sizeof(u32) in a local variable before the second pskb_may_pull() call, or reload iph from skb->data + nhoff first? [Severity: High] Also in include/linux/virtio_net.h:virtio_net_hdr_tnl_to_skb(), what happens when virtio_net_csum_partial_ok_at_offset() returns false for a non-gso packet with a tunnel header? Earlier in virtio_net_hdr_tnl_to_skb(), __virtio_net_hdr_to_skb() only sets shinfo->gso_size and shinfo->gso_type when skb->len - nh_off > gso_size. If a caller provides gso_size >= skb->len - nh_off, skb_is_gso(skb) remains false, which virtio_net_hdr_tnl_to_skb() explicitly accounts for after setting the inner header offsets and skb->encapsulation: skb->inner_transport_header = inner_th + skb_headroom(skb); skb->inner_network_header = inner_nh + skb_headroom(skb); skb->inner_mac_header = inner_nh + skb_headroom(skb); skb->transport_header = outer_th + skb_headroom(skb); skb->encapsulation = 1; if (!skb_is_gso(skb)) skb_shinfo(skb)->gso_type &= ~(SKB_GSO_UDP_TUNNEL_CSUM | SKB_GSO_UDP_TUNNEL); /* * We know VIRTIO_NET_HDR_F_NEEDS_CSUM was set so __virtio_net_hdr_to_skb * set CHECKSUM_PARTIAL. */ if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh, inner_proto)) return 0; /* A tunnel header implies GSO, so drop. */ return __virtio_net_resolve_csum_partial_mismatch(skb); However, in include/linux/virtio_net.h:__virtio_net_resolve_csum_partial_mismatch(): static inline int __virtio_net_resolve_csum_partial_mismatch(struct sk_buff *skb) { if (skb_is_gso(skb)) return -EINVAL; return skb_checksum_help(skb) ?: 1; } When !skb_is_gso(skb) and virtio_net_csum_partial_ok_at_offset() fails because the inner headers are malformed or do not match inner_th, __virtio_net_resolve_csum_partial_mismatch(skb) does not drop the packet. Instead, it completes the inner checksum via skb_checksum_help(skb) and returns 1, which callers treat as success. Does this allow a non-gso tunnel packet with invalid inner headers to enter the networking stack with skb->encapsulation = 1, unvalidated inner header offsets, and any requested outer tunnel checksum (VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) lost when skb_checksum_help() sets ip_summed to CHECKSUM_NONE? Should virtio_net_hdr_tnl_to_skb() return -EINVAL directly when virtio_net_csum_partial_ok_at_offset() returns false? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
