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@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > ---
> > Cc: Miri Korenblit <miriam.rachel.korenblit@xxxxxxxxx>
> > Cc: Johannes Berg <johannes.berg@xxxxxxxxx>
> > Cc: Yedidya Benshimol <yedidya.ben.shimol@xxxxxxxxx>
> > Cc: Emmanuel Grumbach <emmanuel.grumbach@xxxxxxxxx>
> > Cc: Avraham Stern <avraham.stern@xxxxxxxxx>
> > Cc: Daniel Gabay <daniel.gabay@xxxxxxxxx>
> > Cc: <linux-wireless@xxxxxxxxxxxxxxx>
> > ---
> >  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