[PATCH AUTOSEL 6.17-6.12] PCI: Test for bit underflow in pcie_set_readrq()

[Sasha Levin <sashal@kernel.org>]

From: Kees Cook <kees@kernel.org>

[ Upstream commit 00e58ff924b3a684b076f9512fe2753be87b50e1 ]

In preparation for the future commit ("bitops: Add __attribute_const__ to generic
ffs()-family implementations"), which allows GCC's value range tracker
to see past ffs(), GCC 8 on ARM thinks that it might be possible that
"ffs(rq) - 8" used here:

	v = FIELD_PREP(PCI_EXP_DEVCTL_READRQ, ffs(rq) - 8);

could wrap below 0, leading to a very large value, which would be out of
range for the FIELD_PREP() usage:

drivers/pci/pci.c: In function 'pcie_set_readrq':
include/linux/compiler_types.h:572:38: error: call to '__compiletime_assert_471' declared with attribute error: FIELD_PREP: value too large for the field
...
drivers/pci/pci.c:5896:6: note: in expansion of macro 'FIELD_PREP'
  v = FIELD_PREP(PCI_EXP_DEVCTL_READRQ, ffs(rq) - 8);
      ^~~~~~~~~~

If the result of the ffs() is bounds checked before being used in
FIELD_PREP(), the value tracker seems happy again. :)

Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Closes: https://lore.kernel.org/linux-pci/CA+G9fYuysVr6qT8bjF6f08WLyCJRG7aXAeSd2F7=zTaHHd7L+Q@mail.gmail.com/
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Link: https://lore.kernel.org/r/20250905052836.work.425-kees@kernel.org
Signed-off-by: Kees Cook <kees@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Based on my comprehensive analysis, here is my determination:

## Backport Status: **YES**

### Extensive Analysis:

#### Nature of the Fix:
This commit adds a defensive check in `pcie_set_readrq()` to prevent a
potential bit underflow when computing `ffs(rq) - 8`. Specifically, it:
1. Stores the result of `ffs(rq)` in a new variable `firstbit`
2. Adds a check: `if (firstbit < 8) return -EINVAL;`
3. Uses `firstbit - 8` instead of `ffs(rq) - 8` in the FIELD_PREP macro

#### Root Cause & Context:
This is **not a runtime bug fix** - it's a **build fix** triggered by
another optimization commit. The bitops commit (50675b8f5bd4e "bitops:
Add __attribute_const__ to generic ffs()-family implementations") adds
`__attribute_const__` to ffs() functions, enabling GCC's value range
tracker to perform better static analysis. However, GCC-8's conservative
analysis on ARM, RISCV, and MIPS architectures incorrectly determines
that `ffs(rq) - 8` could underflow, causing a **compilation error** (not
a warning):

```
drivers/pci/pci.c: In function 'pcie_set_readrq':
include/linux/compiler_types.h:572:38: error: call to
'__compiletime_assert_471'
declared with attribute error: FIELD_PREP: value too large for the field
```

#### Why the Compiler is Wrong (but we still need to fix it):
Examining the code flow in `pcie_set_readrq()`
(drivers/pci/pci.c:5931-5968):
1. Initial validation: `if (rq < 128 || rq > 4096 ||
   !is_power_of_2(rq))` ensures rq ≥ 128
2. Performance mode clamping: `if (mps < rq) rq = mps;` where mps comes
   from `pcie_get_mps()`
3. `pcie_get_mps()` returns `128 << FIELD_GET(...)`, which is always ≥
   128 (verified at line 5976-5984)
4. Therefore, `ffs(rq) >= ffs(128) = 8`, so underflow is impossible

However, since commit f67577118d115 (2013), `pcie_get_mps()` never
returns an error, always returning valid values ≥ 128. The compiler
cannot prove this through interprocedural analysis.

#### Critical Dependency:
This commit is **tightly coupled** with the bitops commit. Evidence:
- Both commits are signed off by Sasha Levin (autosel backports)
- They appear consecutively in the git history (50675b8f5bd4e →
  5385aceb86f2f)
- The commit message explicitly states: "In preparation for the future
  commit"
- Without this fix, **builds will fail** on GCC-8 ARM/RISCV/MIPS after
  bitops changes

#### Risk Assessment:
**Minimal Risk:**
- Small, localized change (6 lines added in one function)
- Adds defensive validation that cannot break existing functionality
- For all valid inputs (rq ≥ 128), the check passes through
- Only rejects values that would have caused incorrect behavior anyway
- No performance impact
- No changes to critical kernel subsystems beyond PCI

#### Backporting Justification:
1. **Mandatory dependency**: Required if bitops commit is backported
   (which it is - commit 50675b8f5bd4e)
2. **Build fix**: Prevents compilation failures on supported compiler
   configurations
3. **Low risk**: Defensive check with no behavior change for valid
   inputs
4. **Follows stable rules**: Small, contained fix with clear purpose
5. **Multiple architectures affected**: ARM, RISCV, MIPS with GCC-8
6. **Upstream acceptance**: Acked by PCI maintainer (Bjorn Helgaas) and
   Arnd Bergmann

#### Supporting Evidence:
- Reported by: Linux Kernel Functional Testing (LKFT)
- Affects: GCC-8 on arm, riscv, mips architectures
- First seen: Linux 6.17.0-rc3-next-20250828
- Bisected to: bitops __attribute_const__ commit
- Acked-by: Bjorn Helgaas, Arnd Bergmann

### Conclusion:
**YES - This commit should be backported** as it's a necessary build fix
that must accompany the bitops optimization commit. Without it, stable
kernels with the bitops changes will fail to build on GCC-8
ARM/RISCV/MIPS configurations, breaking supported build environments.
The fix is minimal, defensive, and poses negligible regression risk.

 drivers/pci/pci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b0f4d98036cdd..005b92e6585e9 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5932,6 +5932,7 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
 {
 	u16 v;
 	int ret;
+	unsigned int firstbit;
 	struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
 
 	if (rq < 128 || rq > 4096 || !is_power_of_2(rq))
@@ -5949,7 +5950,10 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
 			rq = mps;
 	}
 
-	v = FIELD_PREP(PCI_EXP_DEVCTL_READRQ, ffs(rq) - 8);
+	firstbit = ffs(rq);
+	if (firstbit < 8)
+		return -EINVAL;
+	v = FIELD_PREP(PCI_EXP_DEVCTL_READRQ, firstbit - 8);
 
 	if (bridge->no_inc_mrrs) {
 		int max_mrrs = pcie_get_readrq(dev);
-- 
2.51.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv