[PATCH] tun: fix skb length underflow in NAPI frags path

[PATCH] tun: fix skb length underflow in NAPI frags path

[Harshit Mogalapalli]

When experimental vhost-net zero-copy TX is used with an IFF_NAPI_FRAGS
TAP backend, tun_get_user() receives msg_control, selects zerocopy, and
limits copylen to the linear prefix.

The NAPI frags path subsequently disables zerocopy after allocating the
skb and copies the complete frame instead. tun_napi_alloc_frags() derives
the linear length from the first iterator segment, so a segment larger
than copylen makes skb->data_len underflow. Pulling the Ethernet header
then triggers BUG() because skb->len is smaller than skb->data_len.

Disable zerocopy before calculating the allocation length so this path
allocates and copies the complete iterator.

Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
 drivers/net/tun.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 5a302709a68a..1124b8b33664 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1848,6 +1848,10 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 			zerocopy = true;
 	}
 
+	/* The NAPI frags path copies the complete iterator. */
+	if (frags)
+		zerocopy = false;
+
 	if (!frags && tun_can_build_skb(tun, tfile, len, noblock, zerocopy)) {
 		/* For the packet that is not easy to be processed
 		 * (e.g gso or jumbo packet), we will do it at after
@@ -1868,11 +1872,6 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		if (frags) {
 			mutex_lock(&tfile->napi_mutex);
 			skb = tun_napi_alloc_frags(tfile, copylen, from);
-			/* tun_napi_alloc_frags() enforces a layout for the skb.
-			 * If zerocopy is enabled, then this layout will be
-			 * overwritten by zerocopy_sg_from_iter().
-			 */
-			zerocopy = false;
 		} else {
 			if (!linear)
 				linear = min_t(size_t, good_linear, copylen);
-- 
2.52.0

Re: [PATCH] tun: fix skb length underflow in NAPI frags path

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] tun: fix skb length underflow in NAPI frags path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This fix is incomplete.

Pre-existing issues:
- [High] tun_napi_alloc_frags() builds fragments from `iter_iov(it) + i`
  and the raw `iov->iov_len` (drivers/net/tun.c:1505-1506).
- [Low] In tun_get_user(), the `if (msg_control)` block
  (drivers/net/tun.c:1837-1849) still computes copylen/linear and walks…

--

tun: fix skb length underflow in NAPI frags path

This patch moves the clearing of zerocopy for the IFF_NAPI_FRAGS path in
tun_get_user() so it happens before copylen is computed. The NAPI frags
allocation is then sized for the complete iterator instead of the zerocopy
linear prefix. This avoids a skb->data_len underflow and a BUG() in
__skb_pull().

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 5a302709a68aa..1124b8b33664e 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1848,6 +1848,10 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  			zerocopy = true;
>  	}
>  
> +	/* The NAPI frags path copies the complete iterator. */
> +	if (frags)
> +		zerocopy = false;
> +

[Severity: Low]
This isn't a bug introduced by this patch, but with this override in
place, the msg_control block just above in tun_get_user() has no useful
effect when frags is set:

drivers/net/tun.c:tun_get_user() {
    ...
	if (msg_control) {
		struct iov_iter i = *from;

		/* There are 256 bytes to be copied in skb, so there is
		 * enough room for skb expand head in case it is used.
		 * The rest of the buffer is mapped from userspace.
		 */
		copylen = min(hdr_len ? hdr_len : GOODCOPY_LEN, good_linear);
		linear = copylen;
		iov_iter_advance(&i, copylen);
		if (iov_iter_npages(&i, INT_MAX) <= MAX_SKB_FRAGS)
			zerocopy = true;
	}
    ...
}

For frags, the !zerocopy branch overwrites copylen and linear, and the
result of the iov_iter_npages() walk is discarded.

The comment doesn't describe the frags path either. It also says 256
bytes, but GOODCOPY_LEN is defined as 128.

Would gating the block with msg_control && !frags make the code and the
comment agree, and also skip the extra iov_iter_npages() walk?

