* [Qemu-devel] [PATCH v3 0/2] check VirtiQueue Vring objects @ 2017-11-24 18:34 P J P 2017-11-24 18:34 ` [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set P J P 2017-11-24 18:34 ` [Qemu-devel] [PATCH v3 2/2] tests: add test to check VirtQueue object P J P 0 siblings, 2 replies; 13+ messages in thread From: P J P @ 2017-11-24 18:34 UTC (permalink / raw) To: Qemu Developers Cc: Stefan Hajnoczi, zhangboxian, Paolo Bonzini, Cornelia Huck, Prasad J Pandit From: Prasad J Pandit <pjp@fedoraproject.org> Hello, An user could attempt to use an uninitialised VirtQueue object or set Vring object with undue values, raising an unexpected exception in Qemu. This patch set fixes this issue and also adds a unit test to the suite. Thank you. -- Prasad J Pandit (2): virtio: check VirtQueue Vring object is set tests: add test to check VirtQueue object Update: Fixed style error, add space before parenthesis '(' -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04652.html hw/virtio/virtio.c | 14 +++++++++++--- tests/virtio-blk-test.c | 25 +++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) -- 2.13.6 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set 2017-11-24 18:34 [Qemu-devel] [PATCH v3 0/2] check VirtiQueue Vring objects P J P @ 2017-11-24 18:34 ` P J P 2017-11-27 10:03 ` Cornelia Huck 2017-11-27 11:15 ` Stefan Hajnoczi 2017-11-24 18:34 ` [Qemu-devel] [PATCH v3 2/2] tests: add test to check VirtQueue object P J P 1 sibling, 2 replies; 13+ messages in thread From: P J P @ 2017-11-24 18:34 UTC (permalink / raw) To: Qemu Developers Cc: Stefan Hajnoczi, zhangboxian, Paolo Bonzini, Cornelia Huck, Prasad J Pandit From: Prasad J Pandit <pjp@fedoraproject.org> An user could attempt to use an uninitialised VirtQueue object or unset Vring.align leading to a arithmetic exception. Add check to avoid it. Reported-by: Zhangboxian <zhangboxian@huawei.com> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> --- hw/virtio/virtio.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 5884ce3480..c01eac87a5 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -182,7 +182,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n) { VRing *vring = &vdev->vq[n].vring; - if (!vring->desc) { + if (!vdev->vq[n].vring.num || !vring->desc || !vring->align) { /* not yet setup -> nothing to do */ return; } @@ -1414,6 +1414,9 @@ void virtio_config_modern_writel(VirtIODevice *vdev, void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr) { + if (!vdev->vq[n].vring.num) { + return; + } vdev->vq[n].vring.desc = addr; virtio_queue_update_rings(vdev, n); } @@ -1426,6 +1429,9 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n) void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc, hwaddr avail, hwaddr used) { + if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) { + return; + } vdev->vq[n].vring.desc = desc; vdev->vq[n].vring.avail = avail; vdev->vq[n].vring.used = used; @@ -1494,8 +1500,10 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align) */ assert(k->has_variable_vring_alignment); - vdev->vq[n].vring.align = align; - virtio_queue_update_rings(vdev, n); + if (align) { + vdev->vq[n].vring.align = align; + virtio_queue_update_rings(vdev, n); + } } static bool virtio_queue_notify_aio_vq(VirtQueue *vq) -- 2.13.6 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set 2017-11-24 18:34 ` [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set P J P @ 2017-11-27 10:03 ` Cornelia Huck 2017-11-27 11:15 ` Stefan Hajnoczi 1 sibling, 0 replies; 13+ messages in thread From: Cornelia Huck @ 2017-11-27 10:03 UTC (permalink / raw) To: P J P Cc: Qemu Developers, Stefan Hajnoczi, zhangboxian, Paolo Bonzini, Prasad J Pandit On Sat, 25 Nov 2017 00:04:45 +0530 P J P <ppandit@redhat.com> wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > An user could attempt to use an uninitialised VirtQueue object s/An user/A guest/ ? > or unset Vring.align leading to a arithmetic exception. Add check > to avoid it. > > Reported-by: Zhangboxian <zhangboxian@huawei.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/virtio/virtio.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 5884ce3480..c01eac87a5 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -182,7 +182,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n) > { > VRing *vring = &vdev->vq[n].vring; > > - if (!vring->desc) { > + if (!vdev->vq[n].vring.num || !vring->desc || !vring->align) { > /* not yet setup -> nothing to do */ > return; > } > @@ -1414,6 +1414,9 @@ void virtio_config_modern_writel(VirtIODevice *vdev, > > void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr) > { > + if (!vdev->vq[n].vring.num) { > + return; > + } > vdev->vq[n].vring.desc = addr; > virtio_queue_update_rings(vdev, n); > } > @@ -1426,6 +1429,9 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n) > void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc, > hwaddr avail, hwaddr used) > { > + if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) { Checking for !desc is wrong (why shouldn't a driver be able to unset a descriptor table?) The check for align is not really needed, as virtio-1 disallows setting align anyway. > + return; > + } > vdev->vq[n].vring.desc = desc; > vdev->vq[n].vring.avail = avail; > vdev->vq[n].vring.used = used; > @@ -1494,8 +1500,10 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align) > */ > assert(k->has_variable_vring_alignment); > > - vdev->vq[n].vring.align = align; > - virtio_queue_update_rings(vdev, n); > + if (align) { > + vdev->vq[n].vring.align = align; > + virtio_queue_update_rings(vdev, n); > + } > } > > static bool virtio_queue_notify_aio_vq(VirtQueue *vq) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set 2017-11-24 18:34 ` [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set P J P 2017-11-27 10:03 ` Cornelia Huck @ 2017-11-27 11:15 ` Stefan Hajnoczi 2017-11-27 17:55 ` P J P 1 sibling, 1 reply; 13+ messages in thread From: Stefan Hajnoczi @ 2017-11-27 11:15 UTC (permalink / raw) To: P J P Cc: Qemu Developers, zhangboxian, Paolo Bonzini, Cornelia Huck, Prasad J Pandit [-- Attachment #1: Type: text/plain, Size: 355 bytes --] On Sat, Nov 25, 2017 at 12:04:45AM +0530, P J P wrote: > @@ -1426,6 +1429,9 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n) > void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc, > hwaddr avail, hwaddr used) > { > + if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) { Why !desc? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set 2017-11-27 11:15 ` Stefan Hajnoczi @ 2017-11-27 17:55 ` P J P 2017-11-28 9:11 ` Cornelia Huck 0 siblings, 1 reply; 13+ messages in thread From: P J P @ 2017-11-27 17:55 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Qemu Developers, zhangboxian, Paolo Bonzini, Cornelia Huck +-- On Mon, 27 Nov 2017, Cornelia Huck wrote --+ |The check for align is not really needed, as virtio-1 disallows setting align |anyway. disallows...? | Checking for !desc is wrong (why shouldn't a driver be able to unset a | descriptor table?) +-- On Mon, 27 Nov 2017, Stefan Hajnoczi wrote --+ | > + if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) { | ... | vdev->vq[n].vring.desc = desc; | | Why !desc? virtio_queue_set_rings virtio_init_region_cache VirtQueue *vq = &vdev->vq[n]; ... addr = vq->vring.desc; if (!addr) { return; } These checks seem to be repeating all over. As mentioned earlier, could these be collated in one place, maybe virtio_queue_get_num()? int virtio_queue_get_num(VirtIODevice *vdev, int n) { VirtQueue *vq = &vdev->vq[n]; if (!vq->.vring.num || !vq->vring.desc || !vq->vring.align) { return 0; /* vq not set */ } return vdev->vq[n].vring.num; } Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set 2017-11-27 17:55 ` P J P @ 2017-11-28 9:11 ` Cornelia Huck 2017-11-28 10:37 ` Stefan Hajnoczi 0 siblings, 1 reply; 13+ messages in thread From: Cornelia Huck @ 2017-11-28 9:11 UTC (permalink / raw) To: P J P; +Cc: Stefan Hajnoczi, Qemu Developers, zhangboxian, Paolo Bonzini On Mon, 27 Nov 2017 23:25:28 +0530 (IST) P J P <ppandit@redhat.com> wrote: > +-- On Mon, 27 Nov 2017, Cornelia Huck wrote --+ > |The check for align is not really needed, as virtio-1 disallows setting align > |anyway. > > disallows...? See the check in virtio_queue_set_align(). Moreover, the calculation that breaks virtqueue setup for align == 0 is only called for the legacy setup, IOW not for this virtio-1 only function. > > | Checking for !desc is wrong (why shouldn't a driver be able to unset a > | descriptor table?) > > +-- On Mon, 27 Nov 2017, Stefan Hajnoczi wrote --+ > | > + if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) { > | ... > | vdev->vq[n].vring.desc = desc; > | > | Why !desc? > > virtio_queue_set_rings > virtio_init_region_cache > VirtQueue *vq = &vdev->vq[n]; > ... > addr = vq->vring.desc; > if (!addr) { > return; > } > > These checks seem to be repeating all over. As mentioned earlier, could these > be collated in one place, maybe virtio_queue_get_num()? > > int virtio_queue_get_num(VirtIODevice *vdev, int n) > { > VirtQueue *vq = &vdev->vq[n]; > > if (!vq->.vring.num > || !vq->vring.desc > || !vq->vring.align) { > return 0; /* vq not set */ > } > > return vdev->vq[n].vring.num; > } This is conflating different things: - vq does not exist (num == 0) - vq is not setup by the guest (desc == 0) - vq has no valid alignment (which is only relevant for legacy) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set 2017-11-28 9:11 ` Cornelia Huck @ 2017-11-28 10:37 ` Stefan Hajnoczi 2017-11-28 11:27 ` P J P 0 siblings, 1 reply; 13+ messages in thread From: Stefan Hajnoczi @ 2017-11-28 10:37 UTC (permalink / raw) To: Cornelia Huck; +Cc: P J P, Qemu Developers, zhangboxian, Paolo Bonzini [-- Attachment #1: Type: text/plain, Size: 1257 bytes --] On Tue, Nov 28, 2017 at 10:11:54AM +0100, Cornelia Huck wrote: > On Mon, 27 Nov 2017 23:25:28 +0530 (IST) > P J P <ppandit@redhat.com> wrote: > > +-- On Mon, 27 Nov 2017, Stefan Hajnoczi wrote --+ > > | > + if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) { > > | ... > > | vdev->vq[n].vring.desc = desc; > > | > > | Why !desc? > > > > virtio_queue_set_rings > > virtio_init_region_cache > > VirtQueue *vq = &vdev->vq[n]; > > ... > > addr = vq->vring.desc; > > if (!addr) { > > return; > > } > > > > These checks seem to be repeating all over. As mentioned earlier, could these > > be collated in one place, maybe virtio_queue_get_num()? > > > > int virtio_queue_get_num(VirtIODevice *vdev, int n) > > { > > VirtQueue *vq = &vdev->vq[n]; > > > > if (!vq->.vring.num > > || !vq->vring.desc > > || !vq->vring.align) { > > return 0; /* vq not set */ > > } > > > > return vdev->vq[n].vring.num; > > } > > This is conflating different things: > - vq does not exist (num == 0) > - vq is not setup by the guest (desc == 0) > - vq has no valid alignment (which is only relevant for legacy) I agree. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set 2017-11-28 10:37 ` Stefan Hajnoczi @ 2017-11-28 11:27 ` P J P 2017-11-28 12:00 ` Cornelia Huck 0 siblings, 1 reply; 13+ messages in thread From: P J P @ 2017-11-28 11:27 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Cornelia Huck, Qemu Developers, zhangboxian, Paolo Bonzini +-- On Tue, 28 Nov 2017, Stefan Hajnoczi wrote --+ | > This is conflating different things: | > - vq does not exist (num == 0) | > - vq is not setup by the guest (desc == 0) | > - vq has no valid alignment (which is only relevant for legacy) | | I agree. Either case, vq would be unfit for use, no? -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set 2017-11-28 11:27 ` P J P @ 2017-11-28 12:00 ` Cornelia Huck 2017-11-29 10:11 ` P J P 0 siblings, 1 reply; 13+ messages in thread From: Cornelia Huck @ 2017-11-28 12:00 UTC (permalink / raw) To: P J P; +Cc: Stefan Hajnoczi, Qemu Developers, zhangboxian, Paolo Bonzini On Tue, 28 Nov 2017 16:57:34 +0530 (IST) P J P <ppandit@redhat.com> wrote: > +-- On Tue, 28 Nov 2017, Stefan Hajnoczi wrote --+ > | > This is conflating different things: > | > - vq does not exist (num == 0) > | > - vq is not setup by the guest (desc == 0) > | > - vq has no valid alignment (which is only relevant for legacy) > | > | I agree. > > Either case, vq would be unfit for use, no? What is "unfit for use"? I'm not quite sure what you want to achieve with this patch. I assume you want to fix the issue that a guest may provide invalid values for align etc. which can cause qemu to crash or behave badly. If so, you need to do different things for the different points above. - The guest should not muck around with a non-existing queue (num == 0) in any case, so this should be fenced for any manipulation triggered by the guest. - Processing a non-setup queue (desc == 0; also applies to the other buffers for virtio-1) should be skipped. However, _setting_ desc etc. to 0 from the guest is fine (as long as it follows the other constraints of the spec). - Setting alignment to 0 only applies to legacy + virtio-mmio. I would not overengineer fencing this. A simple check in update_rings should be enough. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set 2017-11-28 12:00 ` Cornelia Huck @ 2017-11-29 10:11 ` P J P 2017-11-29 11:16 ` Cornelia Huck 0 siblings, 1 reply; 13+ messages in thread From: P J P @ 2017-11-29 10:11 UTC (permalink / raw) To: Cornelia Huck Cc: Stefan Hajnoczi, Qemu Developers, zhangboxian, Paolo Bonzini Hello Cornelia, +-- On Tue, 28 Nov 2017, Cornelia Huck wrote --+ | What is "unfit for use"? Unfit for use because we see checks like if (!virtio_queue_get_num(vdev, n)) { continue; ... if (!vdev->vq[n].vring.num) { return; 'virtio_queue_set_rings' sets 'vring.desc' as vdev->vq[n].vring.desc = desc; and calls virtio_init_region_cache(vdev, n); which returns if vq->vring.desc is zero(0). addr = vq->vring.desc; if (!addr) { return; } Same in virtio_queue_set_addr() -> virtio_queue_update_rings(). It seems that for 'vq' instance to be useful, vring.num, vring.desc etc. fields need to be set properly. Unless an unused/free 'vq' is being accessed to set these fields. | I'm not quite sure what you want to achieve with this patch. I assume | you want to fix the issue that a guest may provide invalid values for | align etc. which can cause qemu to crash or behave badly. True. In the process I'm trying to figure out if a usable 'vq' instance could be decided in once place, than having repeating checks, if possible. Ex. 'virtio_queue_update_rings' is called as virtio_queue_set_addr -> virtio_queue_update_rings virtio_queue_set_align -> virtio_queue_update_rings virtio_load for (i = 0; i < num; i++) { if (vdev->vq[i].vring.desc) { ... virtio_queue_update_rings Of these, virtio_load checks that 'vring.desc' is non-zero(0). Current patch adds couple checks to the other two callers above. And again, virtio_queue_update_rings would check if (!vring->num || !vring->desc || !vring->align) { /* not yet setup -> nothing to do */ return; } | If so, you need to do different things for the different points above. | - The guest should not muck around with a non-existing queue (num == 0) | in any case, so this should be fenced for any manipulation triggered | by the guest. I guess done by !virtio_queue_get_num() check above? | - Processing a non-setup queue (desc == 0; also applies to the other | buffers for virtio-1) should be skipped. However, _setting_ desc etc. | to 0 from the guest is fine (as long as it follows the other | constraints of the spec). Okay. Though its non-zero(0) value is preferred? | - Setting alignment to 0 only applies to legacy + virtio-mmio. I would | not overengineer fencing this. A simple check in update_rings should | be enough. Okay.x Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set 2017-11-29 10:11 ` P J P @ 2017-11-29 11:16 ` Cornelia Huck 2017-11-30 9:16 ` P J P 0 siblings, 1 reply; 13+ messages in thread From: Cornelia Huck @ 2017-11-29 11:16 UTC (permalink / raw) To: P J P; +Cc: Stefan Hajnoczi, Qemu Developers, zhangboxian, Paolo Bonzini On Wed, 29 Nov 2017 15:41:45 +0530 (IST) P J P <ppandit@redhat.com> wrote: > Hello Cornelia, > > +-- On Tue, 28 Nov 2017, Cornelia Huck wrote --+ > | What is "unfit for use"? > > Unfit for use because we see checks like > > if (!virtio_queue_get_num(vdev, n)) { > continue; > ... > if (!vdev->vq[n].vring.num) { > return; > > 'virtio_queue_set_rings' sets 'vring.desc' as > > vdev->vq[n].vring.desc = desc; > > and calls virtio_init_region_cache(vdev, n); > which returns if vq->vring.desc is zero(0). > > addr = vq->vring.desc; > if (!addr) { > return; > } > > Same in virtio_queue_set_addr() -> virtio_queue_update_rings(). > > It seems that for 'vq' instance to be useful, vring.num, vring.desc etc. > fields need to be set properly. Unless an unused/free 'vq' is being accessed > to set these fields. I think the basic problem is still that you conflate two things: - vring.num, which cannot be flipped between 0 and !0 by the guest - vring.{desc,avail,used}, which can IOW, if vring.num == 0, the guest cannot manipulate the queue; if vring.desc == 0, the guest can. > > | I'm not quite sure what you want to achieve with this patch. I assume > | you want to fix the issue that a guest may provide invalid values for > | align etc. which can cause qemu to crash or behave badly. > > True. In the process I'm trying to figure out if a usable 'vq' instance could > be decided in once place, than having repeating checks, if possible. > > Ex. 'virtio_queue_update_rings' is called as > > virtio_queue_set_addr > -> virtio_queue_update_rings > > virtio_queue_set_align > -> virtio_queue_update_rings > > virtio_load > for (i = 0; i < num; i++) { > if (vdev->vq[i].vring.desc) { > ... > virtio_queue_update_rings > > Of these, virtio_load checks that 'vring.desc' is non-zero(0). Current > patch adds couple checks to the other two callers above. And again, > > virtio_queue_update_rings would check > > if (!vring->num || !vring->desc || !vring->align) { > /* not yet setup -> nothing to do */ > return; > } vring.num and vring.desc are really different things. You don't want the guest to do anything with the queue if vring.num == 0, while you just want to skip various processing if vring.desc == 0. (virtio_load() does not need to care about vring.num, as it is not triggered by the guest.) > > | If so, you need to do different things for the different points above. > | - The guest should not muck around with a non-existing queue (num == 0) > | in any case, so this should be fenced for any manipulation triggered > | by the guest. > > I guess done by !virtio_queue_get_num() check above? Yes. > > | - Processing a non-setup queue (desc == 0; also applies to the other > | buffers for virtio-1) should be skipped. However, _setting_ desc etc. > | to 0 from the guest is fine (as long as it follows the other > | constraints of the spec). > > Okay. Though its non-zero(0) value is preferred? Many functions have a likely/unlikely check, setup routines excepted. > > | - Setting alignment to 0 only applies to legacy + virtio-mmio. I would > | not overengineer fencing this. A simple check in update_rings should > | be enough. > > Okay.x ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set 2017-11-29 11:16 ` Cornelia Huck @ 2017-11-30 9:16 ` P J P 0 siblings, 0 replies; 13+ messages in thread From: P J P @ 2017-11-30 9:16 UTC (permalink / raw) To: Cornelia Huck Cc: Stefan Hajnoczi, Qemu Developers, zhangboxian, Paolo Bonzini +-- On Wed, 29 Nov 2017, Cornelia Huck wrote --+ | I think the basic problem is still that you conflate two things: | - vring.num, which cannot be flipped between 0 and !0 by the guest | - vring.{desc,avail,used}, which can | | IOW, if vring.num == 0, the guest cannot manipulate the queue; if | vring.desc == 0, the guest can. | | Many functions have a likely/unlikely check, setup routines excepted. Have sent a revised patch v4. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v3 2/2] tests: add test to check VirtQueue object 2017-11-24 18:34 [Qemu-devel] [PATCH v3 0/2] check VirtiQueue Vring objects P J P 2017-11-24 18:34 ` [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set P J P @ 2017-11-24 18:34 ` P J P 1 sibling, 0 replies; 13+ messages in thread From: P J P @ 2017-11-24 18:34 UTC (permalink / raw) To: Qemu Developers Cc: Stefan Hajnoczi, zhangboxian, Paolo Bonzini, Cornelia Huck, Prasad J Pandit From: Prasad J Pandit <pjp@fedoraproject.org> An uninitialised VirtQueue object or one with Vring.align field set to zero(0) could lead to arithmetic exceptions. Add a unit test to validate it. Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> --- tests/virtio-blk-test.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c index e6fb9bac87..45f368dcd9 100644 --- a/tests/virtio-blk-test.c +++ b/tests/virtio-blk-test.c @@ -674,6 +674,30 @@ static void pci_hotplug(void) qtest_shutdown(qs); } +/* + * Check that setting the vring addr on a non-existent virtqueue does + * not crash. + */ +static void test_nonexistent_virtqueue(void) +{ + QPCIBar bar0; + QOSState *qs; + QPCIDevice *dev; + + qs = pci_test_start(); + dev = qpci_device_find(qs->pcibus, QPCI_DEVFN(4, 0)); + g_assert(dev != NULL); + + qpci_device_enable(dev); + bar0 = qpci_iomap(dev, 0, NULL); + + qpci_io_writeb(dev, bar0, VIRTIO_PCI_QUEUE_SEL, 2); + qpci_io_writel(dev, bar0, VIRTIO_PCI_QUEUE_PFN, 1); + + g_free(dev); + qtest_shutdown(qs); +} + static void mmio_basic(void) { QVirtioMMIODevice *dev; @@ -724,6 +748,7 @@ int main(int argc, char **argv) qtest_add_func("/virtio/blk/pci/basic", pci_basic); qtest_add_func("/virtio/blk/pci/indirect", pci_indirect); qtest_add_func("/virtio/blk/pci/config", pci_config); + qtest_add_func("/virtio/blk/pci/nxvirtq", test_nonexistent_virtqueue); if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { qtest_add_func("/virtio/blk/pci/msix", pci_msix); qtest_add_func("/virtio/blk/pci/idx", pci_idx); -- 2.13.6 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-11-30 9:16 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-24 18:34 [Qemu-devel] [PATCH v3 0/2] check VirtiQueue Vring objects P J P 2017-11-24 18:34 ` [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set P J P 2017-11-27 10:03 ` Cornelia Huck 2017-11-27 11:15 ` Stefan Hajnoczi 2017-11-27 17:55 ` P J P 2017-11-28 9:11 ` Cornelia Huck 2017-11-28 10:37 ` Stefan Hajnoczi 2017-11-28 11:27 ` P J P 2017-11-28 12:00 ` Cornelia Huck 2017-11-29 10:11 ` P J P 2017-11-29 11:16 ` Cornelia Huck 2017-11-30 9:16 ` P J P 2017-11-24 18:34 ` [Qemu-devel] [PATCH v3 2/2] tests: add test to check VirtQueue object P J P
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.