[PATCH] PCI: Allow drivers to claim exclusive access to config regions

[PATCH] PCI: Allow drivers to claim exclusive access to config regions

[Dan Williams]

The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
configuration cycles. It assumes one initiator at a time is
reading/writing the data registers. If userspace reads from the response
data payload it may steal data that a kernel driver was expecting to
read. If userspace writes to the request payload it may corrupt the
request a driver was trying to send.

Introduce pci_{request,release}_config_region() for a driver to exclude
the possibility of userspace induced corruption while accessing the DOE
mailbox. Likely there are other configuration state assumptions that a
driver may want to assert are under its exclusive control, so this
capability is not limited to any specific configuration range.

Since writes are targeted and are already prepared for failure the
entire request is failed. The same can not be done for reads as the
device completely disappears from lspci output if any configuration
register in the request is exclusive. Instead skip the actual
configuration cycle on a per-access basis and return all f's as if the
read had failed.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/pci/access.c    |    5 +++--
 drivers/pci/pci-sysfs.c |    3 +++
 drivers/pci/probe.c     |    5 +++++
 include/linux/ioport.h  |    2 ++
 include/linux/pci.h     |   16 ++++++++++++++++
 kernel/resource.c       |   24 +++++++++++-------------
 6 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 46935695cfb9..a6b3cdfbd505 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -225,8 +225,9 @@ int pci_user_read_config_##size						\
 	raw_spin_lock_irq(&pci_lock);				\
 	if (unlikely(dev->block_cfg_access))				\
 		pci_wait_cfg(dev);					\
-	ret = dev->bus->ops->read(dev->bus, dev->devfn,			\
-					pos, sizeof(type), &data);	\
+	if (!resource_is_exclusive(&dev->config_resource, pos, sizeof(type))) \
+		ret = dev->bus->ops->read(dev->bus, dev->devfn,		\
+					  pos, sizeof(type), &data);	\
 	raw_spin_unlock_irq(&pci_lock);				\
 	*val = (type)data;						\
 	return pcibios_err_to_errno(ret);				\
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index f8afd54ca3e1..4ce2831ffadc 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -753,6 +753,9 @@ static ssize_t pci_write_config(struct file *filp, struct kobject *kobj,
 	u8 *data = (u8 *) buf;
 	int ret;
 
+	if (resource_is_exclusive(&dev->config_resource, off, count))
+		return -EBUSY;
+
 	ret = security_locked_down(LOCKDOWN_PCI_ACCESS);
 	if (ret)
 		return ret;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 953f15abc850..8fa3a6f38b53 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2240,6 +2240,11 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
 	INIT_LIST_HEAD(&dev->bus_list);
 	dev->dev.type = &pci_dev_type;
 	dev->bus = pci_bus_get(bus);
+	dev->config_resource = (struct resource) {
+		.name = "PCI Config",
+		.start = 0,
+		.end = -1,
+	};
 
 	return dev;
 }
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 55de385c839c..e1487a892fe7 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -308,6 +308,8 @@ extern void __devm_release_region(struct device *dev, struct resource *parent,
 				  resource_size_t start, resource_size_t n);
 extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size);
 extern bool iomem_is_exclusive(u64 addr);
