diff mbox series

[08/15] wifi: iwlwifi: mvm: Add NULL check before dereferencing the pointer

Message ID 20230612184434.cf7a5ce82fb0.Id3c05d13eeee6638f0930f750e93fb928d5c9dee@changeid (mailing list archive)
State New
Delegated to: Miri Korenblit
Headers show
Series wifi: iwlwifi: updates intended for v6.5 2023-06-12 | expand

Commit Message

Greenman, Gregory June 12, 2023, 3:51 p.m. UTC 
From: Mukesh Sisodiya <mukesh.sisodiya@intel.com>

The p2p, bss and ap vif pointers are assigned based on the mode.
All pointers will not have valid value at same time and can be
NULL, based on configured mode. This can lead to NULL pointer
access. Add NULL pointer check before accessing the data from
vif pointer.

Signed-off-by: Mukesh Sisodiya <mukesh.sisodiya@intel.com>
Signed-off-by: Gregory Greenman <gregory.greenman@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/power.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Johannes Berg June 14, 2023, 10:07 a.m. UTC | #1 
On Mon, 2023-06-12 at 18:51 +0300, gregory.greenman@intel.com wrote:
> From: Mukesh Sisodiya <mukesh.sisodiya@intel.com>
> 
> The p2p, bss and ap vif pointers are assigned based on the mode.
> All pointers will not have valid value at same time and can be
> NULL, based on configured mode. This can lead to NULL pointer
> access.

This is not true.

>  	/* enable PM on bss if bss stand alone */
> -	if (vifs->bss_active && !vifs->p2p_active && !vifs->ap_active) {
> +	if (bss_mvmvif && vifs->bss_active && !vifs->p2p_active &&
> +	    !vifs->ap_active) {
> 

The pointers can only be NULL iff *_active is false, however, it may be
false even if the pointer is non-NULL, so it's not exactly the same.

Probably a static checker thing that didn't understand it?

johannes
Greenman, Gregory June 14, 2023, 12:28 p.m. UTC | #2 
On Wed, 2023-06-14 at 12:07 +0200, Johannes Berg wrote:
> On Mon, 2023-06-12 at 18:51 +0300, gregory.greenman@intel.com wrote:
> > From: Mukesh Sisodiya <mukesh.sisodiya@intel.com>
> > 
> > The p2p, bss and ap vif pointers are assigned based on the mode.
> > All pointers will not have valid value at same time and can be
> > NULL, based on configured mode. This can lead to NULL pointer
> > access.
> 
> This is not true.
> 
> >         /* enable PM on bss if bss stand alone */
> > -       if (vifs->bss_active && !vifs->p2p_active && !vifs->ap_active) {
> > +       if (bss_mvmvif && vifs->bss_active && !vifs->p2p_active &&
> > +           !vifs->ap_active) {
> > 
> 
> The pointers can only be NULL iff *_active is false, however, it may be
> false even if the pointer is non-NULL, so it's not exactly the same.
> 
> Probably a static checker thing that didn't understand it?
> 
> johannes

Right, so the commit message could be rephrased like this:
"While vif pointers are protected by the corresponding "*active" fields,
static checkers can get confused sometimes. Add an explicit check."

Do you want me to resend it with the fixed commit message?
Johannes Berg June 14, 2023, 12:32 p.m. UTC | #3 
On Wed, 2023-06-14 at 12:28 +0000, Greenman, Gregory wrote:
> On Wed, 2023-06-14 at 12:07 +0200, Johannes Berg wrote:
> > On Mon, 2023-06-12 at 18:51 +0300, gregory.greenman@intel.com wrote:
> > > From: Mukesh Sisodiya <mukesh.sisodiya@intel.com>
> > > 
> > > The p2p, bss and ap vif pointers are assigned based on the mode.
> > > All pointers will not have valid value at same time and can be
> > > NULL, based on configured mode. This can lead to NULL pointer
> > > access.
> > 
> > This is not true.
> > 
> > >         /* enable PM on bss if bss stand alone */
> > > -       if (vifs->bss_active && !vifs->p2p_active && !vifs->ap_active) {
> > > +       if (bss_mvmvif && vifs->bss_active && !vifs->p2p_active &&
> > > +           !vifs->ap_active) {
> > > 
> > 
> > The pointers can only be NULL iff *_active is false, however, it may be
> > false even if the pointer is non-NULL, so it's not exactly the same.
> > 
> > Probably a static checker thing that didn't understand it?
> > 
> > johannes
> 
> Right, so the commit message could be rephrased like this:
> "While vif pointers are protected by the corresponding "*active" fields,
> static checkers can get confused sometimes. Add an explicit check."
> 
> Do you want me to resend it with the fixed commit message?

Yes please. I also delegated this and the other one to you in patchwork
again.

johannes
diff mbox series

Patch

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/power.c b/drivers/net/wireless/intel/iwlwifi/mvm/power.c
index ac1dae52556f..19839cc44eb3 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/power.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/power.c
@@ -647,30 +647,32 @@  static void iwl_mvm_power_set_pm(struct iwl_mvm *mvm,
 		return;
 
 	/* enable PM on bss if bss stand alone */
-	if (vifs->bss_active && !vifs->p2p_active && !vifs->ap_active) {
+	if (bss_mvmvif && vifs->bss_active && !vifs->p2p_active &&
+	    !vifs->ap_active) {
 		bss_mvmvif->pm_enabled = true;
 		return;
 	}
 
 	/* enable PM on p2p if p2p stand alone */
-	if (vifs->p2p_active && !vifs->bss_active && !vifs->ap_active) {
+	if (p2p_mvmvif && vifs->p2p_active && !vifs->bss_active &&
+	    !vifs->ap_active) {
 		p2p_mvmvif->pm_enabled = true;
 		return;
 	}
 
-	if (vifs->bss_active && vifs->p2p_active)
+	if (p2p_mvmvif && bss_mvmvif && vifs->bss_active && vifs->p2p_active)
 		client_same_channel =
 			iwl_mvm_have_links_same_channel(bss_mvmvif, p2p_mvmvif);
 
-	if (vifs->bss_active && vifs->ap_active)
+	if (bss_mvmvif && ap_mvmvif && vifs->bss_active && vifs->ap_active)
 		ap_same_channel =
 			iwl_mvm_have_links_same_channel(bss_mvmvif, ap_mvmvif);
 
 	/* clients are not stand alone: enable PM if DCM */
 	if (!(client_same_channel || ap_same_channel)) {
-		if (vifs->bss_active)
+		if (bss_mvmvif && vifs->bss_active)
 			bss_mvmvif->pm_enabled = true;
-		if (vifs->p2p_active)
+		if (p2p_mvmvif && vifs->p2p_active)
 			p2p_mvmvif->pm_enabled = true;
 		return;
 	}