linux-kernel - Re: [PATCH v2] vfio/type1: check pfn valid before converting to struct 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 for Android: free password hash cracker in your pocket

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

Message-ID: <20230522130030.44c6c5c2.alex.williamson@redhat.com>
Date:   Mon, 22 May 2023 13:00:30 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Yan Zhao <yan.y.zhao@...el.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        kevin.tian@...el.com, jgg@...dia.com,
        Sean Christopherson <seanjc@...gle.com>
Subject: Re: [PATCH v2] vfio/type1: check pfn valid before converting to
 struct page

On Fri, 19 May 2023 14:58:43 +0800
Yan Zhao <yan.y.zhao@...el.com> wrote:

> Check physical PFN is valid before converting the PFN to a struct page
> pointer to be returned to caller of vfio_pin_pages().
> 
> vfio_pin_pages() pins user pages with contiguous IOVA.
> If the IOVA of a user page to be pinned belongs to vma of vm_flags
> VM_PFNMAP, pin_user_pages_remote() will return -EFAULT without returning
> struct page address for this PFN. This is because usually this kind of PFN
> (e.g. MMIO PFN) has no valid struct page address associated.
> Upon this error, vaddr_get_pfns() will obtain the physical PFN directly.
> 
> While previously vfio_pin_pages() returns to caller PFN arrays directly,
> after commit
> 34a255e67615 ("vfio: Replace phys_pfn with pages for vfio_pin_pages()"),
> PFNs will be converted to "struct page *" unconditionally and therefore
> the returned "struct page *" array may contain invalid struct page
> addresses.
> 
> Given current in-tree users of vfio_pin_pages() only expect "struct page *
> returned, check PFN validity and return -EINVAL to let the caller be
> aware of IOVAs to be pinned containing PFN not able to be returned in
> "struct page *" array. So that, the caller will not consume the returned
> pointer (e.g. test PageReserved()) and avoid error like "supervisor read
> access in kernel mode".
> 
> Fixes: 34a255e67615 ("vfio: Replace phys_pfn with pages for vfio_pin_pages()")
> Cc: Sean Christopherson <seanjc@...gle.com>
> Reviewed-by: Jason Gunthorpe <jgg@...dia.com>
> Signed-off-by: Yan Zhao <yan.y.zhao@...el.com>
> 
> ---
> v2: update commit message to explain background/problem clearly. (Sean)
> ---
>  drivers/vfio/vfio_iommu_type1.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 493c31de0edb..0620dbe5cca0 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -860,6 +860,11 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>  		if (ret)
>  			goto pin_unwind;
>  
> +		if (!pfn_valid(phys_pfn)) {

Why wouldn't we use our is_invalid_reserved_pfn() test here?  Doing
so would also make it more consistent why we don't need to call
put_pfn() or rewind accounting for this page.  Thanks,

Alex

> +			ret = -EINVAL;
> +			goto pin_unwind;
> +		}
> +
>  		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn);
>  		if (ret) {
>  			if (put_pfn(phys_pfn, dma->prot) && do_accounting)
> 
> base-commit: b3c98052d46948a8d65d2778c7f306ff38366aac