[PATCH] drivers:wireless: Fix a dereference before null check issue

[PATCH] drivers:wireless: Fix a dereference before null check issue

[Paolo Perego]

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.

Signed-off-by: Paolo Perego <pperego@suse.de>
---
 drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c
index f85c01e04ebf..f733c16ffd8e 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c
@@ -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

Re: [PATCH] drivers:wireless: Fix a dereference before null check issue

[Johannes Berg]

On Thu, 2024-11-21 at 18:02 +0100, Paolo Perego wrote:
> This patch fixes a dereference before null check issue discovered by 
> Coverity (CID 1601547)
> 

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

Re: [PATCH] drivers:wireless: Fix a dereference before null check issue

[Paolo Perego]

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

On Thu, Nov 21, 2024 at 06:28:14PM GMT, Johannes Berg wrote:
> On Thu, 2024-11-21 at 18:02 +0100, Paolo Perego wrote:
> > This patch fixes a dereference before null check issue discovered by 
> > Coverity (CID 1601547)
> > 
> 
> 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 :)
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drivers:wireless: Fix a dereference before null check issue

[Johannes Berg]

On Thu, 2024-11-21 at 18:35 +0100, Paolo Perego wrote:
> On Thu, Nov 21, 2024 at 06:28:14PM GMT, Johannes Berg wrote:
> > On Thu, 2024-11-21 at 18:02 +0100, Paolo Perego wrote:
> > > This patch fixes a dereference before null check issue discovered by 
> > > Coverity (CID 1601547)
> > > 
> > 
> > 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 :)
> Oops, I'm so sorry this was already fixed. In Coverity dashboard the
> item seemed to be still open.

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.

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

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

Re: wireless: Fix a dereference before null check issue

[Markus Elfring]

> 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. …

To which messages would you like to refer here?

Regards,
Markus

Re: wireless: Fix a dereference before null check issue

[Markus Elfring]

> Oops, I'm so sorry this was already fixed.

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


>                                            In Coverity dashboard the
> item seemed to be still open.

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


> Apart from that, did I followed the right steps?

Partly, yes.


>                                                  Was my submission good
> enough? (I'm new to kernel hacking and I'm still in the learning phase)

I find details improvable.

1. Change description

2. Patch subject


Regards,
Markus