wifi: iwlwifi: mld: Work around Clang loop unrolling bug
PATCH 1/1 wifi: iwlwifi: mld: Work around Clang loop unrolling bug
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.
---
Cc: Miri Korenblit <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: Yedidya Benshimol <[email protected]>
Cc: Emmanuel Grumbach <[email protected]>
Cc: Avraham Stern <[email protected]>
Cc: Daniel Gabay <[email protected]>
Cc: <[email protected]>
--- drivers/net/wireless/intel/iwlwifi/mld/d3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/wireless/intel/iwlwifi/mld/d3.c +1 -1
@@ -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 ;
Signature · 1 line
2.34.1
Quoted 54 lines · Kees Cook
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 inmemcmp()(viaipv6_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 <[email protected]>Signed-off-by Kees Cook <[email protected]>---
Cc: Miri Korenblit <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: Yedidya Benshimol <[email protected]>
Cc: Emmanuel Grumbach <[email protected]>
Cc: Avraham Stern <[email protected]>
Cc: Daniel Gabay <[email protected]>
Cc: <[email protected]>--- drivers/net/wireless/intel/iwlwifi/mld/d3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/net/wireless/intel/iwlwifi/mld/d3.c +1
-1@@ -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 ;Signature · 1 line
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 [email protected] ...
$ 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
drivers/net/wireless/intel/iwlwifi/mld/d3.c +1 -1
@@ -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 ;
Quoted 59 lines · Nathan Chancellor
Quoted 54 lines · Kees Cook
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 inmemcmp()(viaipv6_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 <[email protected]>Signed-off-by Kees Cook <[email protected]>---
Cc: Miri Korenblit <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: Yedidya Benshimol <[email protected]>
Cc: Emmanuel Grumbach <[email protected]>
Cc: Avraham Stern <[email protected]>
Cc: Daniel Gabay <[email protected]>
Cc: <[email protected]>--- drivers/net/wireless/intel/iwlwifi/mld/d3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/net/wireless/intel/iwlwifi/mld/d3.c +1
-1@@ -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 ;Signature · 1 line
2.34.1I might be going crazy but this does not appear to actually resolve the warning for me, at least with allmodconfig:
Quoted 2 lines
$ 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"drivers/net/wireless/intel/iwlwifi/mld/d3.c iwl_mld_send_proto_offload(struct iwl_mld *mld, · line 1763
Quoted 13 lines
drivers/net/wireless/intel/iwlwifi/mld/d3.c +1
-1@@ -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
Signature · 1 line
Kees Cook
Quoted 9 lines · Kees Cook
Quoted 2 lines · Nathan Chancellor
$ 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-patchto 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")Quoted 6 lines
Quoted 6 lines
drivers/net/wireless/intel/iwlwifi/mld/d3.c +0
-0@@ -1757,7 +1757,7 @@ iwl_mld_send_proto_offload(struct iwl_mld *mld,
Quoted 10 lines
drivers/net/wireless/intel/iwlwifi/mld/d3.c iwl_mld_send_proto_offload(struct iwl_mld *mld, · line 1763
Quoted 7 lines
drivers/net/wireless/intel/iwlwifi/mld/d3.c +1
-1addrconf_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