linux-kernel - Re: [PATCH] PCI: Test for bit underflow in pcie_set

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-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <d430f9ac-153c-41da-9cbe-53aa2f9d0fc3@app.fastmail.com>
Date: Fri, 05 Sep 2025 10:25:49 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Kees Cook" <kees@...nel.org>, "Bjorn Helgaas" <bhelgaas@...gle.com>
Cc: "Linaro Kernel Functional Testing" <lkft@...aro.org>,
 "Anders Roxell" <anders.roxell@...aro.org>,
 "Naresh Kamboju" <naresh.kamboju@...aro.org>, lkft-triage@...ts.linaro.org,
 "Linux Regressions" <regressions@...ts.linux.dev>,
 "Dan Carpenter" <dan.carpenter@...aro.org>,
 "Benjamin Copeland" <benjamin.copeland@...aro.org>,
 linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
 "Peter Zijlstra" <peterz@...radead.org>, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] PCI: Test for bit underflow in pcie_set_readrq()

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@...aro.org>
> Closes: 
> https://lore.kernel.org/linux-pci/CA+G9fYuysVr6qT8bjF6f08WLyCJRG7aXAeSd2F7=zTaHHd7L+Q@mail.gmail.com/
> Signed-off-by: Kees Cook <kees@...nel.org>

Acked-by: Arnd Bergmann <arnd@...db.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