[PATCH] PCI: Test for bit underflow in pcie_set_readrq()

[PATCH] PCI: Test for bit underflow in pcie_set_readrq()

[Kees Cook]

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=zTaHHd7L+Q@mail.gmail.com/
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);
 
 	if (bridge->no_inc_mrrs) {
 		int max_mrrs = pcie_get_readrq(dev);
-- 
2.34.1

Re: [PATCH] PCI: Test for bit underflow in pcie_set_readrq()

[Anders Roxell]

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=zTaHHd7L+Q@mail.gmail.com/
> 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

Re: [PATCH] PCI: Test for bit underflow in pcie_set_readrq()

[Arnd Bergmann]

On Fri, Sep 5, 2025, at 10:16, Anders Roxell wrote:
> On Fri, 5 Sept 2025 at 07:28, Kees Cook <kees@kernel.org> wrote:
>> @@ -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.

Out of the three failures I saw, two also happened with gcc-9, but
gcc-10 looks clean so far.

>          \
> +                                       (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

I did not come across build failures for these.

    Arnd

Re: [PATCH] PCI: Test for bit underflow in pcie_set_readrq()

[Kees Cook]

On Fri, Sep 05, 2025 at 10:16:33AM +0200, Anders Roxell wrote:
> > -       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.

I think the issue is that GCC thinks it knows the range for ffs is not
the entire [0..UINT_MAX], but it _doesn't_ know how "rq" affects the
outcome. (The range checker warnings kick in when it's not the whole
range of a given type.) But I am just guessing, based on what how I've
seen in behave in the past.

> 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

You got warnings for these?

-- 
Kees Cook

Re: [PATCH] PCI: Test for bit underflow in pcie_set_readrq()

[Arnd Bergmann]

On Fri, Sep 5, 2025, at 07:28, Kees Cook 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=zTaHHd7L+Q@mail.gmail.com/
> Signed-off-by: Kees Cook <kees@kernel.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

This looks good to me individually, however I have now tried to
do more randconfig tests with the __attribute_const change
and gcc-8.5.0, and so far found two other files with the
same issue:

In file included from <command-line>:
In function 'mtk_dai_etdm_out_configure.constprop',
    inlined from 'mtk_dai_etdm_configure' at sound/soc/mediatek/mt8188/mt8188-dai-etdm.c:2168:3:
include/linux/compiler_types.h:575:38: error: call to '__compiletime_assert_416' declared with attribute error: FIELD_PREP: value too large for the field
sound/soc/mediatek/mt8188/mt8188-dai-etdm.c:2065:10: note: in expansion of macro 'FIELD_PREP'
   val |= FIELD_PREP(ETDM_OUT_CON4_FS_MASK, get_etdm_fs_timing(rate));
sound/soc/mediatek/mt8188/mt8188-dai-etdm.c:1971:10: note: in expansion of macro 'FIELD_PREP'
   val |= FIELD_PREP(ETDM_IN_CON3_FS_MASK, get_etdm_fs_timing(rate));

drivers/mmc/host/meson-gx-mmc.c: In function 'meson_mmc_start_cmd':
drivers/mmc/host/meson-gx-mmc.c:811:14: note: in expansion of macro 'FIELD_PREP'
   cmd_cfg |= FIELD_PREP(CMD_CFG_TIMEOUT_MASK,

This is fairly rare, but over time there are likely going to be
others like them. I see three possible ways forward here:

a) fix them individually as we run into them, hoping for the best
b) skip that one compile time check on older compilers, like Anders'
   new patch
c) try to come up with a more robust FIELD_PREP() implementation

I already tried hacking on FIELD_PREP(), but my feeling is that it
is already getting out of hand with its complexity, and needs to
become simpler instead. The root cause for the warning here is
apparently the way it evaluates the macro arguments multiple times,
and that causes both extra compile-time complexity and extra code
paths where part of the invocation goes through an inline function
and another path that does not. It may be possible to change this
in a way that moves the BUILD_BUG_ON() portions into an __always_inline
function, and only uses the macro to deal with the type differences.

     Arnd

Re: [PATCH] PCI: Test for bit underflow in pcie_set_readrq()

[Kees Cook]

On Fri, Sep 05, 2025 at 10:25:49AM +0200, Arnd Bergmann wrote:
> On Fri, Sep 5, 2025, at 07:28, Kees Cook 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=zTaHHd7L+Q@mail.gmail.com/
> > Signed-off-by: Kees Cook <kees@kernel.org>
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> This looks good to me individually, however I have now tried to
> do more randconfig tests with the __attribute_const change
> and gcc-8.5.0, and so far found two other files with the
> same issue:
> 
> In file included from <command-line>:
> In function 'mtk_dai_etdm_out_configure.constprop',
>     inlined from 'mtk_dai_etdm_configure' at sound/soc/mediatek/mt8188/mt8188-dai-etdm.c:2168:3:
> include/linux/compiler_types.h:575:38: error: call to '__compiletime_assert_416' declared with attribute error: FIELD_PREP: value too large for the field
> sound/soc/mediatek/mt8188/mt8188-dai-etdm.c:2065:10: note: in expansion of macro 'FIELD_PREP'
>    val |= FIELD_PREP(ETDM_OUT_CON4_FS_MASK, get_etdm_fs_timing(rate));
> sound/soc/mediatek/mt8188/mt8188-dai-etdm.c:1971:10: note: in expansion of macro 'FIELD_PREP'
>    val |= FIELD_PREP(ETDM_IN_CON3_FS_MASK, get_etdm_fs_timing(rate));
> 
> drivers/mmc/host/meson-gx-mmc.c: In function 'meson_mmc_start_cmd':
> drivers/mmc/host/meson-gx-mmc.c:811:14: note: in expansion of macro 'FIELD_PREP'
>    cmd_cfg |= FIELD_PREP(CMD_CFG_TIMEOUT_MASK,

I can't see how these are related to ffs()...

> This is fairly rare, but over time there are likely going to be
> others like them. I see three possible ways forward here:
> 
> a) fix them individually as we run into them, hoping for the best

I think they are valid warnings, so my instinct would be to fix them as
they appear. (e.g. "0 - 8" isn't something FIELD_PREP can do anything
about.)

-Kees

-- 
Kees Cook

Re: [PATCH] PCI: Test for bit underflow in pcie_set_readrq()

[Bjorn Helgaas]

On Thu, Sep 04, 2025 at 10:28:41PM -0700, Kees Cook 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")

What's your plan for merging cbc654d18d37?  I suppose it's intended
for v6.18?  If it will appear in v6.17, let me know so I can merge
this for it as well.

Maybe this should go in v6.17 regardless, to avoid a warning
regression between this patch and cbc654d18d37?

> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Closes: https://lore.kernel.org/linux-pci/CA+G9fYuysVr6qT8bjF6f08WLyCJRG7aXAeSd2F7=zTaHHd7L+Q@mail.gmail.com/
> 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);
>  
>  	if (bridge->no_inc_mrrs) {
>  		int max_mrrs = pcie_get_readrq(dev);
> -- 
> 2.34.1
> 

Re: [PATCH] PCI: Test for bit underflow in pcie_set_readrq()

[Kees Cook]

On Mon, Sep 08, 2025 at 03:53:49PM -0500, Bjorn Helgaas wrote:
> On Thu, Sep 04, 2025 at 10:28:41PM -0700, Kees Cook 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")
> 
> What's your plan for merging cbc654d18d37?  I suppose it's intended
> for v6.18?  If it will appear in v6.17, let me know so I can merge
> this for it as well.

I had it planned for v6.18.

> Maybe this should go in v6.17 regardless, to avoid a warning
> regression between this patch and cbc654d18d37?

Sure, or I could take it as part of the ffs series?

-Kees

-- 
Kees Cook

Re: [PATCH] PCI: Test for bit underflow in pcie_set_readrq()

[Bjorn Helgaas]

On Thu, Sep 04, 2025 at 10:28:41PM -0700, Kees Cook 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=zTaHHd7L+Q@mail.gmail.com/
> Signed-off-by: Kees Cook <kees@kernel.org>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Would be great if you included this as part of your series, thanks!

> ---
> 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);
>  
>  	if (bridge->no_inc_mrrs) {
>  		int max_mrrs = pcie_get_readrq(dev);
> -- 
> 2.34.1
> 

Re: [PATCH] PCI: Test for bit underflow in pcie_set_readrq()

[Kees Cook]

On Mon, Sep 08, 2025 at 04:51:42PM -0500, Bjorn Helgaas wrote:
> On Thu, Sep 04, 2025 at 10:28:41PM -0700, Kees Cook 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=zTaHHd7L+Q@mail.gmail.com/
> > Signed-off-by: Kees Cook <kees@kernel.org>
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> Would be great if you included this as part of your series, thanks!

Okay, thanks! :)

-Kees


-- 
Kees Cook