Re: [PATCH] virtio_ring: fix unmap of indirect descriptors

Re: [PATCH] virtio_ring: fix unmap of indirect descriptors

[Michael S. Tsirkin]

On Fri, Sep 06, 2019 at 02:06:59PM +0200, Matthias Lange wrote:
> The function virtqueue_add_split() DMA-maps the scatterlist buffers. In
> case a mapping error occurs the already mapped buffers must be unmapped.
> This happens by jumping to the 'unmap_release' label.
> 
> In case of indirect descriptors the release is wrong and may leak kernel
> memory. Because the implementation assumes that the head descriptor is
> already mapped it starts iterating over the descriptor list starting
> from the head descriptor. However for indirect descriptors the head
> descriptor is never mapped in case of an error.
> 
> The fix is to initialize the start index with zero in case of indirect
> descriptors and use the 'desc' pointer directly for iterating over the
> descriptor chain.
> 
> This fix also changes the return code from EIO to ENOSPC in case of a
> mapping error. The reason is that drivers such as virtio_blk may retry
> their request and thus can recover from a mapping error.
> 
> Signed-off-by: Matthias Lange <matthias.lange@kernkonzept.com>

Thanks for the patch!

However please, split this to 2 patches.
I think the 1st patch at least is a stable candidate.

About the return code part - could you please add a bit more explanation
in the commit log? E.g. document when does it trigger and what is the
effect of the fix.  Thanks!

> ---
>  drivers/virtio/virtio_ring.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index c8be1c4f5b55..d2e001f92e6e 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -566,13 +566,17 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>  
>  unmap_release:
>  	err_idx = i;
> -	i = head;
> +
> +	if (indirect)
> +		i = 0;
> +	else
> +		i = head;
>  
>  	for (n = 0; n < total_sg; n++) {
>  		if (i == err_idx)
>  			break;
>  		vring_unmap_one_split(vq, &desc[i]);
> -		i = virtio16_to_cpu(_vq->vdev, vq->split.vring.desc[i].next);
> +		i = virtio16_to_cpu(_vq->vdev, desc[i].next);
>  	}
>  
>  	if (indirect)
> @@ -1081,7 +1085,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  	kfree(desc);
>  
>  	END_USE(vq);
> -	return -EIO;
> +	return -ENOSPC;
>  }
>  
>  static inline int virtqueue_add_packed(struct virtqueue *_vq,
> -- 
> 2.17.1