drivers:wireless: Fix a dereference before null check issue

This patch fixes a dereference before null check issue discovered by Coverity (CID 1601547)

In iwl_mvm_parse_wowlan_info_notif() routine data is checked against NULL value at line 2501 but it has been dereferenced three lines before when calculating sizeof() in an assignment.

---
 drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index f85c01e04ebf..f733c16ffd8e 100644
@@ -2495,8 +2495,7 @@ static void iwl_mvm_parse_wowlan_info_notif(struct iwl_mvm *mvm,
 					    struct iwl_wowlan_status_data *status,
 					    u32 len)
 {
-	u32 expected_len = sizeof(*data) +
-		data->num_mlo_link_keys * sizeof(status->mlo_keys[0]);
+	u32 expected_len = 0;
 
 	if (!data) {
 		IWL_ERR(mvm, "iwl_wowlan_info_notif data is NULL\n");

@@ -2504,6 +2503,8 @@ static void iwl_mvm_parse_wowlan_info_notif(struct iwl_mvm *mvm,
 		return;
 	}
 
+	expected_len = sizeof(*data) + data->num_mlo_link_keys * sizeof(status->mlo_keys[0]);
+
 	if (len < expected_len) {
 		IWL_ERR(mvm, "Invalid WoWLAN info notification!\n");
 		status = NULL;

2.47.0

This was reported before by smatch too, and Emmanuel just made a patch to simply remove the NULL checks, because the pointers are statically known to be not NULL. So it's not really an issue other than style/checkers/... anyway :)

johannes

Oops, I'm so sorry this was already fixed. In Coverity dashboard the item seemed to be still open.

Apart from that, did I followed the right steps? Was my submission good enough? (I'm new to kernel hacking and I'm still in the learning phase)

Thanks
Paolo

(*_  Paolo Perego                           @thesp0nge
//\  Software security engineer               suse.com
V_/_ 0A1A 2003 9AE0 B09C 51A4 7ACD FC0D CEA6 0806 294B

Oh it wasn't fixed yet, the patch isn't anywhere near the trees. But it's also not very important, so I doubt we'll handle it urgently.

Well, should've had the right subject prefix, as "wifi: iwlwifi:" but other than that, I guess?

Arguably, you also shouldn't have had the = 0 in the code, since it got unconditionally assigned anyway.

And, if you're going to continue looking at Coverity reports, I'd suggest to dig a bit deeper. We're not here to fix reports from Coverity after all, we should fix _bugs_, and tools will get things wrong :)

johannes

It can occasionally happen that some contributors would like to adjust the same source code places somehow.

It might occasionally be unclear with which delay corresponding items will be synchronised.

Partly, yes.

I find details improvable.

1. Change description

2. Patch subject

Regards,
Markus

To which messages would you like to refer here?

Regards,
Markus