On Wed, 2 Jan 2019 19:25:49 +0100
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
> On Wed, 2 Jan 2019 18:50:20 +0100
> Halil Pasic <pasic@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> >
> > 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) {
>
