linux-kernel - Re: [PATCH v5 2/2] PCI: s390: Support mmap() of BARs and replace VFIO_PCI

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] [day] [month] [year] [list]

Message-ID: <8f1a62e93bde37708a47b7db70767d1e14c608e0.camel@linux.ibm.com>
Date: Thu, 13 Feb 2025 11:06:30 +0100
From: Niklas Schnelle <schnelle@...ux.ibm.com>
To: Alex Williamson <alex.williamson@...hat.com>
Cc: Bjorn Helgaas <helgaas@...nel.org>, Christoph Hellwig <hch@....de>,
        Alexandra Winter <wintera@...ux.ibm.com>,
        Gerd Bayer
 <gbayer@...ux.ibm.com>,
        Matthew Rosato <mjrosato@...ux.ibm.com>,
        Jason
 Gunthorpe	 <jgg@...pe.ca>,
        Thorsten Winkler <twinkler@...ux.ibm.com>,
        Bjorn
 Helgaas	 <bhelgaas@...gle.com>,
        Julian Ruess <julianr@...ux.ibm.com>,
        Halil
 Pasic	 <pasic@...ux.ibm.com>,
        Christian Borntraeger
 <borntraeger@...ux.ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        Gerald
 Schaefer <gerald.schaefer@...ux.ibm.com>,
        Heiko Carstens	
 <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev	
 <agordeev@...ux.ibm.com>,
        linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v5 2/2] PCI: s390: Support mmap() of BARs and replace
 VFIO_PCI_MMAP by a device flag

On Wed, 2025-02-12 at 13:28 -0700, Alex Williamson wrote:
> On Wed, 12 Feb 2025 16:28:32 +0100
> Niklas Schnelle <schnelle@...ux.ibm.com> wrote:
> 
> > On s390 there is a virtual PCI device called ISM which has a few
> > peculiarities. For one, it presents a 256 TiB PCI BAR whose size leads
> > to any attempt to ioremap() the whole BAR failing. This is problematic
> > since mapping the whole BAR is the default behavior of for example
> > vfio-pci in combination with QEMU and VFIO_PCI_MMAP enabled.
> > 
> > Even if one tried to map this BAR only partially, the mapping would not
> > be usable without extra precautions on systems with MIO support enabled.
> > This is because of another oddity, in that this virtual PCI device does
> > not support the newer memory I/O (MIO) PCI instructions and legacy PCI
> > instructions are not accessible through writeq()/readq() when MIO is in
> > use.
> > 
> > In short the ISM device's BAR is not accessible through memory mappings.
> > Indicate this by introducing a new non_mappable_bars flag for the ISM
> > device and set it using a PCI quirk. Use this flag instead of the
> > VFIO_PCI_MMAP Kconfig option to block mapping with vfio-pci. This was
> > the only use of the Kconfig option so remove it. Note that there are no
> > PCI resource sysfs files on s390x already as HAVE_PCI_MMAP is currently
> > not set. If this were to be set in the future pdev->non_mappable_bars
> > can be used to prevent unusable resource files for ISM from being
> > created.
> 
> I think we should also look at it from the opposite side, not just
> s390x maybe adding HAVE_PCI_MMAP in the future, but the fact that we're
> currently adding a generic PCI device flag which isn't honored by the
> one mechanism that PCI core provides to mmap MMIO BARs to userspace.
> It seems easier to implement it in pci_mmap_resource() now rather than
> someone later discovering there's no enforcement outside of the very
> narrow s390x use case.  Thanks,
> 
> Alex

That is a very good point! I did try enabling HAVE_PCI_MMAP for s390 a
while back and I believe that ran into trouble with ISM devices too. So
I just did a quick test of enabling HAVE_PCI_MMAP with
ARCH_GENERIC_PCI_MMAP_RESOURCE for s390. Then added a check for 
pdev->non_mappable_bars to pci_create_resource_files() and
proc_bus_pci_mmap(). I pondered adding it to pci_mmap_resource() too
but felt like not showing the resource at all, like we do now with
!HAVE_PCI_MMAP is cleaner.

Using a little test program that just mmap()s BAR 0 of an NVMe and
reads the NVMe version at offset 8 using our PCI MIO load instruction
works. Also, as expected I don't get resourceX files for ISM devices
with the added check. 

I still have to test the /proc/bus/pci based mmap() but would expect
that to work too. So I'd be open to adding another patch which adds
HAVE_PCI_MMAP for s390, if we see too much risk with that, we could
alternatively add just the pdev->non_mappable_bars but then they would
be untested, still better than hoping that someone remembers to add
that in the future.

Thanks,
Niklas