| Message ID | SI2PR01MB439380682B8703CB5340AD59DCB62@SI2PR01MB4393.apcprd01.prod.exchangelabs.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Add support for ACS Enhanced Capability | expand |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Pre-existing issues: - [High] This problem wasn't introduced by this patch, but user-provided ACS command line parameters silently wipe out standard IOMMU protections (SV, TB, UF) for unspecified bits by reverting to hardware defaults. -- commit 62951886e4f26a51d147451d4aa7384b487c2ff6 Author: Wei Wang <wei.w.wang@hotmail.com> PCI: Refactor disable_acs_redir and config_acs param handling This commit splits the intertwined disable_acs_redir and config_acs parameter handling inside __pci_config_acs() into dedicated functions. It also improves parameter parsing by checking for a matching device before parsing flags and replacing complex string parsing with cleaner loops and switch statements. > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 587c013f04db3..6cffdbba99d34 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c [ ... ] > +static void pci_param_disable_acs_redir(struct pci_dev *dev, > + struct pci_acs *caps) > +{ > + const u16 acs_redir_mask = PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC; > + > + if (!disable_acs_redir_param || > + !pci_dev_match_disable_acs_redir(dev, disable_acs_redir_param) || > + !pci_dev_specific_disable_acs_redir(dev)) > return; > > + caps->ctrl = caps->fw_ctrl & ~acs_redir_mask; [Severity: High] This isn't a bug introduced by this patch, but does this overwrite silently discard the standard IOMMU protections (SV, TB, UF) established earlier? When an administrator uses pci=disable_acs_redir=..., pci_enable_acs() first enables OS-level protections via pci_std_enable_acs(dev, &caps). However, because this assignment bases the new control register completely on caps->fw_ctrl, any previously configured security bits are erased. This breaks PCIe isolation guarantees for the affected devices. [ ... ] > +static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps) > +{ [ ... ] > + invalid_bits = (enabled_bits | disabled_bits) & ~valid_ctrl; > + if (invalid_bits) { > + pci_dbg(dev, "Ignoring unsupported ACS bits: %#06x\n", > + invalid_bits); > + enabled_bits &= valid_ctrl; > + disabled_bits &= valid_ctrl; > + } > > - 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); > + pci_dbg(dev, "ACS enabled: %#06x, disabled: %#06x\n", > + enabled_bits, disabled_bits); > > - /* > - * 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); > + caps->ctrl = (caps->fw_ctrl | enabled_bits) & ~disabled_bits; [Severity: High] This is a pre-existing issue, but does this assignment also inadvertently wipe out the IOMMU protections set up by pci_std_enable_acs()? Similar to pci_param_disable_acs_redir(), when pci=config_acs=... is used, all un-masked bits revert to their fw_ctrl hardware default state rather than persisting their pci_std_enable_acs() modifications. This could potentially allow DMA spoofing or unauthorized memory access on devices where the admin only intended to tweak specific ACS bits.
On 9/3/26 12:26 PM, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: > > Pre-existing issues: > - [High] This problem wasn't introduced by this patch, but user-provided ACS command line parameters silently wipe out standard IOMMU protections (SV, TB, UF) for unspecified bits by reverting to hardware defaults. I plan to keep the patch as is unless someone sees a concrete case that warrants a behavior change. > -- > > commit 62951886e4f26a51d147451d4aa7384b487c2ff6 > Author: Wei Wang <wei.w.wang@hotmail.com> > > PCI: Refactor disable_acs_redir and config_acs param handling > > This commit splits the intertwined disable_acs_redir and config_acs parameter > handling inside __pci_config_acs() into dedicated functions. It also improves > parameter parsing by checking for a matching device before parsing flags and > replacing complex string parsing with cleaner loops and switch statements. > >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 587c013f04db3..6cffdbba99d34 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c > > [ ... ] > >> +static void pci_param_disable_acs_redir(struct pci_dev *dev, >> + struct pci_acs *caps) >> +{ >> + const u16 acs_redir_mask = PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC; >> + >> + if (!disable_acs_redir_param || >> + !pci_dev_match_disable_acs_redir(dev, disable_acs_redir_param) || >> + !pci_dev_specific_disable_acs_redir(dev)) >> return; >> >> + caps->ctrl = caps->fw_ctrl & ~acs_redir_mask; > > [Severity: High] > This isn't a bug introduced by this patch, but does this overwrite silently > discard the standard IOMMU protections (SV, TB, UF) established earlier? > > When an administrator uses pci=disable_acs_redir=..., pci_enable_acs() > first enables OS-level protections via pci_std_enable_acs(dev, &caps). > > However, because this assignment bases the new control register completely on > caps->fw_ctrl, any previously configured security bits are erased. This breaks > PCIe isolation guarantees for the affected devices. > This mainly addresses how to handle the "x" bit. kernel-parameters.txt mentions: " pci=config_acs=10x@pci:0:0 would configure all devices that support ACS to enable P2P Request Redirect, disable Translation Blocking, and leave Source Validation unchanged from whatever power-up or firmware set it to. " So the original intent was to leave the state as the firmware left it. Regarding IOMMU grouping, the impact should be minimal since groups are made later from the register as actually written (via pci_acs_enabled()), so there is no false claim of isolation. > [ ... ] > >> +static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps) >> +{ > > [ ... ] > >> + invalid_bits = (enabled_bits | disabled_bits) & ~valid_ctrl; >> + if (invalid_bits) { >> + pci_dbg(dev, "Ignoring unsupported ACS bits: %#06x\n", >> + invalid_bits); >> + enabled_bits &= valid_ctrl; >> + disabled_bits &= valid_ctrl; >> + } >> >> - 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); >> + pci_dbg(dev, "ACS enabled: %#06x, disabled: %#06x\n", >> + enabled_bits, disabled_bits); >> >> - /* >> - * 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); >> + caps->ctrl = (caps->fw_ctrl | enabled_bits) & ~disabled_bits; > > [Severity: High] > This is a pre-existing issue, but does this assignment also inadvertently wipe > out the IOMMU protections set up by pci_std_enable_acs()? > > Similar to pci_param_disable_acs_redir(), when pci=config_acs=... is used, > all un-masked bits revert to their fw_ctrl hardware default state rather than > persisting their pci_std_enable_acs() modifications. > > This could potentially allow DMA spoofing or unauthorized memory access on > devices where the admin only intended to tweak specific ACS bits. > Similar to the above, RR/CR are already cleared by the admin's request, so subsequent device grouping will work as expected.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 587c013f04db..6cffdbba99d3 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -915,90 +915,121 @@ struct pci_acs { u16 fw_ctrl; }; -static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps, - const char *p, const u16 acs_mask, const u16 acs_flags) +static bool pci_dev_match_disable_acs_redir(struct pci_dev *dev, const char *p) { - u16 valid_ctrl = dev->acs_capabilities & GENMASK_U16(6, 0); - u16 flags = acs_flags; - u16 mask = acs_mask; - char *delimit; int ret = 0; - if (!p) - return; - while (*p) { - if (!acs_mask) { - /* Check for ACS flags */ - delimit = strstr(p, "@"); - if (delimit) { - int end; - u32 shift = 0; - - end = delimit - p - 1; - mask = 0; - flags = 0; - - while (end > -1) { - if (*(p + end) == '0') { - mask |= 1 << shift; - shift++; - end--; - } else if (*(p + end) == '1') { - mask |= 1 << shift; - flags |= 1 << shift; - shift++; - end--; - } else if ((*(p + end) == 'x') || (*(p + end) == 'X')) { - shift++; - end--; - } else { - pci_err(dev, "Invalid ACS flags... Ignoring\n"); - return; - } - } - p = delimit + 1; - } else { - pci_err(dev, "ACS Flags missing\n"); - return; - } - } - ret = pci_dev_str_match(dev, p, &p); - if (ret < 0) { - pr_warn_once("PCI: Can't parse ACS command line parameter\n"); - break; - } else if (ret == 1) { - /* Found a match */ + if (ret == 1) { + return true; + } else if (ret < 0) { + pr_warn_once("PCI: Can't parse disable_acs_redir param\n"); break; } } - if (ret != 1) + return false; +} + +static void pci_param_disable_acs_redir(struct pci_dev *dev, + struct pci_acs *caps) +{ + const u16 acs_redir_mask = PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC; + + if (!disable_acs_redir_param || + !pci_dev_match_disable_acs_redir(dev, disable_acs_redir_param) || + !pci_dev_specific_disable_acs_redir(dev)) return; - if (!pci_dev_specific_disable_acs_redir(dev)) - return; + caps->ctrl = caps->fw_ctrl & ~acs_redir_mask; +} - if (!acs_mask && (mask & ~valid_ctrl)) { - pci_dbg(dev, "Ignoring unsupported ACS bits: %#06x\n", - mask & ~valid_ctrl); - mask &= valid_ctrl; - flags &= valid_ctrl; - } - - 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); +static const char *pci_dev_match_config_acs(struct pci_dev *dev, const char *p) +{ + const char *seg; + int ret; /* - * For mask bits that are 0, copy them from the firmware setting - * and apply flags for all the mask bits that are 1. + * Example: 11101010x@0000:01:00.0;101x@0000:02:00.0;1@0000:03:00.0 + * Three segments delimited via ';'. @seg always points to the start + * of a segment and p advances to the start of the device id (BDF) + * after '@'. Upon match, return the start of the matching segment. */ - caps->ctrl = (caps->fw_ctrl & ~mask) | (flags & mask); + while (*p) { + seg = p; + p = strchr(seg, '@'); + /* If malformed string, stop parsing. */ + if (!p) { + pci_err(dev, "ACS Flags missing\n"); + break; + } - pci_info(dev, "Configured ACS to %#06x\n", caps->ctrl); + p++; + /* pci_dev_str_match() updates p to the next segment. */ + ret = pci_dev_str_match(dev, p, &p); + if (ret == 1) + return seg; + + if (ret < 0) { + pr_warn_once("PCI: Can't parse config_acs param\n"); + break; + } + } + + return NULL; +} + +static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps) +{ + u16 shift = 0, valid_ctrl = dev->acs_capabilities & GENMASK_U16(6, 0); + u16 invalid_bits, enabled_bits = 0, disabled_bits = 0; + const char *p, *seg; + + if (!config_acs_param || !valid_ctrl) + return; + + seg = pci_dev_match_config_acs(dev, config_acs_param); + if (!seg) + return; + + p = strchr(seg, '@'); + /* Parse bitstring backwards from '@' */ + while (p > seg) { + if (shift >= 16) { + pci_err(dev, "ACS bitstring exceeds 16 bits\n"); + return; + } + switch (*--p) { + case '1': + enabled_bits |= BIT(shift); + break; + case '0': + disabled_bits |= BIT(shift); + break; + case 'x': + case 'X': + /* Skip this bit */ + break; + default: + pci_err(dev, "Invalid ACS flag character '%c'\n", *p); + return; + } + shift++; + } + + invalid_bits = (enabled_bits | disabled_bits) & ~valid_ctrl; + if (invalid_bits) { + pci_dbg(dev, "Ignoring unsupported ACS bits: %#06x\n", + invalid_bits); + enabled_bits &= valid_ctrl; + disabled_bits &= valid_ctrl; + } + + pci_dbg(dev, "ACS enabled: %#06x, disabled: %#06x\n", + enabled_bits, disabled_bits); + + caps->ctrl = (caps->fw_ctrl | enabled_bits) & ~disabled_bits; } /** @@ -1033,6 +1064,7 @@ void pci_enable_acs(struct pci_dev *dev) { struct pci_acs caps; bool enable_acs = false; + u16 kernel_default_ctrl; int pos; /* If an iommu is present we start with kernel default caps */ @@ -1051,14 +1083,18 @@ void pci_enable_acs(struct pci_dev *dev) if (enable_acs) pci_std_enable_acs(dev, &caps); + kernel_default_ctrl = caps.ctrl; + /* * Always apply caps from the command line, even if there is no iommu. * Trust that the admin has a reason to change the ACS settings. */ - __pci_config_acs(dev, &caps, disable_acs_redir_param, - PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC, - ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC)); - __pci_config_acs(dev, &caps, config_acs_param, 0, 0); + pci_param_disable_acs_redir(dev, &caps); + pci_param_config_acs(dev, &caps); + + if (caps.ctrl != kernel_default_ctrl) + pci_info(dev, "User configured ACS to %#06x (default: %#06x)\n", + caps.ctrl, kernel_default_ctrl); pci_write_config_word(dev, pos + PCI_ACS_CTRL, caps.ctrl); }
The current implementation mixes disable_acs_redir and config_acs param processing inside __pci_config_acs(), using acs_mask==0 as an implicit signal to switch into config_acs_param mode. The intertwined logic is hard to follow and easy to break: - The acs_mask==0 special case is non-obvious and obscures which code paths apply to which parameter. - The interleaved logic is fragile; changes intended for one parameter can unintentionally affect the other. For example, pci_dev_specific_disable_acs_redir() is invoked on the common path even though it should apply only to disable_acs_redir. Split the two behaviors into dedicated functions, pci_param_disable_acs_redir() and pci_param_config_acs(), making the control flow explicit and easier to maintain. The new pci_param_config_acs() implementation also improves on the original logic: - It searches for the matching device first and parses ACS flags only for the matched device, avoiding unnecessary work. - A switch statement replaces multiple if/else blocks, improving readability. - Variable names (enabled_bits, disabled_bits) more clearly express their roles compared to the previous mask/flags pair. - Error messages are clearer. This refactor separates concerns, improves robustness, and makes future extensions to ACS parameter handling safer and easier to review. Signed-off-by: Wei Wang <wei.w.wang@hotmail.com> --- drivers/pci/pci.c | 180 +++++++++++++++++++++++++++------------------- 1 file changed, 108 insertions(+), 72 deletions(-)