On Fri, 5 Sept 2025 at 07:28, Kees Cook kees@kernel.org wrote:
After commit cbc654d18d37 ("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. :)
Fixes: cbc654d18d37 ("bitops: Add __attribute_const__ to generic ffs()-family implementations") Reported-by: Linux Kernel Functional Testing lkft@linaro.org Closes: https://lore.kernel.org/linux-pci/CA+G9fYuysVr6qT8bjF6f08WLyCJRG7aXAeSd2F7=z... Signed-off-by: Kees Cook kees@kernel.org
Cc: Bjorn Helgaas bhelgaas@google.com Cc: Anders Roxell anders.roxell@linaro.org Cc: Naresh Kamboju naresh.kamboju@linaro.org Cc: lkft-triage@lists.linaro.org Cc: Linux Regressions regressions@lists.linux.dev Cc: Arnd Bergmann arnd@arndb.de Cc: Dan Carpenter dan.carpenter@linaro.org Cc: Ben Copeland benjamin.copeland@linaro.org Cc: lkft-triage@lists.linaro.org Cc: linux-pci@vger.kernel.org Cc: linux-kernel@vger.kernel.org
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 b0f4d98036cd..005b92e6585e 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);
Hi Kees,
Thank you for looking into this.
These warnings are not a one time thing. the later versions of gcc can figure it out that firstbit is at least 8 based on the "rq < 128" (i guess), so we're adding bogus code. maybe we should just disable the check for gcc-8.
Maybe something like this:
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h index 5355f8f806a9..4716025c98c7 100644 --- a/include/linux/bitfield.h +++ b/include/linux/bitfield.h @@ -65,9 +65,20 @@ BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \ _pfx "mask is not constant"); \ BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \ - BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \ - ~((_mask) >> __bf_shf(_mask)) & \ - (0 + (_val)) : 0, \ + /* Value validation disabled for gcc < 9 due to __attribute_const__ issues. + */ \ + BUILD_BUG_ON_MSG(__GNUC__ >= 9 && __builtin_constant_p(_val) ? \ + ~((_mask) >> __bf_shf(_mask)) & \ + (0 + (_val)) : 0, \ _pfx "value too large for the field"); \ BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \ __bf_cast_unsigned(_reg, ~0ull), \
I found similar patterns with ffs and FIELD_PREP here drivers/dma/uniphier-xdmac.c row 156 and 165 drivers/gpu/drm/i915/display/intel_cursor_regs.h row 17
Cheers, Anders