+extern bool resource_is_exclusive(struct resource *resource, u64 addr,
+				  resource_size_t size);
 
 extern int
 walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 86c799c97b77..d3d78e0df2e7 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -401,6 +401,7 @@ struct pci_dev {
 	 */
 	unsigned int	irq;
 	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
+	struct resource config_resource;	/* driver exclusive config register ranges */
 
 	bool		match_driver;		/* Skip attaching driver */
 
@@ -1330,6 +1331,21 @@ int pci_request_selected_regions(struct pci_dev *, int, const char *);
 int pci_request_selected_regions_exclusive(struct pci_dev *, int, const char *);
 void pci_release_selected_regions(struct pci_dev *, int);
 
+static inline __must_check struct resource *
+pci_request_config_region(struct pci_dev *pdev, unsigned int where,
+			  unsigned int len, const char *name)
+{
+	return __request_region(&pdev->config_resource, where, len, name,
+				IORESOURCE_EXCLUSIVE);
+}
+
+static inline void pci_release_config_region(struct pci_dev *pdev,
+					     unsigned int where,
+					     unsigned int len)
+{
+	__release_region(&pdev->config_resource, where, len);
+}
+
 /* drivers/pci/bus.c */
 void pci_add_resource(struct list_head *resources, struct resource *res);
 void pci_add_resource_offset(struct list_head *resources, struct resource *res,
diff --git a/kernel/resource.c b/kernel/resource.c
index 627e61b0c124..ab1aed06e8b0 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1706,27 +1706,16 @@ static int strict_iomem_checks;
 #endif
 
 /*
- * check if an address is reserved in the iomem resource tree
+ * check if an address is reserved in the @resource tree
  * returns true if reserved, false if not reserved.
  */
-bool iomem_is_exclusive(u64 addr)
+bool resource_is_exclusive(struct resource *p, u64 addr, resource_size_t size)
 {
-	struct resource *p = &iomem_resource;
 	bool err = false;
 	loff_t l;
-	int size = PAGE_SIZE;
-
-	if (!strict_iomem_checks)
-		return false;
-
-	addr = addr & PAGE_MASK;
 
 	read_lock(&resource_lock);
 	for (p = p->child; p ; p = r_next(NULL, p, &l)) {
-		/*
-		 * We can probably skip the resources without
-		 * IORESOURCE_IO attribute?
-		 */
 		if (p->start >= addr + size)
 			break;
 		if (p->end < addr)
@@ -1749,6 +1738,15 @@ bool iomem_is_exclusive(u64 addr)
 	return err;
 }
 
+bool iomem_is_exclusive(u64 addr)
+{
+	if (!strict_iomem_checks)
+		return false;
+
+	return resource_is_exclusive(&iomem_resource, addr & PAGE_MASK,
+				     PAGE_SIZE);
+}
+
 struct resource_entry *resource_list_create_entry(struct resource *res,
 						  size_t extra_size)
 {

Re: [PATCH] PCI: Allow drivers to claim exclusive access to config regions

[Greg Kroah-Hartman]

On Wed, Mar 24, 2021 at 06:23:54PM -0700, Dan Williams wrote:
> The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
> configuration cycles. It assumes one initiator at a time is
> reading/writing the data registers.

That sounds like a horrible protocol for a multi-processor system.
Where is it described and who can we go complain to for creating such a
mess?

> If userspace reads from the response
> data payload it may steal data that a kernel driver was expecting to
> read. If userspace writes to the request payload it may corrupt the
> request a driver was trying to send.

Fun!  So you want to keep root in userspace from doing this?  I thought
we already do that today?

> Introduce pci_{request,release}_config_region() for a driver to exclude
> the possibility of userspace induced corruption while accessing the DOE
> mailbox. Likely there are other configuration state assumptions that a
> driver may want to assert are under its exclusive control, so this
> capability is not limited to any specific configuration range.

As you do not have a user for these functions, it's hard to see how they
would be used.  We also really can't add new apis with no in-tree users,
so do you have a patch series that requires this functionality
somewhere?

> Since writes are targeted and are already prepared for failure the
> entire request is failed. The same can not be done for reads as the
> device completely disappears from lspci output if any configuration
> register in the request is exclusive. Instead skip the actual
> configuration cycle on a per-access basis and return all f's as if the
> read had failed.

returning all ff is a huge hint to many drivers that the device is gone,
not that it just failed.  So what happens to code that thinks that and
then tears stuff down as if the device has been removed?

Trying to protect drivers from userspace here feels odd, what userspace
tools are trying to access these devices while they are under
"exclusive" control from the kernel?  lspci not running as root should
not be doing anything crazy, but if you want to run it as root,
shouldn't you be allowed to access it properly?

What hardware has this problem that we need to claim exclusive ownership
over that differs from the old hardware we used to have that would do
crazy things when reading from from userspace?  We had this problem a
long time ago and lived with it, what changed now?

thanks,

greg k-h

Re: [PATCH] PCI: Allow drivers to claim exclusive access to config regions

[Christoph Hellwig]

On Thu, Mar 25, 2021 at 07:54:59AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Mar 24, 2021 at 06:23:54PM -0700, Dan Williams wrote:
> > The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
> > configuration cycles. It assumes one initiator at a time is
> > reading/writing the data registers.
> 
> That sounds like a horrible protocol for a multi-processor system.
> Where is it described and who can we go complain to for creating such a
> mess?

Indeed.  Dan, is there a way to stilk kill this protocol off before it
leaks into the wild?

Re: [PATCH] PCI: Allow drivers to claim exclusive access to config regions

[Dan Williams]

On Thu, Mar 25, 2021 at 1:29 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, Mar 25, 2021 at 07:54:59AM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Mar 24, 2021 at 06:23:54PM -0700, Dan Williams wrote:
> > > The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
> > > configuration cycles. It assumes one initiator at a time is
> > > reading/writing the data registers.
> >
> > That sounds like a horrible protocol for a multi-processor system.
> > Where is it described and who can we go complain to for creating such a
> > mess?
>
> Indeed.  Dan, is there a way to stilk kill this protocol off before it
> leaks into the wild?

Unfortunately I think that opportunity was more than a year ago, and
there's been a proliferation of derivative protocols building on it
since.

Re: [PATCH] PCI: Allow drivers to claim exclusive access to config regions

[Greg Kroah-Hartman]

On Thu, Mar 25, 2021 at 10:55:01AM -0700, Dan Williams wrote:
> On Thu, Mar 25, 2021 at 1:29 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Thu, Mar 25, 2021 at 07:54:59AM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Mar 24, 2021 at 06:23:54PM -0700, Dan Williams wrote:
> > > > The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
> > > > configuration cycles. It assumes one initiator at a time is
> > > > reading/writing the data registers.
> > >
> > > That sounds like a horrible protocol for a multi-processor system.
> > > Where is it described and who can we go complain to for creating such a
> > > mess?
> >
> > Indeed.  Dan, is there a way to stilk kill this protocol off before it
> > leaks into the wild?
> 
> Unfortunately I think that opportunity was more than a year ago, and
> there's been a proliferation of derivative protocols building on it
> since.

Doesn't mean it can't be changed, right?

Are there any actual devices that require this?

thanks,

greg k-h

Re: [PATCH] PCI: Allow drivers to claim exclusive access to config regions

[Dan Williams]

On Wed, Mar 24, 2021 at 11:55 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Mar 24, 2021 at 06:23:54PM -0700, Dan Williams wrote:
> > The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
> > configuration cycles. It assumes one initiator at a time is
> > reading/writing the data registers.
>
> That sounds like a horrible protocol for a multi-processor system.
> Where is it described and who can we go complain to for creating such a
> mess?

It appears it was added to the PCIE specification in March of last
year, I don't attend those meetings. I only learned about it since the
CXL specification adopted it.

>
> > If userspace reads from the response
> > data payload it may steal data that a kernel driver was expecting to
> > read. If userspace writes to the request payload it may corrupt the
> > request a driver was trying to send.
>
> Fun!  So you want to keep root in userspace from doing this?  I thought
> we already do that today?

The only limitation I found was temporary locking via
pci_cfg_access_lock(), and some limitations on max config offset, not
permanent access disable.

>
> > Introduce pci_{request,release}_config_region() for a driver to exclude
> > the possibility of userspace induced corruption while accessing the DOE
> > mailbox. Likely there are other configuration state assumptions that a
> > driver may want to assert are under its exclusive control, so this
> > capability is not limited to any specific configuration range.
>
> As you do not have a user for these functions, it's hard to see how they
> would be used.  We also really can't add new apis with no in-tree users,
> so do you have a patch series that requires this functionality
> somewhere?

Whoops, I buried the lead here. This is in reaction to / support of
Jonathan's efforts to use this mailbox to retrieve a blob of memory
characteristics data from CXL devices called the CDAT [1]. That blob
is used to populate / extend ACPI SRAT/HMAT/SLIT data for CXL attached
memory. So while I was reviewing his patches it occurred to me that
the b0rked nature of this interface relative to pci-sysfs needed to be
addressed. This should wait to go in with his series.

[1]: https://lore.kernel.org/linux-acpi/20210310180306.1588376-2-Jonathan.Cameron@huawei.com/

>
> > Since writes are targeted and are already prepared for failure the
> > entire request is failed. The same can not be done for reads as the
> > device completely disappears from lspci output if any configuration
> > register in the request is exclusive. Instead skip the actual
> > configuration cycle on a per-access basis and return all f's as if the
> > read had failed.
>
> returning all ff is a huge hint to many drivers that the device is gone,
> not that it just failed.  So what happens to code that thinks that and
> then tears stuff down as if the device has been removed?

This is limited to the pci_user_* family of accessors, kernel drivers
should be unaffected. The protection for kernel drivers colliding is
the same as request_mem_region() coordination.

Userspace drivers will of course be horribly confused, but those
should not be binding to devices that are claimed by a kernel driver
in the first place.

> Trying to protect drivers from userspace here feels odd, what userspace
> tools are trying to access these devices while they are under
> "exclusive" control from the kernel?  lspci not running as root should
> not be doing anything crazy, but if you want to run it as root,
> shouldn't you be allowed to access it properly?

The main concern is unwanted userspace reads. An inopportune "hexdump
/sys/bus/pci/devices/$device/config" will end up reading the DOE
payload register and advancing the device state machine surprising the
kernel iterator that might be reading the payload.

If root really wants to read that portion of config space it can also
unload the driver similar to the policy for /dev/mem colliding with
exclusive device-mmio... although that raises the question how would
root know. At least for exclusive /dev/mem /proc/iomem can show who is
claiming that resource.

If userspace needs to submit DOE requests then that should probably be
a proper generic driver to submit requests, not raw pci config access.

> What hardware has this problem that we need to claim exclusive ownership
> over that differs from the old hardware we used to have that would do
> crazy things when reading from from userspace?  We had this problem a
> long time ago and lived with it, what changed now?

All I can infer from the comments in drivers/pci/access.c is "bad
things happens on some devices if you allow reads past a certain
offset", but no concern for reads for offsets less than
pdev->cfg_size. I think what's changed is that this is the first time
Linux has had to worry about an awkward polled I/O data transfer
protocol over config cycles.

To make matters worse there appears to be a proliferation of protocols
being layered on top of DOE. In addition to CXL Table Access for CDAT
retrieval [2] I'm aware of CXL Compliance Testing [3], Integrity and
Data Encryption (IDE) [4], and Component Measurement and
Authentication (CMA) [5].

I've not read those, but I worry security_locked_down() may want to
prevent even root userspace mucking with "security" interfaces. So
that *might* be a reason to ensure exclusive kernel access beyond the
basic sanity of the kernel being able to have uninterrupted request /
response sessions with this mailbox

[2]: https://uefi.org/sites/default/files/resources/Coherent%20Device%20Attribute%20Table_1.01.pdf
[3]: https://www.computeexpresslink.org/download-the-specification
[4]: https://members.pcisig.com/wg/PCI-SIG/document/15149
[5]: https://members.pcisig.com/wg/PCI-SIG/document/14236

Re: [PATCH] PCI: Allow drivers to claim exclusive access to config regions

[Greg Kroah-Hartman]

On Thu, Mar 25, 2021 at 10:43:41AM -0700, Dan Williams wrote:
> On Wed, Mar 24, 2021 at 11:55 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Mar 24, 2021 at 06:23:54PM -0700, Dan Williams wrote:
> > > The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
> > > configuration cycles. It assumes one initiator at a time is
> > > reading/writing the data registers.
> >
> > That sounds like a horrible protocol for a multi-processor system.
> > Where is it described and who can we go complain to for creating such a
> > mess?
> 
> It appears it was added to the PCIE specification in March of last
> year, I don't attend those meetings. I only learned about it since the
> CXL specification adopted it.

Given that the kernel community can not participate in the PCI spec
process, we rely on the member companies that do work there to at least
create sane things.  Who do I need to go complain to about this
happening, again?

> > > If userspace reads from the response
> > > data payload it may steal data that a kernel driver was expecting to
> > > read. If userspace writes to the request payload it may corrupt the
> > > request a driver was trying to send.
> >
> > Fun!  So you want to keep root in userspace from doing this?  I thought
> > we already do that today?
> 
> The only limitation I found was temporary locking via
> pci_cfg_access_lock(), and some limitations on max config offset, not
> permanent access disable.

Temp locking should be all that is needed, right?  If not then something
really is wrong with the protocol design.

> > > Introduce pci_{request,release}_config_region() for a driver to exclude
> > > the possibility of userspace induced corruption while accessing the DOE
> > > mailbox. Likely there are other configuration state assumptions that a
> > > driver may want to assert are under its exclusive control, so this
> > > capability is not limited to any specific configuration range.
> >
> > As you do not have a user for these functions, it's hard to see how they
> > would be used.  We also really can't add new apis with no in-tree users,
> > so do you have a patch series that requires this functionality
> > somewhere?
> 
> Whoops, I buried the lead here. This is in reaction to / support of
> Jonathan's efforts to use this mailbox to retrieve a blob of memory
> characteristics data from CXL devices called the CDAT [1]. That blob
> is used to populate / extend ACPI SRAT/HMAT/SLIT data for CXL attached
> memory. So while I was reviewing his patches it occurred to me that
> the b0rked nature of this interface relative to pci-sysfs needed to be
> addressed. This should wait to go in with his series.
> 
> [1]: https://lore.kernel.org/linux-acpi/20210310180306.1588376-2-Jonathan.Cameron@huawei.com/

Interesting.  Messy.  Ick.

> > > Since writes are targeted and are already prepared for failure the
> > > entire request is failed. The same can not be done for reads as the
> > > device completely disappears from lspci output if any configuration
> > > register in the request is exclusive. Instead skip the actual
> > > configuration cycle on a per-access basis and return all f's as if the
> > > read had failed.
> >
> > returning all ff is a huge hint to many drivers that the device is gone,
> > not that it just failed.  So what happens to code that thinks that and
> > then tears stuff down as if the device has been removed?
> 
> This is limited to the pci_user_* family of accessors, kernel drivers
> should be unaffected. The protection for kernel drivers colliding is
> the same as request_mem_region() coordination.

So you want to lock out user accesses entirely if a kernel driver is
bound to the device?  I missed that here.  If so, that's a big change
that people are not going to like.

> Userspace drivers will of course be horribly confused, but those
> should not be binding to devices that are claimed by a kernel driver
> in the first place.

I'm not worried about "userspace drivers", I'm worried about tools like
'lspci' and others that like reading PCI device state from userspace at
tiems.

> > Trying to protect drivers from userspace here feels odd, what userspace
> > tools are trying to access these devices while they are under
> > "exclusive" control from the kernel?  lspci not running as root should
> > not be doing anything crazy, but if you want to run it as root,
> > shouldn't you be allowed to access it properly?
> 
> The main concern is unwanted userspace reads. An inopportune "hexdump
> /sys/bus/pci/devices/$device/config" will end up reading the DOE
> payload register and advancing the device state machine surprising the
> kernel iterator that might be reading the payload.
> 
> If root really wants to read that portion of config space it can also
> unload the driver similar to the policy for /dev/mem colliding with
> exclusive device-mmio... although that raises the question how would
> root know. At least for exclusive /dev/mem /proc/iomem can show who is
> claiming that resource.

Yes, root does not know, so this feels really broken from a spec
point-of-view.

> If userspace needs to submit DOE requests then that should probably be
> a proper generic driver to submit requests, not raw pci config access.

True, but again as this is exposed through the "normal" config space,
that is not possible.  Again, broken design :(

> > What hardware has this problem that we need to claim exclusive ownership
> > over that differs from the old hardware we used to have that would do
> > crazy things when reading from from userspace?  We had this problem a
> > long time ago and lived with it, what changed now?
> 
> All I can infer from the comments in drivers/pci/access.c is "bad
> things happens on some devices if you allow reads past a certain
> offset", but no concern for reads for offsets less than
> pdev->cfg_size. I think what's changed is that this is the first time
> Linux has had to worry about an awkward polled I/O data transfer
> protocol over config cycles.

I recall many old PCI devices that didn't like their memory being read
at all, and could cause bus lockups, which is why the userspace tools
would print scary warnings if you really wanted to do this.

Feels like we are back to that mess again.

> To make matters worse there appears to be a proliferation of protocols
> being layered on top of DOE. In addition to CXL Table Access for CDAT
> retrieval [2] I'm aware of CXL Compliance Testing [3], Integrity and
> Data Encryption (IDE) [4], and Component Measurement and
> Authentication (CMA) [5].
> 
> I've not read those, but I worry security_locked_down() may want to
> prevent even root userspace mucking with "security" interfaces. So
> that *might* be a reason to ensure exclusive kernel access beyond the
> basic sanity of the kernel being able to have uninterrupted request /
> response sessions with this mailbox
> 
> [2]: https://uefi.org/sites/default/files/resources/Coherent%20Device%20Attribute%20Table_1.01.pdf
> [3]: https://www.computeexpresslink.org/download-the-specification
> [4]: https://members.pcisig.com/wg/PCI-SIG/document/15149
> [5]: https://members.pcisig.com/wg/PCI-SIG/document/14236

I have no access to these proposed specs, sorry.

If things are being layered on top of a broken design, well, they get
what they asked for.

How are the "other" operating systems going to handle this mess?  How
did they "sign off" on this crazy thing?

thanks,

greg k-h

Re: [PATCH] PCI: Allow drivers to claim exclusive access to config regions

[Bjorn Helgaas]

[+cc Christoph]

On Wed, Mar 24, 2021 at 06:23:54PM -0700, Dan Williams wrote:
> The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
> configuration cycles. It assumes one initiator at a time is
> reading/writing the data registers. If userspace reads from the response
> data payload it may steal data that a kernel driver was expecting to
> read. If userspace writes to the request payload it may corrupt the
> request a driver was trying to send.

IIUC the problem we're talking about is that userspace config access,
e.g., via "lspci" or "setpci" may interfere with kernel usage of DOE.
I attached what I think are the relevant bits from the spec.

It looks to me like config *reads* should not be a problem: A read of
Write Data Mailbox always returns 0 and looks innocuous.  A userspace
read of Read Data Mailbox may return a DW of the data object, but it
doesn't advance the cursor, so it shouldn't interfere with a kernel
read.  

A write to Write Data Mailbox could obviously corrupt an object being
written to the device.  A config write to Read Data Mailbox *does*
advance the cursor, so that would definitely interfere with a kernel
user.  

So I think we're really talking about an issue with "setpci" and I
don't expect "lspci" to be a problem.  "setpci" is a valuable tool,
and the fact that it can hose your system is not really news.  I don't
know how hard we should work to protect against that.

Bjorn


From PCIe r6.0 v0.7 draft (sec 7.9.24):

  DOE Control Register

    DOE Go – A write of 1b to this bit indicates to the DOE instance
    that it can start consuming the data object transferred through the
    DOE Write Data Mailbox Register.

    Behavior is undefined if the DOE Go bit is Set before the entire
    data object has been written to the DOE Write Data Mailbox Register.

    Behavior is undefined if the DOE Go bit is written with 1b when the
    DOE Busy bit is Set.

    Reads from this bit must always return 0b.

  DOE Write Data Mailbox Register

    DOE Write Data Mailbox – The DOE instance receives data objects
    via writes to this register.

    A successfully completed write to this register adds one DW to the
    incoming data object.

    Setting the DOE Go bit in the DOE Control Register indicates to
    the DOE Instance that the final DW of the data object has been
    written to this register.

    Reads of this register must return all 0’s.

  DOE Read Data Mailbox Register

    DOE Read Data Mailbox – If the Data Object Ready bit is Set, a
    read of this register returns the current DW of the data object.

    A write of any value to this register indicates a successful
    transfer of the current data object DW, and the DOE instance must
    return the next DW in the data object upon the next read of this
    register as long as the Data Object Ready bit remains Set.

    It is permitted for multiple data objects to be read from this
    register back-to-back. When this scenario occurs the Data Object
    Ready bit will remain Set until the final DW is read.

    A write of any value to this register when the Data Object Ready
    bit is Clear must have no effect.

    The value read from this register when Data Object Ready is Clear
    must be 0000 0000h.

> Introduce pci_{request,release}_config_region() for a driver to exclude
> the possibility of userspace induced corruption while accessing the DOE
> mailbox. Likely there are other configuration state assumptions that a
> driver may want to assert are under its exclusive control, so this
> capability is not limited to any specific configuration range.
> 
> Since writes are targeted and are already prepared for failure the
> entire request is failed. The same can not be done for reads as the
> device completely disappears from lspci output if any configuration
> register in the request is exclusive. Instead skip the actual
> configuration cycle on a per-access basis and return all f's as if the
> read had failed.
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/pci/access.c    |    5 +++--
>  drivers/pci/pci-sysfs.c |    3 +++
>  drivers/pci/probe.c     |    5 +++++
>  include/linux/ioport.h  |    2 ++
>  include/linux/pci.h     |   16 ++++++++++++++++
>  kernel/resource.c       |   24 +++++++++++-------------
>  6 files changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 46935695cfb9..a6b3cdfbd505 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -225,8 +225,9 @@ int pci_user_read_config_##size						\
>  	raw_spin_lock_irq(&pci_lock);				\
>  	if (unlikely(dev->block_cfg_access))				\
>  		pci_wait_cfg(dev);					\
> -	ret = dev->bus->ops->read(dev->bus, dev->devfn,			\
> -					pos, sizeof(type), &data);	\
> +	if (!resource_is_exclusive(&dev->config_resource, pos, sizeof(type))) \
> +		ret = dev->bus->ops->read(dev->bus, dev->devfn,		\
> +					  pos, sizeof(type), &data);	\
>  	raw_spin_unlock_irq(&pci_lock);				\
>  	*val = (type)data;						\
>  	return pcibios_err_to_errno(ret);				\
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index f8afd54ca3e1..4ce2831ffadc 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -753,6 +753,9 @@ static ssize_t pci_write_config(struct file *filp, struct kobject *kobj,
>  	u8 *data = (u8 *) buf;
>  	int ret;
>  
> +	if (resource_is_exclusive(&dev->config_resource, off, count))
> +		return -EBUSY;
> +
>  	ret = security_locked_down(LOCKDOWN_PCI_ACCESS);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 953f15abc850..8fa3a6f38b53 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2240,6 +2240,11 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
>  	INIT_LIST_HEAD(&dev->bus_list);
>  	dev->dev.type = &pci_dev_type;
>  	dev->bus = pci_bus_get(bus);
> +	dev->config_resource = (struct resource) {
> +		.name = "PCI Config",
> +		.start = 0,
> +		.end = -1,
> +	};
>  
>  	return dev;
>  }
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 55de385c839c..e1487a892fe7 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -308,6 +308,8 @@ extern void __devm_release_region(struct device *dev, struct resource *parent,
>  				  resource_size_t start, resource_size_t n);
>  extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size);
>  extern bool iomem_is_exclusive(u64 addr);
> +extern bool resource_is_exclusive(struct resource *resource, u64 addr,
> +				  resource_size_t size);
>  
>  extern int
>  walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 86c799c97b77..d3d78e0df2e7 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -401,6 +401,7 @@ struct pci_dev {
>  	 */
>  	unsigned int	irq;
>  	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
> +	struct resource config_resource;	/* driver exclusive config register ranges */
>  
>  	bool		match_driver;		/* Skip attaching driver */
>  
> @@ -1330,6 +1331,21 @@ int pci_request_selected_regions(struct pci_dev *, int, const char *);
>  int pci_request_selected_regions_exclusive(struct pci_dev *, int, const char *);
>  void pci_release_selected_regions(struct pci_dev *, int);
>  
> +static inline __must_check struct resource *
> +pci_request_config_region(struct pci_dev *pdev, unsigned int where,
> +			  unsigned int len, const char *name)
> +{
> +	return __request_region(&pdev->config_resource, where, len, name,
> +				IORESOURCE_EXCLUSIVE);
> +}
> +
> +static inline void pci_release_config_region(struct pci_dev *pdev,
> +					     unsigned int where,
> +					     unsigned int len)
> +{
> +	__release_region(&pdev->config_resource, where, len);
> +}
> +
>  /* drivers/pci/bus.c */
>  void pci_add_resource(struct list_head *resources, struct resource *res);
>  void pci_add_resource_offset(struct list_head *resources, struct resource *res,
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 627e61b0c124..ab1aed06e8b0 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1706,27 +1706,16 @@ static int strict_iomem_checks;
>  #endif
>  
>  /*
> - * check if an address is reserved in the iomem resource tree
> + * check if an address is reserved in the @resource tree
>   * returns true if reserved, false if not reserved.
>   */
> -bool iomem_is_exclusive(u64 addr)
> +bool resource_is_exclusive(struct resource *p, u64 addr, resource_size_t size)
>  {
> -	struct resource *p = &iomem_resource;
>  	bool err = false;
>  	loff_t l;
> -	int size = PAGE_SIZE;
> -
> -	if (!strict_iomem_checks)
> -		return false;
> -
> -	addr = addr & PAGE_MASK;
>  
>  	read_lock(&resource_lock);
>  	for (p = p->child; p ; p = r_next(NULL, p, &l)) {
> -		/*
> -		 * We can probably skip the resources without
> -		 * IORESOURCE_IO attribute?
> -		 */
>  		if (p->start >= addr + size)
>  			break;
>  		if (p->end < addr)
> @@ -1749,6 +1738,15 @@ bool iomem_is_exclusive(u64 addr)
>  	return err;
>  }
>  
> +bool iomem_is_exclusive(u64 addr)
> +{
> +	if (!strict_iomem_checks)
> +		return false;
> +
> +	return resource_is_exclusive(&iomem_resource, addr & PAGE_MASK,
> +				     PAGE_SIZE);
> +}
> +
>  struct resource_entry *resource_list_create_entry(struct resource *res,
>  						  size_t extra_size)
>  {
> 

Re: [PATCH] PCI: Allow drivers to claim exclusive access to config regions

[Greg Kroah-Hartman]

On Fri, Mar 26, 2021 at 11:12:47AM -0500, Bjorn Helgaas wrote:
> [+cc Christoph]
> 
> On Wed, Mar 24, 2021 at 06:23:54PM -0700, Dan Williams wrote:
> > The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
> > configuration cycles. It assumes one initiator at a time is
> > reading/writing the data registers. If userspace reads from the response
> > data payload it may steal data that a kernel driver was expecting to
> > read. If userspace writes to the request payload it may corrupt the
> > request a driver was trying to send.
> 
> IIUC the problem we're talking about is that userspace config access,
> e.g., via "lspci" or "setpci" may interfere with kernel usage of DOE.
> I attached what I think are the relevant bits from the spec.
> 
> It looks to me like config *reads* should not be a problem: A read of
> Write Data Mailbox always returns 0 and looks innocuous.  A userspace
> read of Read Data Mailbox may return a DW of the data object, but it
> doesn't advance the cursor, so it shouldn't interfere with a kernel
> read.  
> 
> A write to Write Data Mailbox could obviously corrupt an object being
> written to the device.  A config write to Read Data Mailbox *does*
> advance the cursor, so that would definitely interfere with a kernel
> user.  
> 
> So I think we're really talking about an issue with "setpci" and I
> don't expect "lspci" to be a problem.  "setpci" is a valuable tool,
> and the fact that it can hose your system is not really news.  I don't
> know how hard we should work to protect against that.

Thanks for looking this up and letting us know.

So this should be fine, reads are ok, it's not as crazy of a protocol
design as Dan alluded to, so the kernel should be ok.  No need to add
additional "protection" here at all, if you run setpci from userspace,
you get what you asked for :)

thanks,

greg k-h

Re: [PATCH] PCI: Allow drivers to claim exclusive access to config regions

[Dan Williams]

On Sat, Mar 27, 2021 at 3:47 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Mar 26, 2021 at 11:12:47AM -0500, Bjorn Helgaas wrote:
> > [+cc Christoph]
> >
> > On Wed, Mar 24, 2021 at 06:23:54PM -0700, Dan Williams wrote:
> > > The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
> > > configuration cycles. It assumes one initiator at a time is
> > > reading/writing the data registers. If userspace reads from the response
> > > data payload it may steal data that a kernel driver was expecting to
> > > read. If userspace writes to the request payload it may corrupt the
> > > request a driver was trying to send.
> >
> > IIUC the problem we're talking about is that userspace config access,
> > e.g., via "lspci" or "setpci" may interfere with kernel usage of DOE.
> > I attached what I think are the relevant bits from the spec.
> >
> > It looks to me like config *reads* should not be a problem: A read of
> > Write Data Mailbox always returns 0 and looks innocuous.  A userspace
> > read of Read Data Mailbox may return a DW of the data object, but it
> > doesn't advance the cursor, so it shouldn't interfere with a kernel
> > read.
> >
> > A write to Write Data Mailbox could obviously corrupt an object being
> > written to the device.  A config write to Read Data Mailbox *does*
> > advance the cursor, so that would definitely interfere with a kernel
> > user.
> >
> > So I think we're really talking about an issue with "setpci" and I
> > don't expect "lspci" to be a problem.  "setpci" is a valuable tool,
> > and the fact that it can hose your system is not really news.  I don't
> > know how hard we should work to protect against that.
>
> Thanks for looking this up and letting us know.
>
> So this should be fine, reads are ok, it's not as crazy of a protocol
> design as Dan alluded to, so the kernel should be ok.  No need to add
> additional "protection" here at all, if you run setpci from userspace,
> you get what you asked for :)
>

Circling back to this after thinking of the implications and looking
at the review of the DOE code, this situation is different than your
typical "userspace gets to keep the pieces if it does a configuration
write". If we assume well behaved non-malicious userpace, it has no
reason to muck with critical config registers. If userspace changes a
BAR value and the system fails, yup, that's its own fault. The DOE
mailbox is different. There are legitimate reasons why non-broken
userspace would want to read some DOE payloads while the kernel is
retrieving its payloads. It also simplifies the kernel implementation
if it does need to worry about other agents interrupting its
transfers. My mistake was making this restriction apply to reads, but
I'm not on the same page that blocking writes is fruitless just
because userspace can do other damage with config writes.

So either the kernel DOE driver needs to use pci_cfg_access_lock() and
revalidate the state of the DOE after acquiring that lock, or it needs
to claim the interface completely and provide a driver for userspace
to submit requests that can be scheduled in the kernel's DOE queue.

Re: [PATCH] PCI: Allow drivers to claim exclusive access to config regions

[Dan Williams]

On Fri, Mar 26, 2021 at 9:12 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Christoph]
>
> On Wed, Mar 24, 2021 at 06:23:54PM -0700, Dan Williams wrote:
> > The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
> > configuration cycles. It assumes one initiator at a time is
> > reading/writing the data registers. If userspace reads from the response
> > data payload it may steal data that a kernel driver was expecting to
> > read. If userspace writes to the request payload it may corrupt the
> > request a driver was trying to send.
>
> IIUC the problem we're talking about is that userspace config access,
> e.g., via "lspci" or "setpci" may interfere with kernel usage of DOE.
> I attached what I think are the relevant bits from the spec.
>
> It looks to me like config *reads* should not be a problem: A read of
> Write Data Mailbox always returns 0 and looks innocuous.  A userspace
> read of Read Data Mailbox may return a DW of the data object, but it
> doesn't advance the cursor, so it shouldn't interfere with a kernel
> read.
>
> A write to Write Data Mailbox could obviously corrupt an object being
> written to the device.  A config write to Read Data Mailbox *does*
> advance the cursor, so that would definitely interfere with a kernel
> user.
>
> So I think we're really talking about an issue with "setpci" and I
> don't expect "lspci" to be a problem.  "setpci" is a valuable tool,
> and the fact that it can hose your system is not really news.  I don't
> know how hard we should work to protect against that.

True, the threat is smaller than I was reading, I apologize for that
noise. Temporary blocking over kernel DOE cycles seems sufficient for
now.

Re: [PATCH] PCI: Allow drivers to claim exclusive access to config regions

[Jonathan Cameron]

On Mon, 29 Mar 2021 09:46:15 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Fri, Mar 26, 2021 at 9:12 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > [+cc Christoph]
> >
> > On Wed, Mar 24, 2021 at 06:23:54PM -0700, Dan Williams wrote:  
> > > The PCIE Data Object Exchange (DOE) mailbox is a protocol run over
> > > configuration cycles. It assumes one initiator at a time is
> > > reading/writing the data registers. If userspace reads from the response
> > > data payload it may steal data that a kernel driver was expecting to
> > > read. If userspace writes to the request payload it may corrupt the
> > > request a driver was trying to send.  
> >
> > IIUC the problem we're talking about is that userspace config access,
> > e.g., via "lspci" or "setpci" may interfere with kernel usage of DOE.
> > I attached what I think are the relevant bits from the spec.
> >
> > It looks to me like config *reads* should not be a problem: A read of
> > Write Data Mailbox always returns 0 and looks innocuous.  A userspace
> > read of Read Data Mailbox may return a DW of the data object, but it
> > doesn't advance the cursor, so it shouldn't interfere with a kernel
> > read.
> >
> > A write to Write Data Mailbox could obviously corrupt an object being
> > written to the device.  A config write to Read Data Mailbox *does*
> > advance the cursor, so that would definitely interfere with a kernel
> > user.
> >
> > So I think we're really talking about an issue with "setpci" and I
> > don't expect "lspci" to be a problem.  "setpci" is a valuable tool,
> > and the fact that it can hose your system is not really news.  I don't
> > know how hard we should work to protect against that.  
> 
> True, the threat is smaller than I was reading, I apologize for that
> noise. Temporary blocking over kernel DOE cycles seems sufficient for
> now.

Was on vacation. Glad I read the whole thread before replying.  The
key point as Bjorn identified is that reads don't hurt and as noted
setpci can break many things anyway so I think we are safe on this.

There are more 'exciting' questions to be addressed about firmware
vs OS ownership of particular DOE mailboxes but that's a whole
separate bit of fun for the future.

Jonathan