lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACGkMEvKOWpxpBR+YRuiJJ3aEsdxU2q+qVwmFw=L5gS3e7A35w@mail.gmail.com> Date: Thu, 21 Dec 2023 10:56:36 +0800 From: Jason Wang <jasowang@...hat.com> To: Yunsheng Lin <linyunsheng@...wei.com> Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, "Michael S. Tsirkin" <mst@...hat.com>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, virtualization@...ts.linux.dev Subject: Re: [PATCH net-next 6/6] tools: virtio: introduce vhost_net_test On Thu, Dec 21, 2023 at 10:48 AM Yunsheng Lin <linyunsheng@...wei.com> wrote: > > On 2023/12/21 10:33, Jason Wang wrote: > > On Wed, Dec 20, 2023 at 8:45 PM Yunsheng Lin <linyunsheng@...wei.com> wrote: > >> > >> On 2023/12/12 12:35, Jason Wang wrote:>>>> +done: > >>>>>> + backend.fd = tun_alloc(); > >>>>>> + assert(backend.fd >= 0); > >>>>>> + vdev_info_init(&dev, features); > >>>>>> + vq_info_add(&dev, 256); > >>>>>> + run_test(&dev, &dev.vqs[0], delayed, batch, reset, nbufs); > >>>>> > >>>>> I'd expect we are testing some basic traffic here. E.g can we use a > >>>>> packet socket then we can test both tx and rx? > >>>> > >>>> Yes, only rx for tun is tested. > >>>> Do you have an idea how to test the tx too? As I am not familar enough > >>>> with vhost_net and tun yet. > >>> > >>> Maybe you can have a packet socket to bind to the tun/tap. Then you can test: > >>> > >>> 1) TAP RX: by write a packet via virtqueue through vhost_net and read > >>> it from packet socket > >>> 2) TAP TX: by write via packet socket and read it from the virtqueue > >>> through vhost_net > >> > >> When implementing the TAP TX by adding VHOST_NET_F_VIRTIO_NET_HDR, > >> I found one possible use of uninitialized data in vhost_net_build_xdp(). > >> > >> And vhost_hlen is set to sizeof(struct virtio_net_hdr_mrg_rxbuf) and > >> sock_hlen is set to zero in vhost_net_set_features() for both tx and rx > >> queue. > >> > >> For vhost_net_build_xdp() called by handle_tx_copy(): > >> > >> The (gso->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) checking below may cause a > >> read of uninitialized data if sock_hlen is zero. > > > > Which data is uninitialized here? > > The 'gso', as the sock_hlen is zero, there is no copying for: > > copied = copy_page_from_iter(alloc_frag->page, > alloc_frag->offset + > offsetof(struct tun_xdp_hdr, gso), > sock_hlen, from); I think you're right. This is something we need to fix. Or we can drop VHOST_NET_F_VIRTIO_NET_HDR as we managed to survive for years: https://patchwork.ozlabs.org/project/netdev/patch/1528429842-22835-1-git-send-email-jasowang@redhat.com/#1930760 > > > > >> > >> And it seems vhost_hdr is skipped in get_tx_bufs(): > >> https://elixir.bootlin.com/linux/latest/source/drivers/vhost/net.c#L616 > >> > >> static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq, > >> struct iov_iter *from) > >> { > >> ... > >> buflen += SKB_DATA_ALIGN(len + pad); > >> alloc_frag->offset = ALIGN((u64)alloc_frag->offset, SMP_CACHE_BYTES); > >> if (unlikely(!vhost_net_page_frag_refill(net, buflen, > >> alloc_frag, GFP_KERNEL))) > >> return -ENOMEM; > >> > >> buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset; > >> copied = copy_page_from_iter(alloc_frag->page, > >> alloc_frag->offset + > >> offsetof(struct tun_xdp_hdr, gso), > >> sock_hlen, from); > >> if (copied != sock_hlen) > >> return -EFAULT; > >> > >> hdr = buf; > >> gso = &hdr->gso; > >> > >> if ((gso->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && > >> vhost16_to_cpu(vq, gso->csum_start) + > >> vhost16_to_cpu(vq, gso->csum_offset) + 2 > > >> vhost16_to_cpu(vq, gso->hdr_len)) { > >> ... > >> } > >> > >> I seems the handle_tx_copy() does not handle the VHOST_NET_F_VIRTIO_NET_HDR > >> case correctly, Or do I miss something obvious here? > > > > In get_tx_bufs() we did: > > > > *len = init_iov_iter(vq, &msg->msg_iter, nvq->vhost_hlen, *out); > > > > Which covers this case? > > It does not seems to cover it, as the vhost_hdr is just skipped without any > handling in get_tx_bufs(): > https://elixir.bootlin.com/linux/v6.7-rc6/source/drivers/vhost/net.c#L616 My understanding is that in this case vhost can't do more than this as the socket doesn't know vnet_hdr. Let's see if Michael is ok with this. Thanks > > > > > Thanks >
Powered by blists - more mailing lists