Re: [PATCH net] tun: prevent underflow in rx headroom calculation

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
tun: prevent underflow in rx headroom calculation

This patch clamps good_linear to zero in tun_get_user() so that a large
tun->align value coming from ndo_set_rx_headroom() does not cause
SKB_MAX_HEAD(align) to underflow into a huge size_t when assigned to the
linear variable, and later cause the skb data pointer to end up outside
the allocated head.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index ffbe6f13fb1f..d3be0f2d5df8 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1780,6 +1780,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  	}
>  
>  	good_linear = SKB_MAX_HEAD(align);
> +	good_linear = max(good_linear, 0);

[High]
Does this fully cover the IFF_TAP path when align is large enough that
SKB_MAX_HEAD(align) is <= 0?

With the clamp, good_linear becomes 0, and later in tun_get_user() the
non-zerocopy path does:

    copylen = len;
    linear = min(hdr_len, good_linear);
    ...
    if (!linear)
        linear = min_t(size_t, good_linear, copylen);
    skb = tun_alloc_skb(tfile, align, copylen, linear, noblock);

so linear ends up 0 and the resulting skb has skb_headlen(skb) == 0.

For IFF_TAP without IFF_NAPI_FRAGS the code then calls:

    skb->protocol = eth_type_trans(skb, tun->dev);

eth_type_trans() -> eth_skb_pull_mac() -> __skb_pull(skb, ETH_HLEN),
and __skb_pull() has:

    if (unlikely(skb->len < skb->data_len))
        BUG();

Since skb->len and skb->data_len both start at the packet length and
skb->len is decremented by ETH_HLEN in the pull, would this reach the
BUG() when a user writes to /dev/net/tun with tun->align pushed above
the SKB_MAX_HEAD threshold via the OVS/netkit/VXLAN stacking described
in the commit message?

The pre-existing behavior at align == 3776 appears to hit the same
BUG() and align > 3776 pre-patch corrupted memory before reaching it,
so this change is an improvement, but would forcing linear >= ETH_HLEN
in the IFF_TAP path (or calling pskb_may_pull() before eth_type_trans()
regardless of the frags flag) be needed to close the remaining panic?
-- 
pw-bot: cr