Re: [PATCH net v5 7/7] virtio_net: cleanup protocol calculation

[netdev-bot+sashiko]

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] net: use-after-free read of iph->ihl after pskb_may_pull() in
  ip4_parse()

--

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -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]
This isn't introduced by this patch, but since inner_proto is what
steers virtio_net_csum_partial_ok_at_offset() into the IPv4 parser,
it's worth raising here.

virtio_net_csum_partial_ok_at_offset() calls virtio_net_l4_start(),
which for ETH_P_IP calls ip4_parse(). In ip4_parse() the fixed header
is pulled, iph is set to skb->data + nhoff, and then a second
pskb_may_pull(skb, nhoff + iph->ihl * sizeof(u32)) pulls in the
options. That second pull can go through __pskb_pull_tail() and
pskb_expand_head() when the options sit in paged frags, the skb is
cloned, or CONFIG_FAIL_SKB_REALLOC fires, and that frees the old
skb->head. The next line then computes *thoff from iph->ihl through
the stale pointer, before the header is reloaded from skb->data.

That is a use-after-free read that a guest, or a tun/tap/af_packet
user, can trigger with an IPv4 header whose options straddle the
linear area. If the freed buffer has been reused, we can also end up
with a bogus *thoff that is then checked against csum_start. Since
ip4_parse() is shared with ip_rcv_core(), br_validate_ipv4() and
nf_reject_iphdr_validate(), plain IPv4 ingress hits it too.

Reading ihl into a local before the second pskb_may_pull(), or
reloading iph from skb->data right after it, should be enough.
Do you want to fold a fix for ip4_parse() into this series, or send
it separately to net with a Fixes: tag for the commit that added
ip4_parse()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.v5.git.mst%40redhat.com