| Message ID | 20241121170257.275607-1-pperego@suse.de (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | drivers:wireless: Fix a dereference before null check issue | expand |
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
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
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
> 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
> 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
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;
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(-)