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)
