[PATCH] wifi: iwlwifi: mld: Work around Clang loop unrolling bug

[PATCH] wifi: iwlwifi: mld: Work around Clang loop unrolling bug

[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 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

Re: [PATCH] wifi: iwlwifi: mld: Work around Clang loop unrolling bug

[Nathan Chancellor]

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;

Re: [PATCH] wifi: iwlwifi: mld: Work around Clang loop unrolling bug

[Kees Cook]

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

-- 
Kees Cook

Re: [PATCH] wifi: iwlwifi: mld: Work around Clang loop unrolling bug

[Nathan Chancellor]

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