| Message ID | 20250421204153.work.935-kees@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: iwlwifi: mld: Work around Clang loop unrolling bug | expand |
On Mon, Apr 21, 2025 at 01:41:57PM -0700, Kees Cook wrote: > The nested loop in iwl_mld_send_proto_offload() confuses Clang into > thinking there could be final loop iteration past the end of the "nsc" > array (which is only 4 entries). The FORTIFY checking in memcmp() > (via ipv6_addr_cmp()) notices this (due to the available bytes in the > out-of-bounds position of &nsc[4] being 0), and errors out, failing > the build. For some reason (likely due to architectural loop unrolling > configurations), this is only exposed on ARM builds currently. Due to > Clang's lack of inline tracking[1], the warning is not very helpful: > > include/linux/fortify-string.h:719:4: error: call to '__read_overflow' declared with 'error' attribute: detected read beyond size of object (1st parameter) > 719 | __read_overflow(); > | ^ > 1 error generated. > > But this was tracked down to iwl_mld_send_proto_offload()'s > ipv6_addr_cmp() call. > > An upstream Clang bug has been filed[2] to track this, but for now. > Fix the build by explicitly bounding the inner loop by "n_nsc", which > is what "c" is already limited to. > > Reported-by: Nathan Chancellor <nathan@kernel.org> > Closes: https://github.com/ClangBuiltLinux/linux/issues/2076 > Link: https://github.com/llvm/llvm-project/pull/73552 [1] > Link: https://github.com/llvm/llvm-project/issues/136603 [2] > Signed-off-by: Kees Cook <kees@kernel.org> > --- > Cc: Miri Korenblit <miriam.rachel.korenblit@intel.com> > Cc: Johannes Berg <johannes.berg@intel.com> > Cc: Yedidya Benshimol <yedidya.ben.shimol@intel.com> > Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > Cc: Avraham Stern <avraham.stern@intel.com> > Cc: Daniel Gabay <daniel.gabay@intel.com> > Cc: <linux-wireless@vger.kernel.org> > --- > drivers/net/wireless/intel/iwlwifi/mld/d3.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/mld/d3.c b/drivers/net/wireless/intel/iwlwifi/mld/d3.c > index 2c6e8ecd93b7..1daca1ef02b2 100644 > --- a/drivers/net/wireless/intel/iwlwifi/mld/d3.c > +++ b/drivers/net/wireless/intel/iwlwifi/mld/d3.c > @@ -1754,7 +1754,7 @@ iwl_mld_send_proto_offload(struct iwl_mld *mld, > > addrconf_addr_solict_mult(&wowlan_data->target_ipv6_addrs[i], > &solicited_addr); > - for (j = 0; j < c; j++) > + for (j = 0; j < c && j < n_nsc; j++) > if (ipv6_addr_cmp(&nsc[j].dest_ipv6_addr, > &solicited_addr) == 0) > break; > -- > 2.34.1 > I might be going crazy but this does not appear to actually resolve the warning for me, at least with allmodconfig: $ git cite a33b5a08cbbd ("Merge tag 'sched_ext-for-6.15-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext") $ make -skj"$(nproc)" ARCH=arm LLVM=1 clean allmodconfig drivers/net/wireless/intel/iwlwifi/mld/d3.o In file included from drivers/net/wireless/intel/iwlwifi/mld/d3.c:5: ... In file included from include/linux/string.h:392: include/linux/fortify-string.h:719:4: error: call to '__read_overflow' declared with 'error' attribute: detected read beyond size of object (1st parameter) 719 | __read_overflow(); | ^ 1 error generated. $ b4 -q shazam 20250421204153.work.935-kees@kernel.org ... $ make -skj"$(nproc)" ARCH=arm LLVM=1 clean allmodconfig drivers/net/wireless/intel/iwlwifi/mld/d3.o In file included from drivers/net/wireless/intel/iwlwifi/mld/d3.c:5: ... In file included from include/linux/string.h:392: include/linux/fortify-string.h:719:4: error: call to '__read_overflow' declared with 'error' attribute: detected read beyond size of object (1st parameter) 719 | __read_overflow(); | ^ 1 error generated. However, if I apply the diff at the bottom of the message, it does... $ git stash pop $ make -skj"$(nproc)" ARCH=arm LLVM=1 clean allmodconfig drivers/net/wireless/intel/iwlwifi/mld/d3.o It is possible there is something else going on with allmodconfig like KASAN or GCOV that causes this but I did not look too closely yet Cheers, Nathan diff --git a/drivers/net/wireless/intel/iwlwifi/mld/d3.c b/drivers/net/wireless/intel/iwlwifi/mld/d3.c index fba6a7b1bb5c..7ce01ad3608e 100644 --- a/drivers/net/wireless/intel/iwlwifi/mld/d3.c +++ b/drivers/net/wireless/intel/iwlwifi/mld/d3.c @@ -1757,7 +1757,7 @@ iwl_mld_send_proto_offload(struct iwl_mld *mld, addrconf_addr_solict_mult(&wowlan_data->target_ipv6_addrs[i], &solicited_addr); - for (j = 0; j < c && j < n_nsc; j++) + for (j = 0; j < n_nsc && j < c; j++) if (ipv6_addr_cmp(&nsc[j].dest_ipv6_addr, &solicited_addr) == 0) break;
On Tue, Apr 22, 2025 at 12:59:03PM -0700, Nathan Chancellor wrote: > On Mon, Apr 21, 2025 at 01:41:57PM -0700, Kees Cook wrote: > > The nested loop in iwl_mld_send_proto_offload() confuses Clang into > > thinking there could be final loop iteration past the end of the "nsc" > > array (which is only 4 entries). The FORTIFY checking in memcmp() > > (via ipv6_addr_cmp()) notices this (due to the available bytes in the > > out-of-bounds position of &nsc[4] being 0), and errors out, failing > > the build. For some reason (likely due to architectural loop unrolling > > configurations), this is only exposed on ARM builds currently. Due to > > Clang's lack of inline tracking[1], the warning is not very helpful: > > > > include/linux/fortify-string.h:719:4: error: call to '__read_overflow' declared with 'error' attribute: detected read beyond size of object (1st parameter) > > 719 | __read_overflow(); > > | ^ > > 1 error generated. > > > > But this was tracked down to iwl_mld_send_proto_offload()'s > > ipv6_addr_cmp() call. > > > > An upstream Clang bug has been filed[2] to track this, but for now. > > Fix the build by explicitly bounding the inner loop by "n_nsc", which > > is what "c" is already limited to. > > > > Reported-by: Nathan Chancellor <nathan@kernel.org> > > Closes: https://github.com/ClangBuiltLinux/linux/issues/2076 > > Link: https://github.com/llvm/llvm-project/pull/73552 [1] > > Link: https://github.com/llvm/llvm-project/issues/136603 [2] > > Signed-off-by: Kees Cook <kees@kernel.org> > > --- > > Cc: Miri Korenblit <miriam.rachel.korenblit@intel.com> > > Cc: Johannes Berg <johannes.berg@intel.com> > > Cc: Yedidya Benshimol <yedidya.ben.shimol@intel.com> > > Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > > Cc: Avraham Stern <avraham.stern@intel.com> > > Cc: Daniel Gabay <daniel.gabay@intel.com> > > Cc: <linux-wireless@vger.kernel.org> > > --- > > drivers/net/wireless/intel/iwlwifi/mld/d3.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/wireless/intel/iwlwifi/mld/d3.c b/drivers/net/wireless/intel/iwlwifi/mld/d3.c > > index 2c6e8ecd93b7..1daca1ef02b2 100644 > > --- a/drivers/net/wireless/intel/iwlwifi/mld/d3.c > > +++ b/drivers/net/wireless/intel/iwlwifi/mld/d3.c > > @@ -1754,7 +1754,7 @@ iwl_mld_send_proto_offload(struct iwl_mld *mld, > > > > addrconf_addr_solict_mult(&wowlan_data->target_ipv6_addrs[i], > > &solicited_addr); > > - for (j = 0; j < c; j++) > > + for (j = 0; j < c && j < n_nsc; j++) > > if (ipv6_addr_cmp(&nsc[j].dest_ipv6_addr, > > &solicited_addr) == 0) > > break; > > -- > > 2.34.1 > > > > I might be going crazy but this does not appear to actually resolve the > warning for me, at least with allmodconfig: > > $ git cite > a33b5a08cbbd ("Merge tag 'sched_ext-for-6.15-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext") Make me look. :) "cite" is a local alias, yes? Looks like my own alias for "short", but basically "short HEAD". From my ~/.gitconfig: [alias] short = "!f() { for i in \"$@\"; do git log -1 --pretty='%h (\"%s\")' \"$i\"; done; }; f" > diff --git a/drivers/net/wireless/intel/iwlwifi/mld/d3.c b/drivers/net/wireless/intel/iwlwifi/mld/d3.c > index fba6a7b1bb5c..7ce01ad3608e 100644 > --- a/drivers/net/wireless/intel/iwlwifi/mld/d3.c > +++ b/drivers/net/wireless/intel/iwlwifi/mld/d3.c > @@ -1757,7 +1757,7 @@ iwl_mld_send_proto_offload(struct iwl_mld *mld, > > addrconf_addr_solict_mult(&wowlan_data->target_ipv6_addrs[i], > &solicited_addr); > - for (j = 0; j < c && j < n_nsc; j++) > + for (j = 0; j < n_nsc && j < c; j++) > if (ipv6_addr_cmp(&nsc[j].dest_ipv6_addr, > &solicited_addr) == 0) > break; Oof, an unstable solution. Well, I guess we work with what we've got. Your change also solves it for me, so I'll send a v2 with it that way. On the "getting it fixed correctly" front, we need someone who can tweak SCEV: https://github.com/llvm/llvm-project/issues/136603 -Kees
On Fri, Apr 25, 2025 at 11:18:33AM -0700, Kees Cook wrote: > On Tue, Apr 22, 2025 at 12:59:03PM -0700, Nathan Chancellor wrote: > > $ git cite > > a33b5a08cbbd ("Merge tag 'sched_ext-for-6.15-rc3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/sched_ext") > > Make me look. :) "cite" is a local alias, yes? Looks like my own alias > for "short", but basically "short HEAD". From my ~/.gitconfig: > > [alias] > short = "!f() { for i in \"$@\"; do git log -1 --pretty='%h (\"%s\")' \"$i\"; done; }; f" Heh, yes, that was me being lazy :) 'cite' ultimately expands to show --format='%h ("%s")' --no-patch to basically do what yours does. $ git cite HEAD~2 HEAD^ HEAD e72e9e693307 ("Merge tag 'net-6.15-rc4' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net") 30e268185e59 ("Merge tag 'landlock-6.15-rc4' of git://git.kernel.org/pub/scm/linux/kernel/git/mic/linux") 02ddfb981de8 ("Merge tag 'scsi-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi") > > diff --git a/drivers/net/wireless/intel/iwlwifi/mld/d3.c b/drivers/net/wireless/intel/iwlwifi/mld/d3.c > > index fba6a7b1bb5c..7ce01ad3608e 100644 > > --- a/drivers/net/wireless/intel/iwlwifi/mld/d3.c > > +++ b/drivers/net/wireless/intel/iwlwifi/mld/d3.c > > @@ -1757,7 +1757,7 @@ iwl_mld_send_proto_offload(struct iwl_mld *mld, > > > > addrconf_addr_solict_mult(&wowlan_data->target_ipv6_addrs[i], > > &solicited_addr); > > - for (j = 0; j < c && j < n_nsc; j++) > > + for (j = 0; j < n_nsc && j < c; j++) > > if (ipv6_addr_cmp(&nsc[j].dest_ipv6_addr, > > &solicited_addr) == 0) > > break; > > Oof, an unstable solution. Well, I guess we work with what we've got. > Your change also solves it for me, so I'll send a v2 with it that way. Indeed... I will review v2 shortly but another option would be stick a #include <linux/unroll.h> #ifdef CONFIG_CC_IS_CLANG unrolled_none #endif above the loop to just avoid tripping the optimizer up altogether but I understand that is just as unstable as this one (even if it is one of the few ways that the compiler gives us to turn off optimizations). Cheers, Nathan
diff --git a/drivers/net/wireless/intel/iwlwifi/mld/d3.c b/drivers/net/wireless/intel/iwlwifi/mld/d3.c index 2c6e8ecd93b7..1daca1ef02b2 100644 --- a/drivers/net/wireless/intel/iwlwifi/mld/d3.c +++ b/drivers/net/wireless/intel/iwlwifi/mld/d3.c @@ -1754,7 +1754,7 @@ iwl_mld_send_proto_offload(struct iwl_mld *mld, addrconf_addr_solict_mult(&wowlan_data->target_ipv6_addrs[i], &solicited_addr); - for (j = 0; j < c; j++) + for (j = 0; j < c && j < n_nsc; j++) if (ipv6_addr_cmp(&nsc[j].dest_ipv6_addr, &solicited_addr) == 0) break;
The nested loop in iwl_mld_send_proto_offload() confuses Clang into thinking there could be final loop iteration past the end of the "nsc" array (which is only 4 entries). The FORTIFY checking in memcmp() (via ipv6_addr_cmp()) notices this (due to the available bytes in the out-of-bounds position of &nsc[4] being 0), and errors out, failing the build. For some reason (likely due to architectural loop unrolling configurations), this is only exposed on ARM builds currently. Due to Clang's lack of inline tracking[1], the warning is not very helpful: include/linux/fortify-string.h:719:4: error: call to '__read_overflow' declared with 'error' attribute: detected read beyond size of object (1st parameter) 719 | __read_overflow(); | ^ 1 error generated. But this was tracked down to iwl_mld_send_proto_offload()'s ipv6_addr_cmp() call. An upstream Clang bug has been filed[2] to track this, but for now. Fix the build by explicitly bounding the inner loop by "n_nsc", which is what "c" is already limited to. Reported-by: Nathan Chancellor <nathan@kernel.org> Closes: https://github.com/ClangBuiltLinux/linux/issues/2076 Link: https://github.com/llvm/llvm-project/pull/73552 [1] Link: https://github.com/llvm/llvm-project/issues/136603 [2] Signed-off-by: Kees Cook <kees@kernel.org> --- Cc: Miri Korenblit <miriam.rachel.korenblit@intel.com> Cc: Johannes Berg <johannes.berg@intel.com> Cc: Yedidya Benshimol <yedidya.ben.shimol@intel.com> Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com> Cc: Avraham Stern <avraham.stern@intel.com> Cc: Daniel Gabay <daniel.gabay@intel.com> Cc: <linux-wireless@vger.kernel.org> --- drivers/net/wireless/intel/iwlwifi/mld/d3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)