Re: [PATCH] virtio-net: fix for heap-buffer-overflow

[Xuan Zhuo]

On Thu, 10 Nov 2022 17:18:23 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Thu, Nov 10, 2022 at 4:28 PM Xuan Zhuo <xuanzhuo@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.

Oh, I ignored it.


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

Yes


>
> >
> > I add a check here, ignore illegal queue_index and cvq queue_index.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@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.

My understanding, it can not be verified in transport for the time being, so
add some instructions first.

Thanks.

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