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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "This is what the bots think :)"
---
net: tun: bound receive headroom

This patch clamps the headroom that tun_set_headroom() stores in tun->align
so that SKB_MAX_HEAD(align) in tun_get_user() cannot underflow, and it adds
pskb_may_pull() calls before reading the raw-TUN first byte and the TAP
Ethernet header.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index fed9dfdfcc3bc..efd2e7d75c9ac 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);
>  }

[Severity: Low]
Is the upper bound intentionally this tight?  At the clamp maximum,
tun_get_user() computes:

	good_linear = SKB_MAX_HEAD(align);

which evaluates to 1 for raw TUN and ETH_HLEN for TAP.  The non-zerocopy
path then does:

	copylen = len;
	linear = min(hdr_len, good_linear);

so the resulting skb has skb_headlen() of 1 (or 14) bytes.

Does that make every write() carrying a virtio_net_hdr with
VIRTIO_NET_HDR_F_NEEDS_CSUM or a GSO type fail?  The chain
tun_vnet_hdr_tnl_to_skb() -> virtio_net_hdr_to_skb() ends in
skb_partial_csum_set(), which requires the csum offsets to live inside the
linear area:

net/core/skbuff.c:skb_partial_csum_set() {
	...
	if (unlikely(csum_start >= U16_MAX || csum_end > skb_headlen(skb))) {
		net_warn_ratelimited("bad partial csum: ...");
		return false;
	}
	...
}

With a 1-byte or 14-byte headlen that check always fails, so tun_get_user()
takes the err = -EINVAL path, bumps rx_frame_errors and drops the frame for
all checksum-offloaded and GSO traffic while the headroom request is in
effect.

Note that for IFF_TUN, tun_can_build_skb() returns false, so the raw-TUN
path always goes through tun_alloc_skb() and always sees good_linear == 1 at
the bound.  tun_alloc_skb() only overrides linear when prepad + len <
PAGE_SIZE, which is not the case for non-trivial packets at this prepad.

Would reserving a usable linear area instead (for example GOODCOPY_LEN, or
the maximum hdr_len the device can accept) give the same memory-safety
guarantee without turning offloaded traffic into drops?  For a Cc: stable
patch, could the commit message also mention this consequence?  Right now it
only says:

  "Leave one linear byte for raw TUN and a complete Ethernet header for TAP,
   including NET_IP_ALIGN."

> @@ -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;
> +			}

[Severity: Low]
Should this path set drop_reason too?  It jumps to drop without touching
drop_reason, so the free_skb label in tun_get_user() reports the initial
SKB_DROP_REASON_NOT_SPECIFIED:

drivers/net/tun.c:tun_get_user() {
	...
free_skb:
	if (!IS_ERR_OR_NULL(skb))
		kfree_skb_reason(skb, drop_reason);
	...
}

Since the call is guarded by skb->len, pskb_may_pull(skb, 1) can only fail
via the GFP_ATOMIC allocation inside __pskb_pull_tail()/pskb_expand_head(),
so SKB_DROP_REASON_NOMEM looks like the accurate value here.

The IFF_TAP arm changed by the same hunk does set an explicit reason for its
pull failure, so the two adjacent sites in the same switch end up reporting
different things to the kfree_skb_reason() tracepoint.

> +			ip_version = skb->len ? (skb->data[0] >> 4) : 0;
>  
>  			switch (ip_version) {
>  			case 4:

[ ... ]

> @@ -1842,7 +1853,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  		skb->dev = tun->dev;
>  		break;
>  	case IFF_TAP:
> -		if (frags && !pskb_may_pull(skb, ETH_HLEN)) {
> +		if (!pskb_may_pull(skb, ETH_HLEN)) {
>  			err = -ENOMEM;
>  			drop_reason = SKB_DROP_REASON_HDR_TRUNC;
>  			goto drop;
-- 
pw-bot: cr