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

[netdev-bot+sashiko@kernel.org]

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