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-next>] [day] [month] [year] [list]
Message-Id: <20250123033716.112115-1-tdave@nvidia.com>
Date: Wed, 22 Jan 2025 19:37:16 -0800
From: Tushar Dave <tdave@...dia.com>
To: jgg@...dia.com,
	corbet@....net,
	bhelgaas@...gle.com,
	paulmck@...nel.org,
	akpm@...ux-foundation.org,
	thuth@...hat.com,
	rostedt@...dmis.org,
	xiongwei.song@...driver.com,
	vidyas@...dia.com,
	linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org
Cc: vsethi@...dia.com,
	sdonthineni@...dia.com,
	Tushar Dave <tdave@...dia.com>
Subject: [PATCH v2] PCI: Fix Extend ACS configurability

Commit 47c8846a49ba ("PCI: Extend ACS configurability") introduced
bugs that fail to configure ACS ctrl to the value specified by the
kernel parameter. Essentially there are two bugs.

First, when ACS is configured for multiple PCI devices using
'config_acs' kernel parameter, it results into error "PCI: Can't parse
ACS command line parameter". This is due to the bug in code that doesn't
preserve the ACS mask instead overwrites the mask with value 0.

For example, using 'config_acs' to configure ACS ctrl for multiple BDFs
fails:

	Kernel command line: pci=config_acs=1111011@...0:02:00.0;101xxxx@...9:00:00.0 "dyndbg=file drivers/pci/pci.c +p"
	PCI: Can't parse ACS command line parameter
	pci 0020:02:00.0: ACS mask  = 0x007f
	pci 0020:02:00.0: ACS flags = 0x007b
	pci 0020:02:00.0: Configured ACS to 0x007b

After this fix:

	Kernel command line: pci=config_acs=1111011@...0:02:00.0;101xxxx@...9:00:00.0 "dyndbg=file drivers/pci/pci.c +p"
	pci 0020:02:00.0: ACS mask  = 0x007f
	pci 0020:02:00.0: ACS flags = 0x007b
	pci 0020:02:00.0: ACS control = 0x005f
	pci 0020:02:00.0: ACS fw_ctrl = 0x0053
	pci 0020:02:00.0: Configured ACS to 0x007b
	pci 0039:00:00.0: ACS mask  = 0x0070
	pci 0039:00:00.0: ACS flags = 0x0050
	pci 0039:00:00.0: ACS control = 0x001d
	pci 0039:00:00.0: ACS fw_ctrl = 0x0000
	pci 0039:00:00.0: Configured ACS to 0x0050

Second bug is in the bit manipulation logic where we copy the bit from
the firmware settings when mask bit 0.

For example, 'disable_acs_redir' fails to clear all three ACS P2P redir
bits due to the wrong bit fiddling:

	Kernel command line: pci=disable_acs_redir=0020:02:00.0;0030:02:00.0;0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p"
	pci 0020:02:00.0: ACS mask  = 0x002c
	pci 0020:02:00.0: ACS flags = 0xffd3
	pci 0020:02:00.0: Configured ACS to 0xfffb
	pci 0030:02:00.0: ACS mask  = 0x002c
	pci 0030:02:00.0: ACS flags = 0xffd3
	pci 0030:02:00.0: Configured ACS to 0xffdf
	pci 0039:00:00.0: ACS mask  = 0x002c
	pci 0039:00:00.0: ACS flags = 0xffd3
	pci 0039:00:00.0: Configured ACS to 0xffd3

After this fix:

	Kernel command line: pci=disable_acs_redir=0020:02:00.0;0030:02:00.0;0039:00:00.0 "dyndbg=file drivers/pci/pci.c +p"
	pci 0020:02:00.0: ACS mask  = 0x002c
	pci 0020:02:00.0: ACS flags = 0xffd3
	pci 0020:02:00.0: ACS control = 0x007f
	pci 0020:02:00.0: ACS fw_ctrl = 0x007b
	pci 0020:02:00.0: Configured ACS to 0x0053
	pci 0030:02:00.0: ACS mask  = 0x002c
	pci 0030:02:00.0: ACS flags = 0xffd3
	pci 0030:02:00.0: ACS control = 0x005f
	pci 0030:02:00.0: ACS fw_ctrl = 0x005f
	pci 0030:02:00.0: Configured ACS to 0x0053
	pci 0039:00:00.0: ACS mask  = 0x002c
	pci 0039:00:00.0: ACS flags = 0xffd3
	pci 0039:00:00.0: ACS control = 0x001d
	pci 0039:00:00.0: ACS fw_ctrl = 0x0000
	pci 0039:00:00.0: Configured ACS to 0x0000

Fixes: 47c8846a49ba ("PCI: Extend ACS configurability")
Signed-off-by: Tushar Dave <tdave@...dia.com>
---

changes in v2:
 - Addressed review comments by Jason and Bjorn.
 - Removed Documentation changes (already taken care by other patch).
 - Amended commit description.

 drivers/pci/pci.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0b29ec6e8e5e..19fbdd8643bc 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -955,8 +955,10 @@ struct pci_acs {
 };
 
 static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
-			     const char *p, u16 mask, u16 flags)
+			     const char *p, const u16 acs_mask, const u16 acs_flags)
 {
+	u16 flags = acs_flags;
+	u16 mask = acs_mask;
 	char *delimit;
 	int ret = 0;
 
@@ -964,7 +966,7 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
 		return;
 
 	while (*p) {
-		if (!mask) {
+		if (!acs_mask) {
 			/* Check for ACS flags */
 			delimit = strstr(p, "@");
 			if (delimit) {
@@ -972,6 +974,8 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
 				u32 shift = 0;
 
 				end = delimit - p - 1;
+				mask = 0;
+				flags = 0;
 
 				while (end > -1) {
 					if (*(p + end) == '0') {
@@ -1028,10 +1032,13 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
 
 	pci_dbg(dev, "ACS mask  = %#06x\n", mask);
 	pci_dbg(dev, "ACS flags = %#06x\n", flags);
+	pci_dbg(dev, "ACS control = %#06x\n", caps->ctrl);
+	pci_dbg(dev, "ACS fw_ctrl = %#06x\n", caps->fw_ctrl);
 
-	/* If mask is 0 then we copy the bit from the firmware setting. */
-	caps->ctrl = (caps->ctrl & ~mask) | (caps->fw_ctrl & mask);
-	caps->ctrl |= flags;
+	/* For mask bits that are 0 copy them from the firmware setting
+	 * and apply flags for all the mask bits that are 1.
+	 */
+	caps->ctrl = (caps->fw_ctrl & ~mask) | (flags & mask);
 
 	pci_info(dev, "Configured ACS to %#06x\n", caps->ctrl);
 }
-- 
2.34.1


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