[PATCH] init_virtio_scsi(): reset the HBA before freeing its virtio ring

[Laszlo Ersek]

When init_virtio_scsi() finds no SCSI targets connected to the HBA, it
frees the virtio ring. Other code in SeaBIOS proceeds to overwrite the
area. However, the ring is in use by qemu at that point -- not only did we
report the (ACK|DRIVER|DRIVER_OK) status earlier, we even communicated
over the ring.

Of course SeaBIOS doesn't "kick" the HBA ever again, hence qemu has no
reason to look at the ring. However, when qemu uses KVM acceleration, and
ioeventfd is enabled for the HBA, then a vmstate change to "running"
(including stop->cont monitor commands and incoming migration) "forces" a
kick (see qemu commit 25db9ebe). Qemu then tries to interpret whatever
unrelated guest data is in the HBA's original ring area, as virtio
protocol. Qemu exits upon seeing the garbage.

init_virtio_scsi() should reset the HBA before allowing the virtio ring
memory to be reused. Device reset causes the hypervisor to drop its
references.

This change is justified / underpinned by pure virtio-spec compliance as
well.

Related RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1013418

Signed-off-by: Laszlo Ersek 
---
 src/hw/virtio-scsi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c
index 4b4ec7b..48fb3e1 100644
--- a/src/hw/virtio-scsi.c
+++ b/src/hw/virtio-scsi.c
@@ -154,16 +154,18 @@ init_virtio_scsi(struct pci_device *pci)
     vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
                   VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);

int i, tot;
     for (tot = 0, i = 0; i < 256; i++)
         tot += virtio_scsi_scan_target(pci, ioaddr, vq, i);

-    if (!tot)
+    if (!tot) {
+        vp_reset(ioaddr);
         goto fail;
+    }

return;

fail:
     free(vq);
 }

[PATCH] init_virtio_scsi(): reset the HBA before freeing its virtio ring

[Paolo Bonzini]

Il 15/01/2014 02:48, Laszlo Ersek ha scritto:
> When init_virtio_scsi() finds no SCSI targets connected to the HBA, it
> frees the virtio ring. Other code in SeaBIOS proceeds to overwrite the
> area. However, the ring is in use by qemu at that point -- not only did we
> report the (ACK|DRIVER|DRIVER_OK) status earlier, we even communicated
> over the ring.
>
> Of course SeaBIOS doesn't "kick" the HBA ever again, hence qemu has no
> reason to look at the ring. However, when qemu uses KVM acceleration, and
> ioeventfd is enabled for the HBA, then a vmstate change to "running"
> (including stop->cont monitor commands and incoming migration) "forces" a
> kick (see qemu commit 25db9ebe). Qemu then tries to interpret whatever
> unrelated guest data is in the HBA's original ring area, as virtio
> protocol. Qemu exits upon seeing the garbage.
>
> init_virtio_scsi() should reset the HBA before allowing the virtio ring
> memory to be reused. Device reset causes the hypervisor to drop its
> references.
>
> This change is justified / underpinned by pure virtio-spec compliance as
> well.
>
> Related RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1013418
> Signed-off-by: Laszlo Ersek
>
> src/hw/virtio-scsi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c
> index 4b4ec7b..48fb3e1 100644
> --- a/src/hw/virtio-scsi.c
> +++ b/src/hw/virtio-scsi.c
> @@ -154,16 +154,18 @@ init_virtio_scsi(struct pci_device *pci)
>      vp_set_status(ioaddr, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>                    VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK);
>  int i, tot;
>  for (tot = 0, i = 0; i < 256; i++)
>      tot += virtio_scsi_scan_target(pci, ioaddr, vq, i);
>
> if (!tot)
>
> if (!tot) {
>    vp_reset(ioaddr);
>    goto fail;
>
> }
>
> return;
>
> fail:
>      free(vq);
>  }

Thanks Laszlo!

Reviewed-by: Paolo Bonzini

Paolo

[PATCH] init_virtio_scsi(): reset the HBA before freeing its virtio ring

[Kevin O'Connor]

On Wed, Jan 15, 2014 at 11:01:59AM +0100, Paolo Bonzini wrote:
> Il 15/01/2014 02:48, Laszlo Ersek ha scritto:
> > When init_virtio_scsi() finds no SCSI targets connected to the HBA, it
> > frees the virtio ring. Other code in SeaBIOS proceeds to overwrite the
> > area. However, the ring is in use by qemu at that point -- not only did we
> > report the (ACK|DRIVER|DRIVER_OK) status earlier, we even communicated
> > over the ring.
> >
> > Of course SeaBIOS doesn't "kick" the HBA ever again, hence qemu has no
> > reason to look at the ring. However, when qemu uses KVM acceleration, and
> > ioeventfd is enabled for the HBA, then a vmstate change to "running"
> > (including stop->cont monitor commands and incoming migration) "forces" a
> > kick (see qemu commit 25db9ebe). Qemu then tries to interpret whatever
> > unrelated guest data is in the HBA's original ring area, as virtio
> > protocol. Qemu exits upon seeing the garbage.
> >
> > init_virtio_scsi() should reset the HBA before allowing the virtio ring
> > memory to be reused. Device reset causes the hypervisor to drop its
> > references.
> >
> > This change is justified / underpinned by pure virtio-spec compliance as
> > well.
> >
> > Related RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1013418
> >
> > Signed-off-by: Laszlo Ersek

[...]
> Thanks Laszlo!
>
> Reviewed-by: Paolo Bonzini
>
> Paolo

Thanks Laszlo - I'm sure that wasn't easy to find.  I've pushed the
patch.

-Kevin

[PATCH] init_virtio_scsi(): reset the HBA before freeing its virtio ring

[Laszlo Ersek]

On 01/17/14 18:17, Kevin O'Connor wrote:
> On Wed, Jan 15, 2014 at 11:01:59AM +0100, Paolo Bonzini wrote:
> > Il 15/01/2014 02:48, Laszlo Ersek ha scritto:
> > > When init_virtio_scsi() finds no SCSI targets connected to the HBA, it
> > > frees the virtio ring. Other code in SeaBIOS proceeds to overwrite the
> > > area. However, the ring is in use by qemu at that point -- not only did we
> > > report the (ACK|DRIVER|DRIVER_OK) status earlier, we even communicated
> > > over the ring.
> > >
> > > Of course SeaBIOS doesn't "kick" the HBA ever again, hence qemu has no
> > > reason to look at the ring. However, when qemu uses KVM acceleration, and
> > > ioeventfd is enabled for the HBA, then a vmstate change to "running"
> > > (including stop->cont monitor commands and incoming migration) "forces" a
> > > kick (see qemu commit 25db9ebe). Qemu then tries to interpret whatever
> > > unrelated guest data is in the HBA's original ring area, as virtio
> > > protocol. Qemu exits upon seeing the garbage.
> > >
> > > init_virtio_scsi() should reset the HBA before allowing the virtio ring
> > > memory to be reused. Device reset causes the hypervisor to drop its
> > > references.
> > >
> > > This change is justified / underpinned by pure virtio-spec compliance as
> > > well.
> > >
> > > Related RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1013418
> > >
> > > Signed-off-by: Laszlo Ersek
>
> [...]
> > Thanks Laszlo!
> >
> > Reviewed-by: Paolo Bonzini
> >
> > Paolo
>
> Thanks Laszlo - I'm sure that wasn't easy to find.

No, it wasn't; the BZ is public and anyone can see my struggle.
> I've pushed the
> patch.

Thank you!

Laszlo