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 <redacted>
---
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(-)
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Date: 2026-08-05 13:14:04
Asim Viladi Oglu Manizada wrote:
quoted hunk
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 <redacted>
---
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(-)
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;
From: Jakub Kicinski <kuba@kernel.org> Date: 2026-08-06 14:22:38
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.
[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."
[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.
On Wednesday, August 5th, 2026 at 6:14 AM, Willem de Bruijn [off-list ref] wrote:
Asim Viladi Oglu Manizada wrote:
quoted
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 <redacted>
---
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(-)
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;
Makes sense, I'll do that in v3, it also keeps returning -EINVAL
in the empty packet case without needing a skb->len check. Will
resubmit within 72 hours or so unless I hear any more concerns.
I'll respond to the AI review separately, those things don't look
concerning.
On Thursday, August 6th, 2026 at 7:22 AM, Jakub Kicinski [off-list ref] wrote:
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.
[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?
I don't really follow this; not for the linear-area reason described. The
virtio-header conversion calls pskb_may_pull() for the required checksum
or GSO headers before skb_partial_csum_set(). The requested pull length
includes the checksum field, so after a successful pull csum_end is within
skb_headlen(). The one-byte and ETH_HLEN values are only the minimum
prefix initially kept linear.
In configs where the 16-bit (vs the one-page) offset limit controls the
cap, an extreme max-headroom request can still leave too little room for
a non-zero checksum-start offset to fit. But that is rejected safely and
is a whole other topic.
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."
[Severity: Low]
Should this path set drop_reason too?
I'll just do what Willem suggested in v3 instead:
if (!pskb_may_pull(skb, 1)) {
err = -EINVAL;
goto drop;
}
ip_version = skb->data[0] >> 4;
So there'd be no skb->len check, so pull failure can also mean
that the packet contains no first byte, at which point SKB_DROP_REASON_NOMEM
wouldn't make sense either. So I think it's best not to set it?
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.