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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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