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

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

[Asim Viladi Oglu Manizada]

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: stable@vger.kernel.org
Assisted-by: avom-custom-harness:gpt-5.5-qwen3.6-mod-mix
Signed-off-by: Asim Viladi Oglu Manizada <manizada@pm.me>
---
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/20260721014117.2234892-1-manizada@pm.me/

 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;
 
 			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;
-- 
2.53.0

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

[Willem de Bruijn]

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: stable@vger.kernel.org
> Assisted-by: avom-custom-harness:gpt-5.5-qwen3.6-mod-mix
> Signed-off-by: Asim Viladi Oglu Manizada <manizada@pm.me>
> ---
> 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/20260721014117.2234892-1-manizada@pm.me/
> 
>  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;

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

[Jakub Kicinski]

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