On Thu, Nov 10, 2022 at 4:28 PM Xuan Zhuo <xuanz...@linux.alibaba.com> wrote:
>
> Run shell script:
>
> cat << EOF | valgrind qemu-system-i386 -display none -machine
> accel=qtest, -m \
> 512M -M q35 -nodefaults -device virtio-net,netdev=net0 -netdev \
> user,id=net0 -qtest stdio
> outl 0xcf8 0x80000810
> outl 0xcfc 0xc000
> outl 0xcf8 0x80000804
> outl 0xcfc 0x01
> outl 0xc00d 0x0200
> outl 0xcf8 0x80000890
> outb 0xcfc 0x4
> outl 0xcf8 0x80000889
> outl 0xcfc 0x1c000000
> outl 0xcf8 0x80000893
> outw 0xcfc 0x100
> EOF
>
> Got:
> ==68666== Invalid read of size 8
> ==68666== at 0x688536: virtio_net_queue_enable (virtio-net.c:575)
> ==68666== by 0x6E31AE: memory_region_write_accessor (memory.c:492)
> ==68666== by 0x6E098D: access_with_adjusted_size (memory.c:554)
> ==68666== by 0x6E4DB3: memory_region_dispatch_write (memory.c:1521)
> ==68666== by 0x6E31AE: memory_region_write_accessor (memory.c:492)
> ==68666== by 0x6E098D: access_with_adjusted_size (memory.c:554)
> ==68666== by 0x6E4DB3: memory_region_dispatch_write (memory.c:1521)
> ==68666== by 0x6EBCD3: flatview_write_continue (physmem.c:2820)
> ==68666== by 0x6EBFBF: flatview_write (physmem.c:2862)
> ==68666== by 0x6EF5E7: address_space_write (physmem.c:2958)
> ==68666== by 0x6DFDEC: cpu_outw (ioport.c:70)
> ==68666== by 0x6F6DF0: qtest_process_command (qtest.c:480)
> ==68666== Address 0x29087fe8 is 24 bytes after a block of size 416 in
> arena "client"
>
> That is reported by Alexander Bulekov.
> https://gitlab.com/qemu-project/qemu/-/issues/1309
>
> Here, the queue_index is the index of the cvq, but cvq does not have the
> corresponding NetClientState,
This is not necessarily truth for some backends like vhost-vDPA.
> so overflow appears.
Note that this is guest trigger-able, so anything that is below the
VIRTIO_QUEUE_MAX but greater or equal than cvq index could hit this.
>
> I add a check here, ignore illegal queue_index and cvq queue_index.
>
> Signed-off-by: Xuan Zhuo <xuanz...@linux.alibaba.com>
> ---
> hw/net/virtio-net.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 975bbc22f9..88f25709d6 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -549,7 +549,14 @@ static RxFilterInfo
> *virtio_net_query_rxfilter(NetClientState *nc)
> static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
If we require the VirtioDeviceClass to validate the index, let's add a
document there. Or we can let the transport to validate this.
Thanks
> {
> VirtIONet *n = VIRTIO_NET(vdev);
> - NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> + NetClientState *nc;
> +
> + /* validate queue_index and skip for cvq */
> + if (queue_index >= n->max_queue_pairs * 2) {
> + return;
> + }
> +
> + nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
>
> if (!nc->peer) {
> return;
> @@ -566,9 +573,16 @@ static void virtio_net_queue_reset(VirtIODevice *vdev,
> uint32_t queue_index)
> static void virtio_net_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
> {
> VirtIONet *n = VIRTIO_NET(vdev);
> - NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> + NetClientState *nc;
> int r;
>
> + /* validate queue_index and skip for cvq */
> + if (queue_index >= n->max_queue_pairs * 2) {
> + return;
> + }
> +
> + nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> +
> if (!nc->peer || !vdev->vhost_started) {
> return;
> }
> --
> 2.32.0.3.g01195cf9f
>
