| Message ID | 20220616235212.15185-6-nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update vfio_pin/unpin_pages API | expand |
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. Oh..I will discuss with Jason once he's back, to see if we should simply drop this change. Thanks!
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
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
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.
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
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? Thanks!
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
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.
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
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index e2b01115b3ec..12cbe66721af 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> @@ -235,7 +236,6 @@ static long copy_from_iova(struct vfio_device *vdev, void *to, u64 iova, unsigned long n) { struct pfn_array pa = {0}; - u64 from; int i, ret; unsigned long l, m; @@ -251,7 +251,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); @@ -259,7 +261,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)
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(). Add kmap_local_page() before doing memcpy on "from". Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/s390/cio/vfio_ccw_cp.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)