wifi: iwlwifi: mld: Work around Clang loop unrolling bug

PATCH In review Kees Cook 2 participants 3 messages Apr 25, 2025 – May 20, 2025 Intel wireless wifi link (iwlwifi)

Share & tools

Kees Cook

author

2025-04-25 18:44 UTC #0 lore raw

PATCH v2 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. Additionally do not repeat the ns_config and targ_addrs array sizes with their open-coded names since they can be determined at compile-time with ARRAY_SIZE().

Reported-by Nathan Chancellor <[email protected]>

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 <[email protected]>

---
v2:
- move "j < n_nsc" forward to stabilize loop bounds (Nathan)
- use ARRAY_SIZE() for robustness
v1: https://lore.kernel.org/all/[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: <[email protected]>

---
 drivers/net/wireless/intel/iwlwifi/mld/d3.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

drivers/net/wireless/intel/iwlwifi/mld/d3.c +5 -9

index dc736fdc176d..c51a6596617d 100644
@@ -1728,17 +1728,13 @@ iwl_mld_send_proto_offload(struct iwl_mld *mld,
 #if IS_ENABLED(CONFIG_IPV6)
 	struct iwl_mld_vif *mld_vif = iwl_mld_vif_from_mac80211(vif);
 	struct iwl_mld_wowlan_data *wowlan_data = &mld_vif->wowlan_data;
-	struct iwl_ns_config *nsc;
-	struct iwl_targ_addr *addrs;
-	int n_nsc, n_addrs;
+	const int n_addrs = ARRAY_SIZE(cmd->targ_addrs);
+	struct iwl_targ_addr *addrs = cmd->targ_addrs;
+	const int n_nsc = ARRAY_SIZE(cmd->ns_config);
+	struct iwl_ns_config *nsc = cmd->ns_config;
 	int i, c;
 	int num_skipped = 0;
 
-	nsc = cmd->ns_config;
-	n_nsc = IWL_PROTO_OFFLOAD_NUM_NS_CONFIG_V3L;
-	addrs = cmd->targ_addrs;
-	n_addrs = IWL_PROTO_OFFLOAD_NUM_IPV6_ADDRS_V3L;
-
 	/* For each address we have (and that will fit) fill a target
 	 * address struct and combine for NS offload structs with the
 	 * solicited node addresses.

@@ -1759,7 +1755,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 < n_nsc && j < c; j++)
 			if (ipv6_addr_cmp(&nsc[j].dest_ipv6_addr,
 					  &solicited_addr) == 0)
 				break;

Signature · 1 line

2.34.1

Nathan Chancellor

Reviewed-by on v2 1/1

2025-04-26 00:04 UTC #1 lore raw

Quoted 23 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 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. Additionally do not repeat the ns_config and targ_addrs array sizes with their open-coded names since they can be determined at compile-time with ARRAY_SIZE().

Quoted 5 lines

Reported-by Nathan Chancellor <[email protected]>
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 <[email protected]>

Reviewed-by Nathan Chancellor <[email protected]>

Quoted 53 lines

---
 drivers/net/wireless/intel/iwlwifi/mld/d3.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

drivers/net/wireless/intel/iwlwifi/mld/d3.c +5 -9

index dc736fdc176d..c51a6596617d 100644
@@ -1728,17 +1728,13 @@ iwl_mld_send_proto_offload(struct iwl_mld *mld,
 #if IS_ENABLED(CONFIG_IPV6)
 	struct iwl_mld_vif *mld_vif = iwl_mld_vif_from_mac80211(vif);
 	struct iwl_mld_wowlan_data *wowlan_data = &mld_vif->wowlan_data;
-	struct iwl_ns_config *nsc;
-	struct iwl_targ_addr *addrs;
-	int n_nsc, n_addrs;
+	const int n_addrs = ARRAY_SIZE(cmd->targ_addrs);
+	struct iwl_targ_addr *addrs = cmd->targ_addrs;
+	const int n_nsc = ARRAY_SIZE(cmd->ns_config);
+	struct iwl_ns_config *nsc = cmd->ns_config;
 	int i, c;
 	int num_skipped = 0;
 
-	nsc = cmd->ns_config;
-	n_nsc = IWL_PROTO_OFFLOAD_NUM_NS_CONFIG_V3L;
-	addrs = cmd->targ_addrs;
-	n_addrs = IWL_PROTO_OFFLOAD_NUM_IPV6_ADDRS_V3L;
-
 	/* For each address we have (and that will fit) fill a target
 	 * address struct and combine for NS offload structs with the
 	 * solicited node addresses.

@@ -1759,7 +1755,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 < n_nsc && j < c; j++)
 			if (ipv6_addr_cmp(&nsc[j].dest_ipv6_addr,
 					  &solicited_addr) == 0)
 				break;

Signature · 1 line

2.34.1

Kees Cook

author

2025-05-20 17:23 UTC #2 lore raw

In reply to Kees Cook

Quoted 23 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 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. Additionally do not repeat the ns_config and targ_addrs array sizes with their open-coded names since they can be determined at compile-time with ARRAY_SIZE().

Quoted 5 lines

Reported-by Nathan Chancellor <[email protected]>
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 <[email protected]>

Ping on this -- can someone pick this up? I can carry it if needed?

-Kees

Quoted 53 lines

---
 drivers/net/wireless/intel/iwlwifi/mld/d3.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

drivers/net/wireless/intel/iwlwifi/mld/d3.c +5 -9

index dc736fdc176d..c51a6596617d 100644
@@ -1728,17 +1728,13 @@ iwl_mld_send_proto_offload(struct iwl_mld *mld,
 #if IS_ENABLED(CONFIG_IPV6)
 	struct iwl_mld_vif *mld_vif = iwl_mld_vif_from_mac80211(vif);
 	struct iwl_mld_wowlan_data *wowlan_data = &mld_vif->wowlan_data;
-	struct iwl_ns_config *nsc;
-	struct iwl_targ_addr *addrs;
-	int n_nsc, n_addrs;
+	const int n_addrs = ARRAY_SIZE(cmd->targ_addrs);
+	struct iwl_targ_addr *addrs = cmd->targ_addrs;
+	const int n_nsc = ARRAY_SIZE(cmd->ns_config);
+	struct iwl_ns_config *nsc = cmd->ns_config;
 	int i, c;
 	int num_skipped = 0;
 
-	nsc = cmd->ns_config;
-	n_nsc = IWL_PROTO_OFFLOAD_NUM_NS_CONFIG_V3L;
-	addrs = cmd->targ_addrs;
-	n_addrs = IWL_PROTO_OFFLOAD_NUM_IPV6_ADDRS_V3L;
-
 	/* For each address we have (and that will fit) fill a target
 	 * address struct and combine for NS offload structs with the
 	 * solicited node addresses.

@@ -1759,7 +1755,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 < n_nsc && j < c; j++)
 			if (ipv6_addr_cmp(&nsc[j].dest_ipv6_addr,
 					  &solicited_addr) == 0)
 				break;

Signature · 1 line

2.34.1

Signature · 1 line

Kees Cook