Re: [RFT][PATCH v1 5/6] vfio/ccw: Add kmap_local_page() for memcpy

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Jun 24, 2022 at 04:30:42PM -0300, Jason Gunthorpe wrote:
> On Fri, Jun 24, 2022 at 12:22:36PM -0700, Nicolin Chen wrote:
> > On Fri, Jun 24, 2022 at 10:56:15AM -0300, Jason Gunthorpe wrote:
> > 
> > > > How about the updated commit log below? Thanks.
> > > > 
> > > > The pinned PFN list returned from vfio_pin_pages() is converted using
> > > > page_to_pfn(), so direct access via memcpy() will crash on S390 if the
> > > > PFN is an IO PFN, as we have to use the memcpy_to/fromio(), which uses
> > > > the special s390 IO access instructions.
> > > > 
> > > > As a standard practice for security purpose, add kmap_local_page() to
> > > > block any IO memory from ever getting into this call path.
> > > 
> > > The kmap_local_page is not about the IO memory, the switch to struct
> > > page is what is protecting against IO memory.
> > > 
> > > Use kmap_local_page() is just the correct way to convert a struct page
> > > into a CPU address to use with memcpy and it is a NOP on S390 because
> > > it doesn't use highmem/etc.
> > 
> > I thought the whole purpose of switching to "struct page *" was to use
> > kmap_local_page() for the memcpy call, and the combination of these two
> > does the protection. Do you mind explaining how the switching part does
> > the protection?
> 
> A 'struct page' (ignoring ZONE_DEVICE) cannot represent IO memory
> inherently because a 'struct page' is always a CPU coherent thing.
> 
> So, when VFIO returns only struct pages it also is promising that the
> memory is not IO.

Ah. The "switch to struct page" means the next patch that changes the
vfio_pin_pages, not the pfn_to_page in this patch.

> The kmap_local_page() arose because the code doing memcpy had to be
> updated to go from a struct page to a void * for use with memcpy and
> the kmap_local_page() is the correct API to use for that.
> 
> The existing code which casts a pfn to a void * is improper.

Yes.

If I understand everything correctly:

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 an 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.