linux-kernel - Re: [PATCH 1/1] vfio iommu type1: set size to PAGE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161205155421.075bbec5@t450s.home>
Date:   Mon, 5 Dec 2016 15:54:21 -0700
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Kirti Wankhede <kwankhede@...dia.com>
Cc:     <pbonzini@...hat.com>, <kraxel@...hat.com>, <cjia@...dia.com>,
        <qemu-devel@...gnu.org>, <kvm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] vfio iommu type1: set size to PAGE_SIZE while
 looking for iova in dma list

On Tue, 6 Dec 2016 02:38:01 +0530
Kirti Wankhede <kwankhede@...dia.com> wrote:

> In the functions of pin_pages/unpin_pages from mdev vendor driver,
> vfio_find_dma() should be called with size as PAGE_SIZE instead of 0.
> vfio_find_dma() searches for the range in dma_list.
> 
> In vfio_dma_do_unmap(), vfio_find_dma() when used to look for start
> address of unmap->iova, size should be 1, not 0. Otherwise vfio_find_dma()
> returns NULL.

Hi Kirti,

I'd prefer to fix the preexisting case of this issue as a separate
patch, we might want to backport it for stable trees separately from the
new cases introduced with mdev.  It would also be great if we could go
into some detail about why size=0 doesn't work in these cases, ie.
vfio_find_dma() accounting for the end address of the range makes it
fall through at the wrong branch point.  For the case of the
preexisting issue, it would also be useful to mention that the other
case of passing size=0 works correctly due to the -1 in the start
address calculation.  Finally, I think I agree with your choice to use
PAGE_SIZE in one case and 1 in the preexisting case, but perhaps we
could spell that out as one is related to a page size interface and the
other is trying to test boundary conditions.  Thanks,

Alex

> Signed-off-by: Kirti Wankhede <kwankhede@...dia.com>
> Signed-off-by: Neo Jia <cjia@...dia.com>
> Change-Id: Iee6fd45441c342b5e8626087046b2e0075d19a08
> ---
>  drivers/vfio/vfio_iommu_type1.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index a28fbddb505c..023fba7b8d5a 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -581,7 +581,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>  		struct vfio_pfn *vpfn;
>  
>  		iova = user_pfn[i] << PAGE_SHIFT;
> -		dma = vfio_find_dma(iommu, iova, 0);
> +		dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
>  		if (!dma) {
>  			ret = -EINVAL;
>  			goto pin_unwind;
> @@ -622,7 +622,7 @@ pin_unwind:
>  		dma_addr_t iova;
>  
>  		iova = user_pfn[j] << PAGE_SHIFT;
> -		dma = vfio_find_dma(iommu, iova, 0);
> +		dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
>  		vfio_unpin_page_external(dma, iova, do_accounting);
>  		phys_pfn[j] = 0;
>  	}
> @@ -659,7 +659,7 @@ static int vfio_iommu_type1_unpin_pages(void *iommu_data,
>  		dma_addr_t iova;
>  
>  		iova = user_pfn[i] << PAGE_SHIFT;
> -		dma = vfio_find_dma(iommu, iova, 0);
> +		dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
>  		if (!dma)
>  			goto unpin_exit;
>  		vfio_unpin_page_external(dma, iova, do_accounting);
> @@ -826,7 +826,7 @@ again:
>  	 * mappings within the range.
>  	 */
>  	if (iommu->v2) {
> -		dma = vfio_find_dma(iommu, unmap->iova, 0);
> +		dma = vfio_find_dma(iommu, unmap->iova, 1);
>  		if (dma && dma->iova != unmap->iova) {
>  			ret = -EINVAL;
>  			goto unlock;