| Message ID | 20171124183446.7308-2-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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)
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?
+-- 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
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)
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
+-- 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
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.
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
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
+-- 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
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)