Re: [RFT][PATCH v2 8/9] vfio/ccw: Add kmap_local_page() for memcpy

[Eric Farman]

On Tue, 2022-07-05 at 23:27 -0700, Nicolin Chen wrote:
> A PFN is not secure enough to promise that the memory is not IO. And
> direct access via memcpy() that only handles CPU memory will crash on
> S390 if the PFN is an IO PFN, as we have to use the
> memcpy_to/fromio()
> that uses the special S390 IO access instructions. On the other hand,
> a "struct page *" is always a CPU coherent thing that fits memcpy().
> 
> Also, casting a PFN to "void *" for memcpy() is not a proper
> practice,
> kmap_local_page() is the correct API to call here, though S390
> doesn't
> use highmem, which means kmap_local_page() is a NOP.
> 
> There's a following patch changing the vfio_pin_pages() API to return
> a list of "struct page *" instead of PFNs. It will block any IO
> memory
> from ever getting into this call path, for such a security purpose.
> In
> this patch, add kmap_local_page() to prepare for that.

This all sounds like it's conflating vfio-ccw with vfio-pci, and
configuration-wise I have a hard time picturing the situation described
above. But in the interest of the change in the next patch, I suppose
it's fine.

Acked-by: Eric Farman <farman@xxxxxxxxxxxxx>

> 
> Suggested-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c
> b/drivers/s390/cio/vfio_ccw_cp.c
> index 3854c3d573f5..cd4ec4f6d6ff 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -11,6 +11,7 @@
>  #include <linux/ratelimit.h>
>  #include <linux/mm.h>
>  #include <linux/slab.h>
> +#include <linux/highmem.h>
>  #include <linux/iommu.h>
>  #include <linux/vfio.h>
>  #include <asm/idals.h>
> @@ -230,7 +231,6 @@ static long copy_from_iova(struct vfio_device
> *vdev, void *to, u64 iova,
>  			   unsigned long n)
>  {
>  	struct page_array pa = {0};
> -	u64 from;
>  	int i, ret;
>  	unsigned long l, m;
>  
> @@ -246,7 +246,9 @@ static long copy_from_iova(struct vfio_device
> *vdev, void *to, u64 iova,
>  
>  	l = n;
>  	for (i = 0; i < pa.pa_nr; i++) {
> -		from = pa.pa_pfn[i] << PAGE_SHIFT;
> +		struct page *page = pfn_to_page(pa.pa_pfn[i]);
> +		void *from = kmap_local_page(page);
> +
>  		m = PAGE_SIZE;
>  		if (i == 0) {
>  			from += iova & (PAGE_SIZE - 1);
> @@ -254,7 +256,8 @@ static long copy_from_iova(struct vfio_device
> *vdev, void *to, u64 iova,
>  		}
>  
>  		m = min(l, m);
> -		memcpy(to + (n - l), (void *)from, m);
> +		memcpy(to + (n - l), from, m);
> +		kunmap_local(from);
>  
>  		l -= m;
>  		if (l == 0)