Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] net: info leak via out-of-bounds stack read in __tun_vnet_hdr_put() - [High] net: truncation of extended virtio-net headers breaks offloads in __tun_vnet_hdr_get() - [Medium] net: incorrect endianness when storing virtio-net timestamp in tun_put_user() --
commit f6ec01e41ac769b584380d489021709a59d0a180 Author: Steffen Trumtrar <[email protected]> tun: support rx-tstamp Demonstrate support for new virtio-net features VIRTIO_NET_HDR_F_TSTAMP [ ... ] > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 5a302709a68aa..924784771b6b7 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -2157,12 +2157,23 @@ static ssize_t tun_put_user(struct tun_struct *tun, > } > > if (vnet_hdr_sz) { > - struct virtio_net_hdr_v1_hash_tunnel hdr; > - struct virtio_net_hdr *gso; > + struct virtio_net_common_hdr hdr; > > memset(&hdr, 0, sizeof(hdr)); > + > + /* hdr has at least the size up to and including the tstamp > field. */ > + if (vnet_hdr_sz >= offsetof(struct virtio_net_common_hdr, > tstamp) + > + sizeof_field(struct virtio_net_common_hdr, > tstamp)) { > + u64 tstamp = ktime_get_clocktai_ns(); > + > + hdr.tstamp[0] = (tstamp & 0x000000000000ffffULL) >> 0; > + hdr.tstamp[1] = (tstamp & 0x00000000ffff0000ULL) >> 16; > + hdr.tstamp[2] = (tstamp & 0x0000ffff00000000ULL) >> 32; > + hdr.tstamp[3] = (tstamp & 0xffff000000000000ULL) >> 48; [Severity: Medium] Since tstamp is defined as a __virtio16 array, does this need to use cpu_to_tun_vnet16() for byte swapping? Assigning a host-endian value directly using native 16-bit shifts could corrupt the timestamp when read by the guest if the negotiated virtio endianness differs from the host native endianness. > + } > + > ret = tun_vnet_hdr_tnl_from_skb(tun->flags, tun->dev, skb, [ ... ] > diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h > index f4c652b1fa44d..0a7fe1ce889b5 100644 > --- a/drivers/net/tun_vnet.h > +++ b/drivers/net/tun_vnet.h > @@ -122,15 +119,20 @@ static inline int __tun_vnet_hdr_get(int sz, unsigned > int flags, > struct iov_iter *from, > struct virtio_net_hdr *hdr) > { > - unsigned int parsed_size = tun_vnet_parse_size(features); > + unsigned int parsed_size = tun_vnet_parse_size(sz); > + u8 tmp[sizeof(struct virtio_net_common_hdr)]; // temp buffer with known > size > u16 hdr_len; > > if (iov_iter_count(from) < sz) > return -EINVAL; > > - if (!copy_from_iter_full(hdr, parsed_size, from)) > + /* copy parsed size data to the tmp buffer, otherwise compiler will > complain */ > + if (!copy_from_iter_full(tmp, parsed_size, from)) > return -EFAULT; > > + /* now copy the relevant data from tmp to hdr */ > + memcpy(hdr, tmp, min(parsed_size, sizeof(*hdr))); [Severity: High] Does this artificially limit the copy to exactly 10 bytes? Because hdr is passed as a pointer to struct virtio_net_hdr, sizeof(*hdr) evaluates statically to 10 bytes. Callers like tun_get_user() might allocate a larger structure, but they will only receive 10 bytes, causing extended virtio-net header fields (such as hash values or tunnel offsets) to be silently dropped. > + > hdr_len = tun_vnet16_to_cpu(flags, hdr->hdr_len); > > if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) { > @@ -155,14 +157,17 @@ static inline int tun_vnet_hdr_get(int sz, unsigned int > flags, > > static inline int __tun_vnet_hdr_put(int sz, netdev_features_t features, > struct iov_iter *iter, > - const struct virtio_net_hdr *hdr) > + const struct virtio_net_common_hdr *hdr) > { > - unsigned int parsed_size = tun_vnet_parse_size(features); > + unsigned int parsed_size = tun_vnet_parse_size(sz); > + u8 buf[sizeof(*hdr)]; > > if (unlikely(iov_iter_count(iter) < sz)) > return -EINVAL; > > - if (unlikely(copy_to_iter(hdr, parsed_size, iter) != parsed_size)) > + memcpy(buf, hdr, parsed_size); [Severity: High] Could this copy leak uninitialized kernel stack memory to userspace? Callers like tap_put_user() and tun_put_user_xdp() allocate a 10-byte struct virtio_net_hdr on the kernel stack and pass its pointer to tun_vnet_hdr_put(). Because tun_vnet_hdr_put() casts the 10-byte pointer to the 32-byte struct virtio_net_common_hdr: return __tun_vnet_hdr_put(sz, 0, iter, (struct virtio_net_common_hdr *)hdr); and parsed_size can be up to 32 bytes (depending on vnet_hdr_sz), this memcpy could read 22 bytes past the end of the 10-byte struct. > + > + if (unlikely(copy_to_iter(buf, parsed_size, iter) != parsed_size)) > return -EFAULT; > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260921-v6-7-topic-virtio-net-ptp-v3-0-3350851c4...@pengutronix.de?part=2
