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)