Re: [Intel-gfx] [RFT][PATCH v1 3/6] vfio: Pass in starting IOVA to vfio_pin/unpin_pages API

Re: [Intel-gfx] [RFT][PATCH v1 3/6] vfio: Pass in starting IOVA to vfio_pin/unpin_pages API

[Christoph Hellwig]

On Thu, Jun 16, 2022 at 04:52:09PM -0700, Nicolin Chen wrote:
> +	ret = vfio_unpin_pages(&vgpu->vfio_device, gfn << PAGE_SHIFT, npage);
> +	drm_WARN_ON(&i915->drm, ret != npage);

The shifting of gfn seems to happen bother here and in the callers.

Also this is the only caller that does anything withthe vfio_unpin_pages
return value.  Given that you touch the API here we might as well
not return any value, and turn the debug checks that can return errors
into WARN_ON_ONCE calls the vfio/iommu_type1 code.

> +extern int vfio_pin_pages(struct vfio_device *device, dma_addr_t iova,
>  			  int npage, int prot, unsigned long *phys_pfn);
> -extern int vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
> +extern int vfio_unpin_pages(struct vfio_device *device, dma_addr_t iova,
>  			    int npage);

This will clash with the extern removal patch that Alex has sent.

Re: [Intel-gfx] [RFT][PATCH v1 3/6] vfio: Pass in starting IOVA to vfio_pin/unpin_pages API

[Nicolin Chen]

On Fri, Jun 17, 2022 at 01:42:42AM -0700, Christoph Hellwig wrote:
> On Thu, Jun 16, 2022 at 04:52:09PM -0700, Nicolin Chen wrote:
> > +	ret = vfio_unpin_pages(&vgpu->vfio_device, gfn << PAGE_SHIFT, npage);
> > +	drm_WARN_ON(&i915->drm, ret != npage);
> 
> The shifting of gfn seems to happen bother here and in the callers.

Sorry. I overlooked this line. I can add another preparatory patches
for callers to pass in an IOVA other than "pfn << PAGE_SHIFT", if you
think it's necessary: although GVT still does things in PFN, both ap
and ccw prepare their PFN lists from IOVAs, which now can be omitted.

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

[Christoph Hellwig]

On Thu, Jun 16, 2022 at 04:52:11PM -0700, Nicolin Chen wrote:
> The pinned PFN list returned from vfio_pin_pages() is simply converted
> using page_to_pfn() without protection, so direct access via memcpy()
> will crash on S390 if the PFN is an IO PFN. Instead, the pages should
> be touched using kmap_local_page().

I don't see how this helps.  kmap_local_page only works for either
pages in the kernel direct map or highmem, but not for memory that needs
to be ioremapped.  And there is no highmem on s390.

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

[Jason Gunthorpe]

On Fri, Jun 17, 2022 at 01:44:30AM -0700, Christoph Hellwig wrote:
> On Thu, Jun 16, 2022 at 04:52:11PM -0700, Nicolin Chen wrote:
> > The pinned PFN list returned from vfio_pin_pages() is simply converted
> > using page_to_pfn() without protection, so direct access via memcpy()
> > will crash on S390 if the PFN is an IO PFN. Instead, the pages should
> > be touched using kmap_local_page().
> 
> I don't see how this helps.  kmap_local_page only works for either
> pages in the kernel direct map or highmem, but not for memory that needs
> to be ioremapped.  And there is no highmem on s390.

The remark about io memory is because on s390 memcpy() will crash even
on ioremapped memory, you have to use the memcpy_to/fromio() which
uses the special s390 io access instructions.

This helps because we now block io memory from ever getting into these
call paths. I'm pretty sure this is a serious security bug, but would
let the IBM folks remark as I don't know it all that well..

As for the kmap, I thought it was standard practice even if it is a
non-highmem? Aren't people trying to use this for other security
stuff these days?

Jason

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

[Christoph Hellwig]

On Sun, Jun 19, 2022 at 11:57:26PM -0300, Jason Gunthorpe wrote:
> The remark about io memory is because on s390 memcpy() will crash even
> on ioremapped memory, you have to use the memcpy_to/fromio() which
> uses the special s390 io access instructions.

Yes.  The same is true for various other architectures, inluding arm64
under the right circumstances.

> This helps because we now block io memory from ever getting into these
> call paths. I'm pretty sure this is a serious security bug, but would
> let the IBM folks remark as I don't know it all that well..

Prevent as in crash when trying to convert it to a page?

