Linux Virtualization - Jan 2017 - [PATCH] virtio_blk: fix panic in initialization error path

From: Omar Sandoval <osandov at fb.com>

If blk_mq_init_queue() returns an error, it gets assigned to
vblk->disk->queue. Then, when we call put_disk(), we end up calling
blk_put_queue() with the ERR_PTR, causing a bad dereference. Fix it by
only assigning to vblk->disk->queue on success.

Signed-off-by: Omar Sandoval <osandov at fb.com>
---
 drivers/block/virtio_blk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 5545a679abd8..8587361e5356 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -628,11 +628,12 @@ static int virtblk_probe(struct virtio_device *vdev)
 	if (err)
 		goto out_put_disk;
 
-	q = vblk->disk->queue = blk_mq_init_queue(&vblk->tag_set);
+	q = blk_mq_init_queue(&vblk->tag_set);
 	if (IS_ERR(q)) {
 		err = -ENOMEM;
 		goto out_free_tags;
 	}
+	vblk->disk->queue = q;
 
 	q->queuedata = vblk;
 
-- 
2.11.0

Omar Sandoval <osandov at osandov.com> writes:
> From: Omar Sandoval <osandov at fb.com>
>
> If blk_mq_init_queue() returns an error, it gets assigned to
> vblk->disk->queue. Then, when we call put_disk(), we end up calling
> blk_put_queue() with the ERR_PTR, causing a bad dereference. Fix it by
> only assigning to vblk->disk->queue on success.
>
> Signed-off-by: Omar Sandoval <osandov at fb.com>
Reviewed-by: Jeff Moyer <jmoyer at redhat.com>

On 2017?01?10? 03:44, Omar Sandoval wrote:
> From: Omar Sandoval <osandov at fb.com>
>
> If blk_mq_init_queue() returns an error, it gets assigned to
> vblk->disk->queue. Then, when we call put_disk(), we end up calling
> blk_put_queue() with the ERR_PTR, causing a bad dereference. Fix it by
> only assigning to vblk->disk->queue on success.
>
> Signed-off-by: Omar Sandoval <osandov at fb.com>
> ---
>   drivers/block/virtio_blk.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 5545a679abd8..8587361e5356 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -628,11 +628,12 @@ static int virtblk_probe(struct virtio_device *vdev)
>   	if (err)
>   		goto out_put_disk;
>   
> -	q = vblk->disk->queue = blk_mq_init_queue(&vblk->tag_set);
> +	q = blk_mq_init_queue(&vblk->tag_set);
>   	if (IS_ERR(q)) {
>   		err = -ENOMEM;
>   		goto out_free_tags;
>   	}
> +	vblk->disk->queue = q;
>   
>   	q->queuedata = vblk;
>   
Acked-by: Jason Wang <jasowang at redhat.com>

On Mon, Jan 09, 2017 at 11:44:12AM -0800, Omar Sandoval
wrote:
> From: Omar Sandoval <osandov at fb.com>
> 
> If blk_mq_init_queue() returns an error, it gets assigned to
> vblk->disk->queue. Then, when we call put_disk(), we end up calling
> blk_put_queue() with the ERR_PTR, causing a bad dereference. Fix it by
> only assigning to vblk->disk->queue on success.
> 
> Signed-off-by: Omar Sandoval <osandov at fb.com>
Acked-by: Michael S. Tsirkin <mst at redhat.com>

Jens, do you mind picking this one up as well, since
you have one virtio-blk patch already?

> ---
>  drivers/block/virtio_blk.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 5545a679abd8..8587361e5356 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -628,11 +628,12 @@ static int virtblk_probe(struct virtio_device *vdev)
>  	if (err)
>  		goto out_put_disk;
>  
> -	q = vblk->disk->queue = blk_mq_init_queue(&vblk->tag_set);
> +	q = blk_mq_init_queue(&vblk->tag_set);
>  	if (IS_ERR(q)) {
>  		err = -ENOMEM;
>  		goto out_free_tags;
>  	}
> +	vblk->disk->queue = q;
>  
>  	q->queuedata = vblk;
>  
> -- 
> 2.11.0

On 01/09/2017 09:10 PM, Michael S. Tsirkin wrote:
> On Mon, Jan 09, 2017 at 11:44:12AM -0800, Omar Sandoval wrote:
>> From: Omar Sandoval <osandov at fb.com>
>>
>> If blk_mq_init_queue() returns an error, it gets assigned to
>> vblk->disk->queue. Then, when we call put_disk(), we end up
calling
>> blk_put_queue() with the ERR_PTR, causing a bad dereference. Fix it by
>> only assigning to vblk->disk->queue on success.
>>
>> Signed-off-by: Omar Sandoval <osandov at fb.com>
> 
> Acked-by: Michael S. Tsirkin <mst at redhat.com>
> 
> Jens, do you mind picking this one up as well, since
> you have one virtio-blk patch already?
No problem, in fact I already queued it up.

-- 
Jens Axboe