* [PATCH] virtio: skip guest index check on device load @ 2020-10-26 15:13 Felipe Franciosi 2020-10-27 11:30 ` Stefan Hajnoczi 0 siblings, 1 reply; 11+ messages in thread From: Felipe Franciosi @ 2020-10-26 15:13 UTC (permalink / raw) To: qemu-devel Cc: Michael S . Tsirkin, Jason Wang, Markus Armbruster, Stefan Hajnoczi, Felipe Franciosi, Paolo Bonzini QEMU must be careful when loading device state off migration streams to prevent a malicious source from exploiting the emulator. Overdoing these checks has the side effect of allowing a guest to "pin itself" in cloud environments by messing with state which is entirely in its control. Similarly to what f3081539 achieved in usb_device_post_load(), this commit removes such a check from virtio_load(). Worth noting, the result of a load without this check is the same as if a guest enables a VQ with invalid indexes to begin with. That is, the virtual device is set in a broken state (by the datapath handler) and must be reset. Signed-off-by: Felipe Franciosi <felipe@nutanix.com> --- hw/virtio/virtio.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 6f8f865aff..0561bdb857 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) RCU_READ_LOCK_GUARD(); for (i = 0; i < num; i++) { if (vdev->vq[i].vring.desc) { - uint16_t nheads; - /* * VIRTIO-1 devices migrate desc, used, and avail ring addresses so * only the region cache needs to be set up. Legacy devices need @@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) continue; } - nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx; - /* Check it isn't doing strange things with descriptor numbers. */ - if (nheads > vdev->vq[i].vring.num) { - error_report("VQ %d size 0x%x Guest index 0x%x " - "inconsistent with Host index 0x%x: delta 0x%x", - i, vdev->vq[i].vring.num, - vring_avail_idx(&vdev->vq[i]), - vdev->vq[i].last_avail_idx, nheads); - return -1; - } vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]); vdev->vq[i].shadow_avail_idx = vring_avail_idx(&vdev->vq[i]); -- 2.18.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] virtio: skip guest index check on device load 2020-10-26 15:13 [PATCH] virtio: skip guest index check on device load Felipe Franciosi @ 2020-10-27 11:30 ` Stefan Hajnoczi 2020-10-27 12:25 ` Michael S. Tsirkin 0 siblings, 1 reply; 11+ messages in thread From: Stefan Hajnoczi @ 2020-10-27 11:30 UTC (permalink / raw) To: Felipe Franciosi Cc: Paolo Bonzini, Jason Wang, qemu-devel, Markus Armbruster, Michael S . Tsirkin [-- Attachment #1: Type: text/plain, Size: 2852 bytes --] On Mon, Oct 26, 2020 at 03:13:32PM +0000, Felipe Franciosi wrote: > QEMU must be careful when loading device state off migration streams to > prevent a malicious source from exploiting the emulator. Overdoing these > checks has the side effect of allowing a guest to "pin itself" in cloud > environments by messing with state which is entirely in its control. > > Similarly to what f3081539 achieved in usb_device_post_load(), this > commit removes such a check from virtio_load(). Worth noting, the result > of a load without this check is the same as if a guest enables a VQ with > invalid indexes to begin with. That is, the virtual device is set in a > broken state (by the datapath handler) and must be reset. > > Signed-off-by: Felipe Franciosi <felipe@nutanix.com> > --- > hw/virtio/virtio.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 6f8f865aff..0561bdb857 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) > RCU_READ_LOCK_GUARD(); > for (i = 0; i < num; i++) { > if (vdev->vq[i].vring.desc) { > - uint16_t nheads; > - > /* > * VIRTIO-1 devices migrate desc, used, and avail ring addresses so > * only the region cache needs to be set up. Legacy devices need > @@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) > continue; > } > > - nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx; > - /* Check it isn't doing strange things with descriptor numbers. */ > - if (nheads > vdev->vq[i].vring.num) { > - error_report("VQ %d size 0x%x Guest index 0x%x " > - "inconsistent with Host index 0x%x: delta 0x%x", > - i, vdev->vq[i].vring.num, > - vring_avail_idx(&vdev->vq[i]), > - vdev->vq[i].last_avail_idx, nheads); > - return -1; > - } Michael, the commit that introduced this check seems to have been for debugging rather than to prevent a QEMU crash, so this removing the check may be safe: commit 258dc7c96bb4b7ca71d5bee811e73933310e168c Author: Michael S. Tsirkin <mst@redhat.com> Date: Sun Oct 17 20:23:48 2010 +0200 virtio: sanity-check available index Checking available index upon load instead of only when vm is running makes is easier to debug failures. Felipe: Did you audit the code to make sure the invalid avail_idx value and the fields it is propagated to (e.g. shadow_avail_idx) are always used in a safe way? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] virtio: skip guest index check on device load 2020-10-27 11:30 ` Stefan Hajnoczi @ 2020-10-27 12:25 ` Michael S. Tsirkin 2020-10-27 12:53 ` Felipe Franciosi 0 siblings, 1 reply; 11+ messages in thread From: Michael S. Tsirkin @ 2020-10-27 12:25 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Paolo Bonzini, Jason Wang, qemu-devel, Markus Armbruster, Felipe Franciosi On Tue, Oct 27, 2020 at 11:30:49AM +0000, Stefan Hajnoczi wrote: > On Mon, Oct 26, 2020 at 03:13:32PM +0000, Felipe Franciosi wrote: > > QEMU must be careful when loading device state off migration streams to > > prevent a malicious source from exploiting the emulator. Overdoing these > > checks has the side effect of allowing a guest to "pin itself" in cloud > > environments by messing with state which is entirely in its control. > > > > Similarly to what f3081539 achieved in usb_device_post_load(), this > > commit removes such a check from virtio_load(). Worth noting, the result > > of a load without this check is the same as if a guest enables a VQ with > > invalid indexes to begin with. That is, the virtual device is set in a > > broken state (by the datapath handler) and must be reset. > > > > Signed-off-by: Felipe Franciosi <felipe@nutanix.com> > > --- > > hw/virtio/virtio.c | 12 ------------ > > 1 file changed, 12 deletions(-) > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > index 6f8f865aff..0561bdb857 100644 > > --- a/hw/virtio/virtio.c > > +++ b/hw/virtio/virtio.c > > @@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) > > RCU_READ_LOCK_GUARD(); > > for (i = 0; i < num; i++) { > > if (vdev->vq[i].vring.desc) { > > - uint16_t nheads; > > - > > /* > > * VIRTIO-1 devices migrate desc, used, and avail ring addresses so > > * only the region cache needs to be set up. Legacy devices need > > @@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) > > continue; > > } > > > > - nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx; > > - /* Check it isn't doing strange things with descriptor numbers. */ > > - if (nheads > vdev->vq[i].vring.num) { > > - error_report("VQ %d size 0x%x Guest index 0x%x " > > - "inconsistent with Host index 0x%x: delta 0x%x", > > - i, vdev->vq[i].vring.num, > > - vring_avail_idx(&vdev->vq[i]), > > - vdev->vq[i].last_avail_idx, nheads); > > - return -1; > > - } > > Michael, the commit that introduced this check seems to have been for > debugging rather than to prevent a QEMU crash, so this removing the > check may be safe: > > commit 258dc7c96bb4b7ca71d5bee811e73933310e168c > Author: Michael S. Tsirkin <mst@redhat.com> > Date: Sun Oct 17 20:23:48 2010 +0200 > > virtio: sanity-check available index > > Checking available index upon load instead of > only when vm is running makes is easier to > debug failures. Agreed. Given this, let's keep the message around, just with LOG_GUEST_ERROR ? > Felipe: Did you audit the code to make sure the invalid avail_idx value > and the fields it is propagated to (e.g. shadow_avail_idx) are always > used in a safe way? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] virtio: skip guest index check on device load 2020-10-27 12:25 ` Michael S. Tsirkin @ 2020-10-27 12:53 ` Felipe Franciosi 2020-10-27 12:56 ` Michael S. Tsirkin 0 siblings, 1 reply; 11+ messages in thread From: Felipe Franciosi @ 2020-10-27 12:53 UTC (permalink / raw) To: Michael S. Tsirkin, Stefan Hajnoczi Cc: Paolo Bonzini, Jason Wang, qemu-devel, Markus Armbruster > On Oct 27, 2020, at 12:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Oct 27, 2020 at 11:30:49AM +0000, Stefan Hajnoczi wrote: >> On Mon, Oct 26, 2020 at 03:13:32PM +0000, Felipe Franciosi wrote: >>> QEMU must be careful when loading device state off migration streams to >>> prevent a malicious source from exploiting the emulator. Overdoing these >>> checks has the side effect of allowing a guest to "pin itself" in cloud >>> environments by messing with state which is entirely in its control. >>> >>> Similarly to what f3081539 achieved in usb_device_post_load(), this >>> commit removes such a check from virtio_load(). Worth noting, the result >>> of a load without this check is the same as if a guest enables a VQ with >>> invalid indexes to begin with. That is, the virtual device is set in a >>> broken state (by the datapath handler) and must be reset. >>> >>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> >>> --- >>> hw/virtio/virtio.c | 12 ------------ >>> 1 file changed, 12 deletions(-) >>> >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>> index 6f8f865aff..0561bdb857 100644 >>> --- a/hw/virtio/virtio.c >>> +++ b/hw/virtio/virtio.c >>> @@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) >>> RCU_READ_LOCK_GUARD(); >>> for (i = 0; i < num; i++) { >>> if (vdev->vq[i].vring.desc) { >>> - uint16_t nheads; >>> - >>> /* >>> * VIRTIO-1 devices migrate desc, used, and avail ring addresses so >>> * only the region cache needs to be set up. Legacy devices need >>> @@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) >>> continue; >>> } >>> >>> - nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx; >>> - /* Check it isn't doing strange things with descriptor numbers. */ >>> - if (nheads > vdev->vq[i].vring.num) { >>> - error_report("VQ %d size 0x%x Guest index 0x%x " >>> - "inconsistent with Host index 0x%x: delta 0x%x", >>> - i, vdev->vq[i].vring.num, >>> - vring_avail_idx(&vdev->vq[i]), >>> - vdev->vq[i].last_avail_idx, nheads); >>> - return -1; >>> - } >> >> Michael, the commit that introduced this check seems to have been for >> debugging rather than to prevent a QEMU crash, so this removing the >> check may be safe: >> >> commit 258dc7c96bb4b7ca71d5bee811e73933310e168c >> Author: Michael S. Tsirkin <mst@redhat.com> >> Date: Sun Oct 17 20:23:48 2010 +0200 >> >> virtio: sanity-check available index >> >> Checking available index upon load instead of >> only when vm is running makes is easier to >> debug failures. > > Agreed. Given this, let's keep the message around, just with > LOG_GUEST_ERROR ? I thought about it. Happy to send a v2 which keeps the check and logs without throwing an error. Separately, I'm thinking of hooking up QEMU with VRING_ERR so datapath handlers can notify QEMU that something went broken and NEEDS_RESET can be flipped on the status register, possibly along a configuration interrupt. I can see libvhost-user supports that, but are there any reasons QEMU doesn't do this already? > >> Felipe: Did you audit the code to make sure the invalid avail_idx value >> and the fields it is propagated to (e.g. shadow_avail_idx) are always >> used in a safe way? > I did it briefly. I also wrote a mock userspace driver that creates this condition in a very controlled way (so I can step half-way through setting up VQs and trigger a migration, for example). But you know how manual tests are... I may have missed something. Your expert eyes are most welcome. :) F. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] virtio: skip guest index check on device load 2020-10-27 12:53 ` Felipe Franciosi @ 2020-10-27 12:56 ` Michael S. Tsirkin 2020-10-27 13:02 ` Felipe Franciosi 0 siblings, 1 reply; 11+ messages in thread From: Michael S. Tsirkin @ 2020-10-27 12:56 UTC (permalink / raw) To: Felipe Franciosi Cc: Paolo Bonzini, Jason Wang, qemu-devel, Stefan Hajnoczi, Markus Armbruster On Tue, Oct 27, 2020 at 12:53:29PM +0000, Felipe Franciosi wrote: > > > > On Oct 27, 2020, at 12:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Tue, Oct 27, 2020 at 11:30:49AM +0000, Stefan Hajnoczi wrote: > >> On Mon, Oct 26, 2020 at 03:13:32PM +0000, Felipe Franciosi wrote: > >>> QEMU must be careful when loading device state off migration streams to > >>> prevent a malicious source from exploiting the emulator. Overdoing these > >>> checks has the side effect of allowing a guest to "pin itself" in cloud > >>> environments by messing with state which is entirely in its control. > >>> > >>> Similarly to what f3081539 achieved in usb_device_post_load(), this > >>> commit removes such a check from virtio_load(). Worth noting, the result > >>> of a load without this check is the same as if a guest enables a VQ with > >>> invalid indexes to begin with. That is, the virtual device is set in a > >>> broken state (by the datapath handler) and must be reset. > >>> > >>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> > >>> --- > >>> hw/virtio/virtio.c | 12 ------------ > >>> 1 file changed, 12 deletions(-) > >>> > >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >>> index 6f8f865aff..0561bdb857 100644 > >>> --- a/hw/virtio/virtio.c > >>> +++ b/hw/virtio/virtio.c > >>> @@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) > >>> RCU_READ_LOCK_GUARD(); > >>> for (i = 0; i < num; i++) { > >>> if (vdev->vq[i].vring.desc) { > >>> - uint16_t nheads; > >>> - > >>> /* > >>> * VIRTIO-1 devices migrate desc, used, and avail ring addresses so > >>> * only the region cache needs to be set up. Legacy devices need > >>> @@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) > >>> continue; > >>> } > >>> > >>> - nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx; > >>> - /* Check it isn't doing strange things with descriptor numbers. */ > >>> - if (nheads > vdev->vq[i].vring.num) { > >>> - error_report("VQ %d size 0x%x Guest index 0x%x " > >>> - "inconsistent with Host index 0x%x: delta 0x%x", > >>> - i, vdev->vq[i].vring.num, > >>> - vring_avail_idx(&vdev->vq[i]), > >>> - vdev->vq[i].last_avail_idx, nheads); > >>> - return -1; > >>> - } > >> > >> Michael, the commit that introduced this check seems to have been for > >> debugging rather than to prevent a QEMU crash, so this removing the > >> check may be safe: > >> > >> commit 258dc7c96bb4b7ca71d5bee811e73933310e168c > >> Author: Michael S. Tsirkin <mst@redhat.com> > >> Date: Sun Oct 17 20:23:48 2010 +0200 > >> > >> virtio: sanity-check available index > >> > >> Checking available index upon load instead of > >> only when vm is running makes is easier to > >> debug failures. > > > > Agreed. Given this, let's keep the message around, just with > > LOG_GUEST_ERROR ? > > I thought about it. Happy to send a v2 which keeps the check and logs > without throwing an error. > > Separately, I'm thinking of hooking up QEMU with VRING_ERR so datapath > handlers can notify QEMU that something went broken and NEEDS_RESET > can be flipped on the status register, possibly along a configuration > interrupt. I can see libvhost-user supports that, but are there any > reasons QEMU doesn't do this already? Mostly because guest support isn't there. That in turn isn't easy, lots of synchronization is needed within guests. > > > >> Felipe: Did you audit the code to make sure the invalid avail_idx value > >> and the fields it is propagated to (e.g. shadow_avail_idx) are always > >> used in a safe way? > > > > I did it briefly. I also wrote a mock userspace driver that creates > this condition in a very controlled way (so I can step half-way > through setting up VQs and trigger a migration, for example). But you > know how manual tests are... I may have missed something. > Your expert eyes are most welcome. :) > > F. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] virtio: skip guest index check on device load 2020-10-27 12:56 ` Michael S. Tsirkin @ 2020-10-27 13:02 ` Felipe Franciosi 2020-10-27 13:04 ` Michael S. Tsirkin 0 siblings, 1 reply; 11+ messages in thread From: Felipe Franciosi @ 2020-10-27 13:02 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Bonzini, Jason Wang, qemu-devel, Stefan Hajnoczi, Markus Armbruster > On Oct 27, 2020, at 12:56 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Oct 27, 2020 at 12:53:29PM +0000, Felipe Franciosi wrote: >> >> >>> On Oct 27, 2020, at 12:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >>> >>> On Tue, Oct 27, 2020 at 11:30:49AM +0000, Stefan Hajnoczi wrote: >>>> On Mon, Oct 26, 2020 at 03:13:32PM +0000, Felipe Franciosi wrote: >>>>> QEMU must be careful when loading device state off migration streams to >>>>> prevent a malicious source from exploiting the emulator. Overdoing these >>>>> checks has the side effect of allowing a guest to "pin itself" in cloud >>>>> environments by messing with state which is entirely in its control. >>>>> >>>>> Similarly to what f3081539 achieved in usb_device_post_load(), this >>>>> commit removes such a check from virtio_load(). Worth noting, the result >>>>> of a load without this check is the same as if a guest enables a VQ with >>>>> invalid indexes to begin with. That is, the virtual device is set in a >>>>> broken state (by the datapath handler) and must be reset. >>>>> >>>>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> >>>>> --- >>>>> hw/virtio/virtio.c | 12 ------------ >>>>> 1 file changed, 12 deletions(-) >>>>> >>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>>>> index 6f8f865aff..0561bdb857 100644 >>>>> --- a/hw/virtio/virtio.c >>>>> +++ b/hw/virtio/virtio.c >>>>> @@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) >>>>> RCU_READ_LOCK_GUARD(); >>>>> for (i = 0; i < num; i++) { >>>>> if (vdev->vq[i].vring.desc) { >>>>> - uint16_t nheads; >>>>> - >>>>> /* >>>>> * VIRTIO-1 devices migrate desc, used, and avail ring addresses so >>>>> * only the region cache needs to be set up. Legacy devices need >>>>> @@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) >>>>> continue; >>>>> } >>>>> >>>>> - nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx; >>>>> - /* Check it isn't doing strange things with descriptor numbers. */ >>>>> - if (nheads > vdev->vq[i].vring.num) { >>>>> - error_report("VQ %d size 0x%x Guest index 0x%x " >>>>> - "inconsistent with Host index 0x%x: delta 0x%x", >>>>> - i, vdev->vq[i].vring.num, >>>>> - vring_avail_idx(&vdev->vq[i]), >>>>> - vdev->vq[i].last_avail_idx, nheads); >>>>> - return -1; >>>>> - } >>>> >>>> Michael, the commit that introduced this check seems to have been for >>>> debugging rather than to prevent a QEMU crash, so this removing the >>>> check may be safe: >>>> >>>> commit 258dc7c96bb4b7ca71d5bee811e73933310e168c >>>> Author: Michael S. Tsirkin <mst@redhat.com> >>>> Date: Sun Oct 17 20:23:48 2010 +0200 >>>> >>>> virtio: sanity-check available index >>>> >>>> Checking available index upon load instead of >>>> only when vm is running makes is easier to >>>> debug failures. >>> >>> Agreed. Given this, let's keep the message around, just with >>> LOG_GUEST_ERROR ? >> >> I thought about it. Happy to send a v2 which keeps the check and logs >> without throwing an error. >> >> Separately, I'm thinking of hooking up QEMU with VRING_ERR so datapath >> handlers can notify QEMU that something went broken and NEEDS_RESET >> can be flipped on the status register, possibly along a configuration >> interrupt. I can see libvhost-user supports that, but are there any >> reasons QEMU doesn't do this already? > > Mostly because guest support isn't there. That in turn isn't easy, > lots of synchronization is needed within guests. Do you mean guest support to reset when seeing that bit in status following the configuration interrupt? It should be safe, though. I can have a stab to see if it breaks Windows/Linux at least, and share an RFC if I get anywhere. Unless you think it's a waste of time. Ideally guests shouldn't find themselves in this situation to begin with, and if they did, resetting would arguably just lead them into corruption again (for example). But it does provide a mechanism for QEMU to find out that the vhost backend stopped. That would help in the context of this patch. F. > > >>> >>>> Felipe: Did you audit the code to make sure the invalid avail_idx value >>>> and the fields it is propagated to (e.g. shadow_avail_idx) are always >>>> used in a safe way? >>> >> >> I did it briefly. I also wrote a mock userspace driver that creates >> this condition in a very controlled way (so I can step half-way >> through setting up VQs and trigger a migration, for example). But you >> know how manual tests are... I may have missed something. >> Your expert eyes are most welcome. :) >> >> F. > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] virtio: skip guest index check on device load 2020-10-27 13:02 ` Felipe Franciosi @ 2020-10-27 13:04 ` Michael S. Tsirkin 2020-10-28 11:00 ` Stefan Hajnoczi 0 siblings, 1 reply; 11+ messages in thread From: Michael S. Tsirkin @ 2020-10-27 13:04 UTC (permalink / raw) To: Felipe Franciosi Cc: Paolo Bonzini, Jason Wang, qemu-devel, Stefan Hajnoczi, Markus Armbruster On Tue, Oct 27, 2020 at 01:02:59PM +0000, Felipe Franciosi wrote: > > > > On Oct 27, 2020, at 12:56 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Tue, Oct 27, 2020 at 12:53:29PM +0000, Felipe Franciosi wrote: > >> > >> > >>> On Oct 27, 2020, at 12:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > >>> > >>> On Tue, Oct 27, 2020 at 11:30:49AM +0000, Stefan Hajnoczi wrote: > >>>> On Mon, Oct 26, 2020 at 03:13:32PM +0000, Felipe Franciosi wrote: > >>>>> QEMU must be careful when loading device state off migration streams to > >>>>> prevent a malicious source from exploiting the emulator. Overdoing these > >>>>> checks has the side effect of allowing a guest to "pin itself" in cloud > >>>>> environments by messing with state which is entirely in its control. > >>>>> > >>>>> Similarly to what f3081539 achieved in usb_device_post_load(), this > >>>>> commit removes such a check from virtio_load(). Worth noting, the result > >>>>> of a load without this check is the same as if a guest enables a VQ with > >>>>> invalid indexes to begin with. That is, the virtual device is set in a > >>>>> broken state (by the datapath handler) and must be reset. > >>>>> > >>>>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com> > >>>>> --- > >>>>> hw/virtio/virtio.c | 12 ------------ > >>>>> 1 file changed, 12 deletions(-) > >>>>> > >>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >>>>> index 6f8f865aff..0561bdb857 100644 > >>>>> --- a/hw/virtio/virtio.c > >>>>> +++ b/hw/virtio/virtio.c > >>>>> @@ -3136,8 +3136,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) > >>>>> RCU_READ_LOCK_GUARD(); > >>>>> for (i = 0; i < num; i++) { > >>>>> if (vdev->vq[i].vring.desc) { > >>>>> - uint16_t nheads; > >>>>> - > >>>>> /* > >>>>> * VIRTIO-1 devices migrate desc, used, and avail ring addresses so > >>>>> * only the region cache needs to be set up. Legacy devices need > >>>>> @@ -3157,16 +3155,6 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) > >>>>> continue; > >>>>> } > >>>>> > >>>>> - nheads = vring_avail_idx(&vdev->vq[i]) - vdev->vq[i].last_avail_idx; > >>>>> - /* Check it isn't doing strange things with descriptor numbers. */ > >>>>> - if (nheads > vdev->vq[i].vring.num) { > >>>>> - error_report("VQ %d size 0x%x Guest index 0x%x " > >>>>> - "inconsistent with Host index 0x%x: delta 0x%x", > >>>>> - i, vdev->vq[i].vring.num, > >>>>> - vring_avail_idx(&vdev->vq[i]), > >>>>> - vdev->vq[i].last_avail_idx, nheads); > >>>>> - return -1; > >>>>> - } > >>>> > >>>> Michael, the commit that introduced this check seems to have been for > >>>> debugging rather than to prevent a QEMU crash, so this removing the > >>>> check may be safe: > >>>> > >>>> commit 258dc7c96bb4b7ca71d5bee811e73933310e168c > >>>> Author: Michael S. Tsirkin <mst@redhat.com> > >>>> Date: Sun Oct 17 20:23:48 2010 +0200 > >>>> > >>>> virtio: sanity-check available index > >>>> > >>>> Checking available index upon load instead of > >>>> only when vm is running makes is easier to > >>>> debug failures. > >>> > >>> Agreed. Given this, let's keep the message around, just with > >>> LOG_GUEST_ERROR ? > >> > >> I thought about it. Happy to send a v2 which keeps the check and logs > >> without throwing an error. > >> > >> Separately, I'm thinking of hooking up QEMU with VRING_ERR so datapath > >> handlers can notify QEMU that something went broken and NEEDS_RESET > >> can be flipped on the status register, possibly along a configuration > >> interrupt. I can see libvhost-user supports that, but are there any > >> reasons QEMU doesn't do this already? > > > > Mostly because guest support isn't there. That in turn isn't easy, > > lots of synchronization is needed within guests. > > Do you mean guest support to reset when seeing that bit in status > following the configuration interrupt? > > It should be safe, though. I can have a stab to see if it breaks > Windows/Linux at least, and share an RFC if I get anywhere. > > Unless you think it's a waste of time. Ideally guests shouldn't find > themselves in this situation to begin with, and if they did, resetting > would arguably just lead them into corruption again (for example). But > it does provide a mechanism for QEMU to find out that the vhost > backend stopped. That would help in the context of this patch. > > F. It's not a waste of time, it's just a lot of work within guests. > > > > > >>> > >>>> Felipe: Did you audit the code to make sure the invalid avail_idx value > >>>> and the fields it is propagated to (e.g. shadow_avail_idx) are always > >>>> used in a safe way? > >>> > >> > >> I did it briefly. I also wrote a mock userspace driver that creates > >> this condition in a very controlled way (so I can step half-way > >> through setting up VQs and trigger a migration, for example). But you > >> know how manual tests are... I may have missed something. > >> Your expert eyes are most welcome. :) > >> > >> F. > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] virtio: skip guest index check on device load 2020-10-27 13:04 ` Michael S. Tsirkin @ 2020-10-28 11:00 ` Stefan Hajnoczi 2020-10-28 11:30 ` Michael S. Tsirkin 2020-11-02 13:31 ` Dr. David Alan Gilbert 0 siblings, 2 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2020-10-28 11:00 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Bonzini, Jason Wang, qemu-devel, Markus Armbruster, Felipe Franciosi [-- Attachment #1: Type: text/plain, Size: 939 bytes --] On Tue, Oct 27, 2020 at 09:04:46AM -0400, Michael S. Tsirkin wrote: > It's not a waste of time, it's just a lot of work > within guests. Luckily it does no harm to set the NEEDS_RESET bit even if the guest doesn't handle it. If the guest driver is unaware it may continue to submit requests to the device for a while. The device emulation code stops accepting new requests though. This means the device will become unresponsive until reset, which is not ideal but okay in the case where the device was put into an invalid state. I agree that supporting NEEDS_RESET transparently inside guests is difficult. The driver needs to reset and resume the device without reporting errors to applications. In some cases drivers may not have enough state in order to do that. It's also tricky to test all code paths. I guess this is why no one has done it: drivers shouldn't enter the NEEDS_RESET state anyway and handling it is complex. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] virtio: skip guest index check on device load 2020-10-28 11:00 ` Stefan Hajnoczi @ 2020-10-28 11:30 ` Michael S. Tsirkin 2020-10-28 12:44 ` Stefan Hajnoczi 2020-11-02 13:31 ` Dr. David Alan Gilbert 1 sibling, 1 reply; 11+ messages in thread From: Michael S. Tsirkin @ 2020-10-28 11:30 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Paolo Bonzini, Jason Wang, qemu-devel, Markus Armbruster, Felipe Franciosi On Wed, Oct 28, 2020 at 11:00:38AM +0000, Stefan Hajnoczi wrote: > On Tue, Oct 27, 2020 at 09:04:46AM -0400, Michael S. Tsirkin wrote: > > It's not a waste of time, it's just a lot of work > > within guests. > > Luckily it does no harm to set the NEEDS_RESET bit even if the guest > doesn't handle it. > > If the guest driver is unaware it may continue to submit requests to the > device for a while. The device emulation code stops accepting new > requests though. This means the device will become unresponsive until > reset, which is not ideal but okay in the case where the device was put > into an invalid state. There is no actual rule that device must stop processing requests. Driver can only assume that is the case after it started the actual reset. > I agree that supporting NEEDS_RESET transparently inside guests is > difficult. The driver needs to reset and resume the device without > reporting errors to applications. In some cases drivers may not have > enough state in order to do that. It's also tricky to test all code > paths. I guess this is why no one has done it: drivers shouldn't enter > the NEEDS_RESET state anyway and handling it is complex. > > Stefan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] virtio: skip guest index check on device load 2020-10-28 11:30 ` Michael S. Tsirkin @ 2020-10-28 12:44 ` Stefan Hajnoczi 0 siblings, 0 replies; 11+ messages in thread From: Stefan Hajnoczi @ 2020-10-28 12:44 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Bonzini, Jason Wang, qemu-devel, Markus Armbruster, Felipe Franciosi [-- Attachment #1: Type: text/plain, Size: 1127 bytes --] On Wed, Oct 28, 2020 at 07:30:55AM -0400, Michael S. Tsirkin wrote: > On Wed, Oct 28, 2020 at 11:00:38AM +0000, Stefan Hajnoczi wrote: > > On Tue, Oct 27, 2020 at 09:04:46AM -0400, Michael S. Tsirkin wrote: > > > It's not a waste of time, it's just a lot of work > > > within guests. > > > > Luckily it does no harm to set the NEEDS_RESET bit even if the guest > > doesn't handle it. > > > > If the guest driver is unaware it may continue to submit requests to the > > device for a while. The device emulation code stops accepting new > > requests though. This means the device will become unresponsive until > > reset, which is not ideal but okay in the case where the device was put > > into an invalid state. > > There is no actual rule that device must stop processing requests. > Driver can only assume that is the case after it started the > actual reset. Right, I mean QEMU's implementation stops processing new requests once the device has been marked "broken": void *virtqueue_pop(VirtQueue *vq, size_t sz) { if (virtio_device_disabled(vq->vdev)) { <--- return NULL; [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] virtio: skip guest index check on device load 2020-10-28 11:00 ` Stefan Hajnoczi 2020-10-28 11:30 ` Michael S. Tsirkin @ 2020-11-02 13:31 ` Dr. David Alan Gilbert 1 sibling, 0 replies; 11+ messages in thread From: Dr. David Alan Gilbert @ 2020-11-02 13:31 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Michael S. Tsirkin, Jason Wang, qemu-devel, Markus Armbruster, Felipe Franciosi, Paolo Bonzini * Stefan Hajnoczi (stefanha@redhat.com) wrote: > On Tue, Oct 27, 2020 at 09:04:46AM -0400, Michael S. Tsirkin wrote: > > It's not a waste of time, it's just a lot of work > > within guests. > > Luckily it does no harm to set the NEEDS_RESET bit even if the guest > doesn't handle it. > > If the guest driver is unaware it may continue to submit requests to the > device for a while. The device emulation code stops accepting new > requests though. This means the device will become unresponsive until > reset, which is not ideal but okay in the case where the device was put > into an invalid state. > > I agree that supporting NEEDS_RESET transparently inside guests is > difficult. The driver needs to reset and resume the device without > reporting errors to applications. Is that required? I mean, what are the semantics of NEEDS_RESET - is it assuming that you must be able to do a silent recovery? Dave > In some cases drivers may not have > enough state in order to do that. It's also tricky to test all code > paths. I guess this is why no one has done it: drivers shouldn't enter > the NEEDS_RESET state anyway and handling it is complex. > > Stefan -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-11-02 13:32 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-26 15:13 [PATCH] virtio: skip guest index check on device load Felipe Franciosi 2020-10-27 11:30 ` Stefan Hajnoczi 2020-10-27 12:25 ` Michael S. Tsirkin 2020-10-27 12:53 ` Felipe Franciosi 2020-10-27 12:56 ` Michael S. Tsirkin 2020-10-27 13:02 ` Felipe Franciosi 2020-10-27 13:04 ` Michael S. Tsirkin 2020-10-28 11:00 ` Stefan Hajnoczi 2020-10-28 11:30 ` Michael S. Tsirkin 2020-10-28 12:44 ` Stefan Hajnoczi 2020-11-02 13:31 ` Dr. David Alan Gilbert
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).