[RFC,1/1] s390/virtio: handle find on invalid queue gracefully
diff mbox series

Message ID 20190102175020.45251-1-pasic@linux.ibm.com
State New
Headers show
Series
  • [RFC,1/1] s390/virtio: handle find on invalid queue gracefully
Related show

Commit Message

Halil Pasic Jan. 2, 2019, 5:50 p.m. UTC
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(+)

Comments

Cornelia Huck Jan. 2, 2019, 6:25 p.m. UTC | #1
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) {
Halil Pasic Jan. 3, 2019, 1 p.m. UTC | #2
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) {
>
Cornelia Huck Jan. 4, 2019, 2:01 p.m. UTC | #3
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 :)

Patch
diff mbox series

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) {