> As for the kmap, I thought it was standard practice even if it is a
> non-highmem? Aren't people trying to use this for other security
> stuff these days?

Ira has been lookin into the protection keys, although they don't
apply to s390.  Either way I don't object to using kmap, but the
commit log doesn't make much sense to me.

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

[Jason Gunthorpe]

On Sun, Jun 19, 2022 at 11:32:07PM -0700, Christoph Hellwig wrote:

> > This helps because we now block io memory from ever getting into these
> > call paths. I'm pretty sure this is a serious security bug, but would
> > let the IBM folks remark as I don't know it all that well..
> 
> Prevent as in crash when trying to convert it to a page?

That or when calling memcpy() on an IO memory PFN that the guest
passed into the dma s/g list the ccw driver is processing.

Jason

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

[Nicolin Chen]

On Sun, Jun 19, 2022 at 11:32:07PM -0700, Christoph Hellwig wrote:
> On Sun, Jun 19, 2022 at 11:57:26PM -0300, Jason Gunthorpe wrote:
> > The remark about io memory is because on s390 memcpy() will crash even
> > on ioremapped memory, you have to use the memcpy_to/fromio() which
> > uses the special s390 io access instructions.
> 
> Yes.  The same is true for various other architectures, inluding arm64
> under the right circumstances.
> 
> > This helps because we now block io memory from ever getting into these
> > call paths. I'm pretty sure this is a serious security bug, but would
> > let the IBM folks remark as I don't know it all that well..
> 
> Prevent as in crash when trying to convert it to a page?
> 
> > As for the kmap, I thought it was standard practice even if it is a
> > non-highmem? Aren't people trying to use this for other security
> > stuff these days?
> 
> Ira has been lookin into the protection keys, although they don't
> apply to s390.  Either way I don't object to using kmap, but the
> commit log doesn't make much sense to me.

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.

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

[Jason Gunthorpe]

On Tue, Jun 21, 2022 at 02:21:22PM -0700, Nicolin Chen wrote:
> On Sun, Jun 19, 2022 at 11:32:07PM -0700, Christoph Hellwig wrote:
> > On Sun, Jun 19, 2022 at 11:57:26PM -0300, Jason Gunthorpe wrote:
> > > The remark about io memory is because on s390 memcpy() will crash even
> > > on ioremapped memory, you have to use the memcpy_to/fromio() which
> > > uses the special s390 io access instructions.
> > 
> > Yes.  The same is true for various other architectures, inluding arm64
> > under the right circumstances.
> > 
> > > This helps because we now block io memory from ever getting into these
> > > call paths. I'm pretty sure this is a serious security bug, but would
> > > let the IBM folks remark as I don't know it all that well..
> > 
> > Prevent as in crash when trying to convert it to a page?
> > 
> > > As for the kmap, I thought it was standard practice even if it is a
> > > non-highmem? Aren't people trying to use this for other security
> > > stuff these days?
> > 
> > Ira has been lookin into the protection keys, although they don't
> > apply to s390.  Either way I don't object to using kmap, but the
> > commit log doesn't make much sense to me.
> 
> 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.

Jason

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

[Jason Gunthorpe]

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.

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.

Jason

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

[Jason Gunthorpe]

On Fri, Jun 24, 2022 at 01:12:56PM -0700, Nicolin Chen wrote:

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

Yes, basically

Jason

Re: [Intel-gfx] [RFT][PATCH v1 6/6] vfio: Replace phys_pfn with phys_page for vfio_pin_pages()

[Christoph Hellwig]

There is a bunch of code an comments in the iommu type1 code that
suggest we can pin memory that is not page backed.  

>  int vfio_pin_pages(struct vfio_device *device, dma_addr_t iova,
> +		   int npage, int prot, struct page **phys_page)

I don't think phys_page makes much sense as an argument name.
I'd just call this pages.

> +			phys_page[i] = pfn_to_page(vpfn->pfn);

Please store the actual page pointer in the vfio_pfn structure.

>  		remote_vaddr = dma->vaddr + (iova - dma->iova);
> -		ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i],
> +		ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn,
>  					     do_accounting);

Please just return the actual page from vaddr_get_pfns through
vfio_pin_pages_remote and vfio_pin_page_external, maybe even as a prep
patch as that is a fair amount of churn.

Re: [Intel-gfx] [RFT][PATCH v1 6/6] vfio: Replace phys_pfn with phys_page for vfio_pin_pages()

[Christoph Hellwig]

