linux-kernel - Re: [RFT][PATCH v2 8/9] vfio/ccw: Add kmap_local

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]

Date:   Fri, 08 Jul 2022 16:29:29 -0400
From:   Eric Farman <farman@...ux.ibm.com>
To:     Nicolin Chen <nicolinc@...dia.com>, kwankhede@...dia.com,
        corbet@....net, hca@...ux.ibm.com, gor@...ux.ibm.com,
        agordeev@...ux.ibm.com, borntraeger@...ux.ibm.com,
        svens@...ux.ibm.com, zhenyuw@...ux.intel.com, zhi.a.wang@...el.com,
        jani.nikula@...ux.intel.com, joonas.lahtinen@...ux.intel.com,
        rodrigo.vivi@...el.com, tvrtko.ursulin@...ux.intel.com,
        airlied@...ux.ie, daniel@...ll.ch, mjrosato@...ux.ibm.com,
        pasic@...ux.ibm.com, vneethv@...ux.ibm.com, oberpar@...ux.ibm.com,
        freude@...ux.ibm.com, akrowiak@...ux.ibm.com,
        jjherne@...ux.ibm.com, alex.williamson@...hat.com,
        cohuck@...hat.com, jgg@...dia.com, kevin.tian@...el.com,
        hch@...radead.org
Cc:     jchrist@...ux.ibm.com, kvm@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org, intel-gvt-dev@...ts.freedesktop.org,
        intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org
Subject: Re: [RFT][PATCH v2 8/9] vfio/ccw: Add kmap_local_page() for memcpy

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@...ux.ibm.com>

> 
> Suggested-by: Jason Gunthorpe <jgg@...dia.com>
> Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
> ---
>  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)