Re: [PATCH net v2] net: tun: bound receive headroom

Asim Viladi Oglu Manizada wrote:
> tun_get_user() uses tun->align both as skb headroom and when choosing how
> much packet data to keep linear. OVS can propagate an oversized headroom
> request from another port to TUN or TAP.
> 
> When align is larger than the usable space in a one-page skb head,
> SKB_MAX_HEAD(align) underflows and the result becomes negative when stored
> in good_linear. That value later wraps when assigned to the size_t linear
> variable, and tun_alloc_skb() can place skb->data outside the allocated
> head.
> 
> Bound the headroom stored by TUN to the one-page skb-head budget and the
> largest non-sentinel 16-bit skb header offset. Leave one linear byte for
> raw TUN and a complete Ethernet header for TAP, including NET_IP_ALIGN.
> 
> Also pull the raw-TUN protocol byte and the TAP Ethernet header before
> accessing them, so these checks remain safe for nonlinear skbs supplied by
> other allocation paths.
> 
> Fixes: eaea34b23c46 ("net/tun: implement ndo_set_rx_headroom")
> Cc: [email protected]
> Assisted-by: avom-custom-harness:gpt-5.5-qwen3.6-mod-mix
> Signed-off-by: Asim Viladi Oglu Manizada <[email protected]>
> ---
> v2:
> - bound tun->align instead of clamping good_linear to zero
> - derive the bound from the one-page head, 16-bit offset, and TUN/TAP
>   linear-header requirements
> - pull the raw-TUN protocol byte before reading it
> - make the TAP Ethernet-header pull unconditional
> v1: https://lore.kernel.org/netdev/[email protected]/
> 
>  drivers/net/tun.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index fed9dfdfcc3b..efd2e7d75c9a 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1107,11 +1107,16 @@ static netdev_features_t tun_net_fix_features(struct net_device *dev,
>  static void tun_set_headroom(struct net_device *dev, int new_hr)
>  {
>  	struct tun_struct *tun = netdev_priv(dev);
> +	size_t max_headroom;
>  
> -	if (new_hr < NET_SKB_PAD)
> -		new_hr = NET_SKB_PAD;
> +	max_headroom = min_t(size_t, SKB_MAX_HEAD(0), U16_MAX - 1);
>  
> -	tun->align = new_hr;
> +	if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
> +		max_headroom -= ETH_HLEN + NET_IP_ALIGN;
> +	else
> +		max_headroom -= 1;
> +
> +	tun->align = clamp_t(int, new_hr, NET_SKB_PAD, max_headroom);
>  }
>  
>  static void
> @@ -1822,7 +1827,13 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  	switch (tun->flags & TUN_TYPE_MASK) {
>  	case IFF_TUN:
>  		if (tun->flags & IFF_NO_PI) {
> -			u8 ip_version = skb->len ? (skb->data[0] >> 4) : 0;
> +			u8 ip_version;
> +
> +			if (skb->len && !pskb_may_pull(skb, 1)) {
> +				err = -ENOMEM;
> +				goto drop;
> +			}
> +			ip_version = skb->len ? (skb->data[0] >> 4) : 0;

Overall, LGTM, thanks. Let's wait for the bots too.

This can be a bit simpler. pskb_may_pull checks that len < skb->len.
And it is not an allocation failure, but a bad packet.

	if (!pskb_may_pull(skb, 1)) {
		err = -EINVAL;
		goto drop;
	}

	ip_version = skb->data[0] >> 4;