On Fri, Jun 17, 2022 at 03:06:25PM -0700, Nicolin Chen wrote:
> On Fri, Jun 17, 2022 at 01:54:05AM -0700, Christoph Hellwig wrote:
> > There is a bunch of code an comments in the iommu type1 code that
> > suggest we can pin memory that is not page backed.  
> 
> Would you mind explaining the use case for pinning memory that
> isn't page backed? And do we have such use case so far?

Sorry, I should have deleted that sentence.  I wrote it before spending
some more time to dig through the code and all the locked memory has
page backing.  There just seem to be a lot of checks left inbetween
if a pfn is page backed, mostly due to the pfn based calling convetions.

> I can do that. I tried once, but there were just too much changes
> inside type1 code that felt like a chain reaction. If we plan to
> eventually replace with IOMMUFD implementations, these changes in
> type1 might not be necessary, I thought.

To make sure we keep full compatibility I suspect the final iommufd
implementation has to be gradutally created from the existing code
anyway.

Re: [Intel-gfx] [RFT][PATCH v1 6/6] vfio: Replace phys_pfn with phys_page for vfio_pin_pages()

[Jason Gunthorpe]

On Fri, Jun 17, 2022 at 01:54:05AM -0700, Christoph Hellwig wrote:
> There is a bunch of code an comments in the iommu type1 code that
> suggest we can pin memory that is not page backed.  

AFAIK you can.. The whole follow_pte() mechanism allows raw PFNs to be
loaded into the type1 maps and the pin API will happily return
them. This happens in almost every qemu scenario because PCI MMIO BAR
memory ends up routed down this path.

Thanks,
Jason

Re: [Intel-gfx] [RFT][PATCH v1 6/6] vfio: Replace phys_pfn with phys_page for vfio_pin_pages()

[Christoph Hellwig]

On Mon, Jun 20, 2022 at 12:00:46AM -0300, Jason Gunthorpe wrote:
> On Fri, Jun 17, 2022 at 01:54:05AM -0700, Christoph Hellwig wrote:
> > There is a bunch of code an comments in the iommu type1 code that
> > suggest we can pin memory that is not page backed.  
> 
> AFAIK you can.. The whole follow_pte() mechanism allows raw PFNs to be
> loaded into the type1 maps and the pin API will happily return
> them. This happens in almost every qemu scenario because PCI MMIO BAR
> memory ends up routed down this path.

Indeed, my read wasn't deep enough.  Which means that we can't change
the ->pin_pages interface to return a struct pages array, as we don't
have one for those.

Re: [Intel-gfx] [RFT][PATCH v1 6/6] vfio: Replace phys_pfn with phys_page for vfio_pin_pages()

[Christoph Hellwig]

On Sun, Jun 19, 2022 at 10:51:47PM -0700, Christoph Hellwig wrote:
> On Mon, Jun 20, 2022 at 12:00:46AM -0300, Jason Gunthorpe wrote:
> > On Fri, Jun 17, 2022 at 01:54:05AM -0700, Christoph Hellwig wrote:
> > > There is a bunch of code an comments in the iommu type1 code that
> > > suggest we can pin memory that is not page backed.  
> > 
> > AFAIK you can.. The whole follow_pte() mechanism allows raw PFNs to be
> > loaded into the type1 maps and the pin API will happily return
> > them. This happens in almost every qemu scenario because PCI MMIO BAR
> > memory ends up routed down this path.
> 
> Indeed, my read wasn't deep enough.  Which means that we can't change
> the ->pin_pages interface to return a struct pages array, as we don't
> have one for those.

Actually.  gvt requires a struct page, and both s390 seem to require
normal non-I/O, non-remapped kernel pointers.  So I think for the
vfio_pin_pages we can assume that we only want page backed memory and
remove the follow_fault_pfn case entirely.   But we'll probably have
to keep it for the vfio_iommu_replay case that is not tied to
emulated IOMMU drivers.

Re: [Intel-gfx] [RFT][PATCH v1 6/6] vfio: Replace phys_pfn with phys_page for vfio_pin_pages()

[Jason Gunthorpe]