>  	if (!frags && tun_can_build_skb(tun, tfile, len, noblock, zerocopy)) {
>  		/* For the packet that is not easy to be processed
>  		 * (e.g gso or jumbo packet), we will do it at after
> @@ -1868,11 +1872,6 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  		if (frags) {
>  			mutex_lock(&tfile->napi_mutex);
>  			skb = tun_napi_alloc_frags(tfile, copylen, from);

[Severity: High]
Does this fully close the underflow described in the commit message?

copylen is an int, but len is a size_t taken from iov_iter_count(). The
frags path now always goes through this assignment:

drivers/net/tun.c:tun_get_user() {
    ...
		if (!zerocopy) {
			copylen = len;
			linear = min(hdr_len, good_linear);
		}
    ...
}

tun_napi_alloc_frags() checks the truncated value against ETH_MAX_MTU. It
then takes linear from the first segment, which is not truncated:

drivers/net/tun.c:tun_napi_alloc_frags() {
    ...
	linear = iov_iter_single_seg_count(it);
	err = __skb_grow(skb, linear);
	if (err)
		goto free;

	skb->len = len;
	skb->data_len = len - linear;
    ...
}

So when len is over 4 GiB, the first segment can still be larger than the
allocation length.

vhost-net appears able to deliver such an iterator to an IFF_NAPI_FRAGS
TAP backend. init_iov_iter() adds the guest descriptor lengths into a
size_t, and get_tx_bufs() only rejects a length of 0. The default copy
mode then calls handle_tx_copy()->tun_sendmsg()->tun_get_user() with
msg_control = NULL.

For example, this needs a guest memslot of at least 4 GiB. The guest
builds a chain whose first iovec is about 0xfffffff3 bytes after the
vnet header, followed by a 272 byte iovec. That gives len = 0x100000103
and copylen = 259:

- __skb_grow() with diff = 0xfffffff3 passes a negative ntail to
  pskb_expand_head(), so it makes a small allocation instead of failing
- skb->len = 259 and skb->data_len = 272
- skb_copy_datagram_from_iter() and pskb_may_pull(ETH_HLEN) both succeed
- eth_type_trans()->__skb_pull() sees skb->len < skb->data_len and
  calls BUG()

Copy mode already reached copylen = len before this patch. With this
change, the msg_control case reaches it too.

Would it make sense to pass the size_t len directly to
tun_napi_alloc_frags(), or to reject lengths that don't fit in an int
before narrowing?

[Severity: High]
This is a pre-existing issue, but tun_napi_alloc_frags(), which this fix
relies on, assumes the iterator is ITER_IOVEC. It also doesn't clamp the
last fragment to the remaining count:

drivers/net/tun.c:tun_napi_alloc_frags() {
    ...
	for (i = 1; i < it->nr_segs; i++) {
		const struct iovec *iov = iter_iov(it) + i;
		size_t fragsz = iov->iov_len;
    ...
}

io_uring IORING_OP_WRITE_FIXED on a tun fd reaches tun_get_user() through
tun_chr_write_iter() with an ITER_BVEC iterator. io_import_fixed() sets
the count to len, but the last bvec keeps its full bv_len:

io_uring/rsrc.c:io_import_fixed() {
    ...
	nr_segs = (offset + len + bvec->bv_offset + folio_mask) >> imu->folio_shift;
	iov_iter_bvec(iter, ddir, bvec, nr_segs, len);
    ...
}

In this case iter_iov() returns the bio_vec array cast to struct iovec.
On 64-bit little-endian, iov_len reads as bv_len | (u64)bv_offset << 32.
For page-aligned bvecs that is just bv_len.

Take a 5000 byte write from a page-aligned registered buffer. linear is
about 4084 and data_len is about 904, but the second fragment is given a
size of 4096. skb_copy_datagram_from_iter() writes only 904 bytes into it.

Can this leave about 3192 bytes of uninitialized netdev_alloc_frag()
memory inside the fragment? The fragment sizes would then add up to more
than skb->data_len.

If so, anything that later walks the frags by skb_frag_size() could expose
that memory.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005054224.4154727-1-harshit.m.mogalapalli%40oracle.com

Re: [PATCH] tun: fix skb length underflow in NAPI frags path

[Willem de Bruijn]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0 (1)
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] This fix is incomplete.
> 
> Pre-existing issues:
> - [High] tun_napi_alloc_frags() builds fragments from `iter_iov(it) + i`
>   and the raw `iov->iov_len` (drivers/net/tun.c:1505-1506).
> - [Low] In tun_get_user(), the `if (msg_control)` block
>   (drivers/net/tun.c:1837-1849) still computes copylen/linear and walks…
> 
> --
> 
> tun: fix skb length underflow in NAPI frags path
> 
> This patch moves the clearing of zerocopy for the IFF_NAPI_FRAGS path in
> tun_get_user() so it happens before copylen is computed. The NAPI frags
> allocation is then sized for the complete iterator instead of the zerocopy
> linear prefix. This avoids a skb->data_len underflow and a BUG() in
> __skb_pull().
> 
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 5a302709a68aa..1124b8b33664e 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1848,6 +1848,10 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> >  			zerocopy = true;
> >  	}
> >  
> > +	/* The NAPI frags path copies the complete iterator. */
> > +	if (frags)
> > +		zerocopy = false;
> > +
> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but with this override in
> place, the msg_control block just above in tun_get_user() has no useful
> effect when frags is set:
> 
> drivers/net/tun.c:tun_get_user() {
>     ...
> 	if (msg_control) {
> 		struct iov_iter i = *from;
> 
> 		/* There are 256 bytes to be copied in skb, so there is
> 		 * enough room for skb expand head in case it is used.
> 		 * The rest of the buffer is mapped from userspace.
> 		 */
> 		copylen = min(hdr_len ? hdr_len : GOODCOPY_LEN, good_linear);
> 		linear = copylen;
> 		iov_iter_advance(&i, copylen);
> 		if (iov_iter_npages(&i, INT_MAX) <= MAX_SKB_FRAGS)
> 			zerocopy = true;
> 	}
>     ...
> }
> 
> For frags, the !zerocopy branch overwrites copylen and linear, and the
> result of the iov_iter_npages() walk is discarded.
> 
> The comment doesn't describe the frags path either. It also says 256
> bytes, but GOODCOPY_LEN is defined as 128.
> 
> Would gating the block with msg_control && !frags make the code and the
> comment agree, and also skip the extra iov_iter_npages() walk?

This is nice suggestion.

> 
> >  	if (!frags && tun_can_build_skb(tun, tfile, len, noblock, zerocopy)) {
> >  		/* For the packet that is not easy to be processed
> >  		 * (e.g gso or jumbo packet), we will do it at after
> > @@ -1868,11 +1872,6 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> >  		if (frags) {
> >  			mutex_lock(&tfile->napi_mutex);
> >  			skb = tun_napi_alloc_frags(tfile, copylen, from);
> 
> [Severity: High]
> Does this fully close the underflow described in the commit message?
> 
> copylen is an int, but len is a size_t taken from iov_iter_count(). The
> frags path now always goes through this assignment:
> 
> drivers/net/tun.c:tun_get_user() {
>     ...
> 		if (!zerocopy) {
> 			copylen = len;
> 			linear = min(hdr_len, good_linear);
> 		}
>     ...
> }
> 
> tun_napi_alloc_frags() checks the truncated value against ETH_MAX_MTU. It
> then takes linear from the first segment, which is not truncated:
> 
> drivers/net/tun.c:tun_napi_alloc_frags() {
>     ...
> 	linear = iov_iter_single_seg_count(it);
> 	err = __skb_grow(skb, linear);
> 	if (err)
> 		goto free;
> 
> 	skb->len = len;
> 	skb->data_len = len - linear;
>     ...
> }
> 
> So when len is over 4 GiB, the first segment can still be larger than the

This is not a normal length. Not sure whether it is feasible (quite
plausible), but would warrant a separate patch, not included here.

> allocation length.
> 
> vhost-net appears able to deliver such an iterator to an IFF_NAPI_FRAGS
> TAP backend. init_iov_iter() adds the guest descriptor lengths into a
> size_t, and get_tx_bufs() only rejects a length of 0. The default copy
> mode then calls handle_tx_copy()->tun_sendmsg()->tun_get_user() with
> msg_control = NULL.
> 
> For example, this needs a guest memslot of at least 4 GiB. The guest
> builds a chain whose first iovec is about 0xfffffff3 bytes after the
> vnet header, followed by a 272 byte iovec. That gives len = 0x100000103
> and copylen = 259:
> 
> - __skb_grow() with diff = 0xfffffff3 passes a negative ntail to
>   pskb_expand_head(), so it makes a small allocation instead of failing
> - skb->len = 259 and skb->data_len = 272
> - skb_copy_datagram_from_iter() and pskb_may_pull(ETH_HLEN) both succeed
> - eth_type_trans()->__skb_pull() sees skb->len < skb->data_len and
>   calls BUG()
> 
> Copy mode already reached copylen = len before this patch. With this
> change, the msg_control case reaches it too.
> 
> Would it make sense to pass the size_t len directly to
> tun_napi_alloc_frags(), or to reject lengths that don't fit in an int
> before narrowing?
> 
> [Severity: High]
> This is a pre-existing issue, but tun_napi_alloc_frags(), which this fix
> relies on, assumes the iterator is ITER_IOVEC. It also doesn't clamp the
> last fragment to the remaining count:
> 
> drivers/net/tun.c:tun_napi_alloc_frags() {
>     ...
> 	for (i = 1; i < it->nr_segs; i++) {
> 		const struct iovec *iov = iter_iov(it) + i;
> 		size_t fragsz = iov->iov_len;
>     ...
> }
> 
> io_uring IORING_OP_WRITE_FIXED on a tun fd reaches tun_get_user() through

If this is a bug only reachable by io_uring x napi_frags, we should
just disable the combination of these two.

napi_frags was added as a way to test insertion of packets with
specific segment layout. No need to be able to do that from io_uring.

> tun_chr_write_iter() with an ITER_BVEC iterator. io_import_fixed() sets
> the count to len, but the last bvec keeps its full bv_len:
> 
> io_uring/rsrc.c:io_import_fixed() {
>     ...
> 	nr_segs = (offset + len + bvec->bv_offset + folio_mask) >> imu->folio_shift;
> 	iov_iter_bvec(iter, ddir, bvec, nr_segs, len);
>     ...
> }
> 
> In this case iter_iov() returns the bio_vec array cast to struct iovec.
> On 64-bit little-endian, iov_len reads as bv_len | (u64)bv_offset << 32.
> For page-aligned bvecs that is just bv_len.
> 
> Take a 5000 byte write from a page-aligned registered buffer. linear is
> about 4084 and data_len is about 904, but the second fragment is given a
> size of 4096. skb_copy_datagram_from_iter() writes only 904 bytes into it.
> 
> Can this leave about 3192 bytes of uninitialized netdev_alloc_frag()
> memory inside the fragment? The fragment sizes would then add up to more
> than skb->data_len.
> 
> If so, anything that later walks the frags by skb_frag_size() could expose
> that memory.
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005054224.4154727-1-harshit.m.mogalapalli%40oracle.com