netdev - Re: [PATCH net-next 6/6] tools: virtio: introduce vhost_net

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 21 Dec 2023 10:47:52 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Jason Wang <jasowang@...hat.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 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);

> 
>>
>> 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

> 
> Thanks