On Sun, Jun 19, 2022 at 11:37:47PM -0700, Christoph Hellwig wrote:
> On Sun, Jun 19, 2022 at 10:51:47PM -0700, Christoph Hellwig wrote:
> > On Mon, Jun 20, 2022 at 12:00:46AM -0300, Jason Gunthorpe wrote:
> > > On Fri, Jun 17, 2022 at 01:54:05AM -0700, Christoph Hellwig wrote:
> > > > There is a bunch of code an comments in the iommu type1 code that
> > > > suggest we can pin memory that is not page backed.  
> > > 
> > > AFAIK you can.. The whole follow_pte() mechanism allows raw PFNs to be
> > > loaded into the type1 maps and the pin API will happily return
> > > them. This happens in almost every qemu scenario because PCI MMIO BAR
> > > memory ends up routed down this path.
> > 
> > Indeed, my read wasn't deep enough.  Which means that we can't change
> > the ->pin_pages interface to return a struct pages array, as we don't
> > have one for those.
> 
> Actually.  gvt requires a struct page, and both s390 seem to require
> normal non-I/O, non-remapped kernel pointers.  So I think for the
> vfio_pin_pages we can assume that we only want page backed memory and
> remove the follow_fault_pfn case entirely.   But we'll probably have
> to keep it for the vfio_iommu_replay case that is not tied to
> emulated IOMMU drivers.

Right, that is my thinking - since all drivers actually need a struct
page we should have the API return a working struct page and have the
VFIO layer isolate the non-struct page stuff so it never leaks out of
this API.

This nicely fixes the various problems in all the drivers if io memory
comes down this path.

It is also why doing too much surgery deeper into type 1 probably
isn't too worthwhile as it still needs raw pfns in its data
structures for iommu modes.

Thanks,
Jason

Re: [Intel-gfx] [RFT][PATCH v1 6/6] vfio: Replace phys_pfn with phys_page for vfio_pin_pages()

[Nicolin Chen]

On Mon, Jun 20, 2022 at 12:36:28PM -0300, Jason Gunthorpe wrote:
> On Sun, Jun 19, 2022 at 11:37:47PM -0700, Christoph Hellwig wrote:
> > On Sun, Jun 19, 2022 at 10:51:47PM -0700, Christoph Hellwig wrote:
> > > On Mon, Jun 20, 2022 at 12:00:46AM -0300, Jason Gunthorpe wrote:
> > > > On Fri, Jun 17, 2022 at 01:54:05AM -0700, Christoph Hellwig wrote:
> > > > > There is a bunch of code an comments in the iommu type1 code that
> > > > > suggest we can pin memory that is not page backed.  
> > > > 
> > > > AFAIK you can.. The whole follow_pte() mechanism allows raw PFNs to be
> > > > loaded into the type1 maps and the pin API will happily return
> > > > them. This happens in almost every qemu scenario because PCI MMIO BAR
> > > > memory ends up routed down this path.
> > > 
> > > Indeed, my read wasn't deep enough.  Which means that we can't change
> > > the ->pin_pages interface to return a struct pages array, as we don't
> > > have one for those.
> > 
> > Actually.  gvt requires a struct page, and both s390 seem to require
> > normal non-I/O, non-remapped kernel pointers.  So I think for the
> > vfio_pin_pages we can assume that we only want page backed memory and
> > remove the follow_fault_pfn case entirely.   But we'll probably have
> > to keep it for the vfio_iommu_replay case that is not tied to
> > emulated IOMMU drivers.
> 
> Right, that is my thinking - since all drivers actually need a struct
> page we should have the API return a working struct page and have the
> VFIO layer isolate the non-struct page stuff so it never leaks out of
> this API.
> 
> This nicely fixes the various problems in all the drivers if io memory
> comes down this path.
> 
> It is also why doing too much surgery deeper into type 1 probably
> isn't too worthwhile as it still needs raw pfns in its data
> structures for iommu modes.

Christoph, do you agree with Jason's remark on not doing too much
surgery into type1 code? Or do you still want this series to change
type1 like removing follow_fault_pfn() that you mentioned above?

Re: [Intel-gfx] [RFT][PATCH v1 1/6] vfio/ap: Pass in physical address of ind to ap_aqic()

[Nicolin Chen]

On Mon, Jun 20, 2022 at 12:00:53PM +0200, Harald Freudenberger wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2022-06-17 01:52, Nicolin Chen wrote:
> > The ap_aqic() is called by vfio_ap_irq_enable() where it passes in a
> > virt value that's casted from a physical address "h_nib". Inside the
> > ap_aqic(), it does virt_to_phys() again.
> > 
> > Since ap_aqic() needs a physical address, let's just pass in a pa of
> > ind directly. So change the "ind" to "pa_ind".
> > 
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

> Add my Reviewed-By: Harald Freudenberger <freude@linux.ibm.com>

Will do. Thanks!