Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v1 3/8] virtio: Harden split buffer detachment
To:     Andi Kleen <ak@linux.intel.com>, mst@redhat.com
Cc:     virtualization@lists.linux-foundation.org, hch@lst.de,
        m.szyprowski@samsung.com, robin.murphy@arm.com,
        iommu@lists.linux-foundation.org, x86@kernel.org,
        sathyanarayanan.kuppuswamy@linux.intel.com, jpoimboe@redhat.com,
        linux-kernel@vger.kernel.org
References: <20210603004133.4079390-1-ak@linux.intel.com>
 <20210603004133.4079390-4-ak@linux.intel.com>
From:   Jason Wang <jasowang@redhat.com>
Message-ID: <284ca65d-d8b4-a671-4dba-df478a3610f1@redhat.com>
Date:   Thu, 3 Jun 2021 10:29:08 +0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.10.2
MIME-Version: 1.0
In-Reply-To: <20210603004133.4079390-4-ak@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


在 2021/6/3 上午8:41, Andi Kleen 写道:
> Harden the split buffer detachment path by adding boundary checking. Note
> that when this fails we may fail to unmap some swiotlb mapping, which could
> result in a leak and a DOS. But that's acceptable because an malicious host
> can DOS us anyways.
>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>   drivers/virtio/virtio_ring.c | 25 +++++++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index d37ff5a0ff58..1e9aa1e95e1b 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -651,12 +651,19 @@ static bool virtqueue_kick_prepare_split(struct virtqueue *_vq)
>   	return needs_kick;
>   }
>   
> -static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> -			     void **ctx)
> +static int detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> +			    void **ctx)
>   {
>   	unsigned int i, j;
>   	__virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
>   
> +	/* We'll leak DMA mappings when this happens, but nothing
> +	 * can be done about that. In the worst case the host
> +	 * could DOS us, but it can of course do that anyways.
> +	 */
> +	if (!inside_split_ring(vq, head))
> +		return -EIO;


I think the caller have already did this for us with even more check on 
the token (virtqueue_get_buf_ctx_split()):

         if (unlikely(i >= vq->split.vring.num)) {
                 BAD_RING(vq, "id %u out of range\n", i);
                 return NULL;
         }
         if (unlikely(!vq->split.desc_state[i].data)) {
                 BAD_RING(vq, "id %u is not a head!\n", i);
                 return NULL;
         }


> +
>   	/* Clear data ptr. */
>   	vq->split.desc_state[head].data = NULL;
>   
> @@ -666,6 +673,8 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
>   	while (vq->split.vring.desc[i].flags & nextflag) {
>   		vring_unmap_one_split(vq, &vq->split.vring.desc[i]);
>   		i = virtio16_to_cpu(vq->vq.vdev, vq->split.vring.desc[i].next);
> +		if (!inside_split_ring(vq, i))
> +			return -EIO;


Similarly, if we don't depend on the metadata stored in the descriptor, 
we don't need this check.


>   		vq->vq.num_free++;
>   	}
>   
> @@ -684,7 +693,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
>   
>   		/* Free the indirect table, if any, now that it's unmapped. */
>   		if (!indir_desc)
> -			return;
> +			return 0;
>   
>   		len = virtio32_to_cpu(vq->vq.vdev,
>   				vq->split.vring.desc[head].len);
> @@ -701,6 +710,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
>   	} else if (ctx) {
>   		*ctx = vq->split.desc_state[head].indir_desc;
>   	}
> +	return 0;
>   }
>   
>   static inline bool more_used_split(const struct vring_virtqueue *vq)
> @@ -717,6 +727,7 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
>   	void *ret;
>   	unsigned int i;
>   	u16 last_used;
> +	int err;
>   
>   	START_USE(vq);
>   
> @@ -751,7 +762,12 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
>   
>   	/* detach_buf_split clears data, so grab it now. */
>   	ret = vq->split.desc_state[i].data;
> -	detach_buf_split(vq, i, ctx);
> +	err = detach_buf_split(vq, i, ctx);
> +	if (err) {
> +		END_USE(vq);


This reminds me that we don't use END_USE() after BAD_RING() which 
should be fixed.

Thanks


> +		return NULL;
> +	}
> +
>   	vq->last_used_idx++;
>   	/* If we expect an interrupt for the next entry, tell host
>   	 * by writing event index and flush out the write before
> @@ -863,6 +879,7 @@ static void *virtqueue_detach_unused_buf_split(struct virtqueue *_vq)
>   		/* detach_buf_split clears data, so grab it now. */
>   		buf = vq->split.desc_state[i].data;
>   		detach_buf_split(vq, i, NULL);
> +		/* Don't need to check for error because nothing is returned */
>   		vq->split.avail_idx_shadow--;
>   		vq->split.vring.avail->idx = cpu_to_virtio16(_vq->vdev,
>   				vq->split.avail_idx_shadow);