| Message ID | 20190102175020.45251-1-pasic@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/1] s390/virtio: handle find on invalid queue gracefully | expand |
On Wed, 2 Jan 2019 18:50:20 +0100 Halil Pasic <pasic@linux.ibm.com> wrote: > A queue with a capacity of zero is clearly not a valid virtio queue. > Some emulators report zero queue size if queried with an invalid queue > index. Instead of crashing in this case let us just return -EINVAL. To > make that work properly, let us fix the notifier cleanup logic as well. > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > --- > > This patch is motivated by commit 86a5597 "virtio-balloon: > VIRTIO_BALLOON_F_FREE_PAGE_HINT" (Wei Wang, 2018-08-27) which triggered > the described scenario. The emulator in question is the current QEMU. > The problem we run into is the underflow in the following loop > in __vring_new_virtqueue(): > for (i = 0; i < vring.num-1; i++) > vq->vring.desc[i].next = cpu_to_virtio16(vdev, i + 1) > Namely vring.num is an unsigned int. > > RFC because I'm not sure about -EINVAL being a good choice, and about > us caring about what happens if a virtio driver misbehaves like described. For virtio-pci, the spec says that a zero queue size means that the queue is unavailable. I don't think we have specified that explicitly for virtio-ccw, but it does make sense. virtio-pci returns -ENOENT in that case, which might be a good choice here as well. > > --- > drivers/s390/virtio/virtio_ccw.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index fc9dbad476c0..147927ed4fca 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -272,6 +272,8 @@ static void virtio_ccw_drop_indicators(struct virtio_ccw_device *vcdev) > { > struct virtio_ccw_vq_info *info; > > + if (!vcdev->airq_info) > + return; Which case is this guarding against? names[i] was NULL for every index? > list_for_each_entry(info, &vcdev->virtqueues, node) > drop_airq_indicator(info->vq, vcdev->airq_info); > } > @@ -514,6 +516,10 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > err = info->num; > goto out_err; > } > + if (info->num == 0) { > + err = -EINVAL; > + goto out_err; > + } > size = PAGE_ALIGN(vring_size(info->num, KVM_VIRTIO_CCW_RING_ALIGN)); > info->queue = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO); > if (info->queue == NULL) {
On Wed, 2 Jan 2019 19:25:49 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On Wed, 2 Jan 2019 18:50:20 +0100 > Halil Pasic <pasic@linux.ibm.com> wrote: > > > A queue with a capacity of zero is clearly not a valid virtio queue. > > Some emulators report zero queue size if queried with an invalid queue > > index. Instead of crashing in this case let us just return -EINVAL. To > > make that work properly, let us fix the notifier cleanup logic as well. > > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > > --- > > > > This patch is motivated by commit 86a5597 "virtio-balloon: > > VIRTIO_BALLOON_F_FREE_PAGE_HINT" (Wei Wang, 2018-08-27) which triggered > > the described scenario. The emulator in question is the current QEMU. > > The problem we run into is the underflow in the following loop > > in __vring_new_virtqueue(): > > for (i = 0; i < vring.num-1; i++) > > vq->vring.desc[i].next = cpu_to_virtio16(vdev, i + 1) > > Namely vring.num is an unsigned int. > > > > RFC because I'm not sure about -EINVAL being a good choice, and about > > us caring about what happens if a virtio driver misbehaves like described. > > For virtio-pci, the spec says that a zero queue size means that the > queue is unavailable. I don't think we have specified that explicitly > for virtio-ccw, but it does make sense. > > virtio-pci returns -ENOENT in that case, which might be a good choice > here as well. virtio-mmio does the same. I will change it to -ENOENT. Maybe also do something like int virtio_ccw_read_vq_conf() { [..] return vcdev->config_block->num ?: -ENOENT; } instead of the extra if statement, or? > > > > > --- > > drivers/s390/virtio/virtio_ccw.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > index fc9dbad476c0..147927ed4fca 100644 > > --- a/drivers/s390/virtio/virtio_ccw.c > > +++ b/drivers/s390/virtio/virtio_ccw.c > > @@ -272,6 +272,8 @@ static void virtio_ccw_drop_indicators(struct virtio_ccw_device *vcdev) > > { > > struct virtio_ccw_vq_info *info; > > > > + if (!vcdev->airq_info) > > + return; > > Which case is this guarding against? names[i] was NULL for every index? > Consider: static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs, struct virtqueue *vqs[], vq_callback_t *callbacks[], const char * const names[], const bool *ctx, struct irq_affinity *desc) { struct virtio_ccw_device *vcdev = to_vc_device(vdev); unsigned long *indicatorp = NULL; int ret, i; struct ccw1 *ccw; ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL); if (!ccw) return -ENOMEM; for (i = 0; i < nvqs; ++i) { vqs[i] = virtio_ccw_setup_vq(vdev, i, callbacks[i], names[i], ctx ? ctx[i] : false, ccw); if (IS_ERR(vqs[i])) { ret = PTR_ERR(vqs[i]); vqs[i] = NULL; goto out; } } [..] if (vcdev->is_thinint) { ret = virtio_ccw_register_adapter_ind(vcdev, vqs, nvqs, ccw); if (ret) /* no error, just fall back to legacy interrupts */ vcdev->is_thinint = false; } [..] out: kfree(indicatorp); kfree(ccw); virtio_ccw_del_vqs(vdev); return ret; } when the loop that calls virtio_ccw_setup_vq() fails after a couple of iterations. We end up with some queues in vcdev->virtqueues but with virtio_ccw_register_adapter_ind() never called and thus with vcdev->airq_info never set. So when virtio_ccw_del_vqs() tries to clean up we get an invalid pointer dereference. Does that answer your question? I don't quite get your comments about names[i] == NULL. Thanks, Halil > > list_for_each_entry(info, &vcdev->virtqueues, node) > > drop_airq_indicator(info->vq, vcdev->airq_info); > > } > > @@ -514,6 +516,10 @@ static struct virtqueue > > *virtio_ccw_setup_vq(struct virtio_device *vdev, err = info->num; > > goto out_err; > > } > > + if (info->num == 0) { > > + err = -EINVAL; > > + goto out_err; > > + } > > size = PAGE_ALIGN(vring_size(info->num, > > KVM_VIRTIO_CCW_RING_ALIGN)); info->queue = alloc_pages_exact(size, > > GFP_KERNEL | __GFP_ZERO); if (info->queue == NULL) { >
On Thu, 3 Jan 2019 14:00:10 +0100 Halil Pasic <pasic@linux.ibm.com> wrote: > On Wed, 2 Jan 2019 19:25:49 +0100 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Wed, 2 Jan 2019 18:50:20 +0100 > > Halil Pasic <pasic@linux.ibm.com> wrote: > > > > > A queue with a capacity of zero is clearly not a valid virtio queue. > > > Some emulators report zero queue size if queried with an invalid queue > > > index. Instead of crashing in this case let us just return -EINVAL. To > > > make that work properly, let us fix the notifier cleanup logic as well. > > > > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com> > > > --- > > > > > > This patch is motivated by commit 86a5597 "virtio-balloon: > > > VIRTIO_BALLOON_F_FREE_PAGE_HINT" (Wei Wang, 2018-08-27) which triggered > > > the described scenario. The emulator in question is the current QEMU. > > > The problem we run into is the underflow in the following loop > > > in __vring_new_virtqueue(): > > > for (i = 0; i < vring.num-1; i++) > > > vq->vring.desc[i].next = cpu_to_virtio16(vdev, i + 1) > > > Namely vring.num is an unsigned int. > > > > > > RFC because I'm not sure about -EINVAL being a good choice, and about > > > us caring about what happens if a virtio driver misbehaves like described. > > > > For virtio-pci, the spec says that a zero queue size means that the > > queue is unavailable. I don't think we have specified that explicitly > > for virtio-ccw, but it does make sense. > > > > virtio-pci returns -ENOENT in that case, which might be a good choice > > here as well. > > virtio-mmio does the same. I will change it to -ENOENT. Maybe also do > something like > int virtio_ccw_read_vq_conf() { > [..] > return vcdev->config_block->num ?: -ENOENT; > } > > instead of the extra if statement, or? Yes, why not. > > > > > > > > > --- > > > drivers/s390/virtio/virtio_ccw.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > > index fc9dbad476c0..147927ed4fca 100644 > > > --- a/drivers/s390/virtio/virtio_ccw.c > > > +++ b/drivers/s390/virtio/virtio_ccw.c > > > @@ -272,6 +272,8 @@ static void virtio_ccw_drop_indicators(struct virtio_ccw_device *vcdev) > > > { > > > struct virtio_ccw_vq_info *info; > > > > > > + if (!vcdev->airq_info) > > > + return; > > > > Which case is this guarding against? names[i] was NULL for every index? > > > > Consider: > static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs, > struct virtqueue *vqs[], > vq_callback_t *callbacks[], > const char * const names[], > const bool *ctx, > struct irq_affinity *desc) > { > struct virtio_ccw_device *vcdev = to_vc_device(vdev); > unsigned long *indicatorp = NULL; > int ret, i; > struct ccw1 *ccw; > > ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL); > if (!ccw) > return -ENOMEM; > > for (i = 0; i < nvqs; ++i) { > vqs[i] = virtio_ccw_setup_vq(vdev, i, callbacks[i], names[i], > ctx ? ctx[i] : false, ccw); > if (IS_ERR(vqs[i])) { > ret = PTR_ERR(vqs[i]); > vqs[i] = NULL; > goto out; > } > } > [..] > if (vcdev->is_thinint) { > ret = virtio_ccw_register_adapter_ind(vcdev, vqs, nvqs, ccw); > if (ret) > /* no error, just fall back to legacy interrupts */ > vcdev->is_thinint = false; > } > [..] > out: > kfree(indicatorp); > kfree(ccw); > virtio_ccw_del_vqs(vdev); > return ret; > } > when the loop that calls virtio_ccw_setup_vq() fails after a couple > of iterations. We end up with some queues in vcdev->virtqueues but > with virtio_ccw_register_adapter_ind() never called and thus with > vcdev->airq_info never set. So when virtio_ccw_del_vqs() tries to clean > up we get an invalid pointer dereference. > > Does that answer your question? Yes, thanks. > > I don't quite get your comments about names[i] == NULL. I was looking at a tree with "virtio: don't allocate vqs when names[i] = NULL" applied :)
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index fc9dbad476c0..147927ed4fca 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -272,6 +272,8 @@ static void virtio_ccw_drop_indicators(struct virtio_ccw_device *vcdev) { struct virtio_ccw_vq_info *info; + if (!vcdev->airq_info) + return; list_for_each_entry(info, &vcdev->virtqueues, node) drop_airq_indicator(info->vq, vcdev->airq_info); } @@ -514,6 +516,10 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, err = info->num; goto out_err; } + if (info->num == 0) { + err = -EINVAL; + goto out_err; + } size = PAGE_ALIGN(vring_size(info->num, KVM_VIRTIO_CCW_RING_ALIGN)); info->queue = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO); if (info->queue == NULL) {
A queue with a capacity of zero is clearly not a valid virtio queue. Some emulators report zero queue size if queried with an invalid queue index. Instead of crashing in this case let us just return -EINVAL. To make that work properly, let us fix the notifier cleanup logic as well. Signed-off-by: Halil Pasic <pasic@linux.ibm.com> --- This patch is motivated by commit 86a5597 "virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT" (Wei Wang, 2018-08-27) which triggered the described scenario. The emulator in question is the current QEMU. The problem we run into is the underflow in the following loop in __vring_new_virtqueue(): for (i = 0; i < vring.num-1; i++) vq->vring.desc[i].next = cpu_to_virtio16(vdev, i + 1) Namely vring.num is an unsigned int. RFC because I'm not sure about -EINVAL being a good choice, and about us caring about what happens if a virtio driver misbehaves like described. --- drivers/s390/virtio/virtio_ccw.c | 6 ++++++ 1 file changed, 6 insertions(+)