[PATCH next] wifi: iwlwifi: Fix uninitialized variable with __free()

[PATCH next] wifi: iwlwifi: Fix uninitialized variable with __free()

[Dan Carpenter]

Pointers declared with the __free(kfree) attribute need to be initialized
because they will be passed to kfree() on every return path.  There are
two return statement before the "cmd" pointer is initialized so this
leads to an uninitialized variable bug.

Fixes: d1e879ec600f ("wifi: iwlwifi: add iwlmld sub-driver")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/net/wireless/intel/iwlwifi/mld/debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
index c759c5c68dc0..1d4b2ad5d388 100644
--- a/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
+++ b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
@@ -556,8 +556,8 @@ iwl_dbgfs_vif_twt_setup_write(struct iwl_mld *mld, char *buf, size_t count,
 	};
 	struct ieee80211_vif *vif = data;
 	struct iwl_mld_vif *mld_vif = iwl_mld_vif_from_mac80211(vif);
+	struct iwl_dhc_cmd *cmd __free(kfree) = NULL;
 	struct iwl_dhc_twt_operation *dhc_twt_cmd;
-	struct iwl_dhc_cmd *cmd __free(kfree);
 	u64 target_wake_time;
 	u32 twt_operation, interval_exp, interval_mantissa, min_wake_duration;
 	u8 trigger, flow_type, flow_id, protection, tenth_param;
-- 
2.47.2

RE: [PATCH next] wifi: iwlwifi: Fix uninitialized variable with __free()

[Korenblit, Miriam Rachel]

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@linaro.org>
> Sent: Wednesday, 12 March 2025 10:32
> To: Korenblit, Miriam Rachel <miriam.rachel.korenblit@intel.com>
> Cc: Berg, Johannes <johannes.berg@intel.com>; Anjaneyulu, Pagadala Yesu
> <pagadala.yesu.anjaneyulu@intel.com>; Grumbach, Emmanuel
> <emmanuel.grumbach@intel.com>; Stern, Avraham
> <avraham.stern@intel.com>; Ben Shimol, Yedidya
> <yedidya.ben.shimol@intel.com>; Gabay, Daniel <daniel.gabay@intel.com>;
> linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-
> janitors@vger.kernel.org
> Subject: [PATCH next] wifi: iwlwifi: Fix uninitialized variable with __free()
> 
> Pointers declared with the __free(kfree) attribute need to be initialized because
> they will be passed to kfree() on every return path.  There are two return
> statement before the "cmd" pointer is initialized so this leads to an uninitialized
> variable bug.
> 
> Fixes: d1e879ec600f ("wifi: iwlwifi: add iwlmld sub-driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/net/wireless/intel/iwlwifi/mld/debugfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> index c759c5c68dc0..1d4b2ad5d388 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> @@ -556,8 +556,8 @@ iwl_dbgfs_vif_twt_setup_write(struct iwl_mld *mld, char
> *buf, size_t count,
>  	};
>  	struct ieee80211_vif *vif = data;
>  	struct iwl_mld_vif *mld_vif = iwl_mld_vif_from_mac80211(vif);
> +	struct iwl_dhc_cmd *cmd __free(kfree) = NULL;
>  	struct iwl_dhc_twt_operation *dhc_twt_cmd;
> -	struct iwl_dhc_cmd *cmd __free(kfree);
>  	u64 target_wake_time;
>  	u32 twt_operation, interval_exp, interval_mantissa, min_wake_duration;
>  	u8 trigger, flow_type, flow_id, protection, tenth_param;
> --
> 2.47.2
Acked-by: Miri Korenblit <miriam.rachel.korenblit@intel.com>

Re: [PATCH next] wifi: iwlwifi: Fix uninitialized variable with __free()

[Jeff Johnson]

On 3/12/2025 1:31 AM, Dan Carpenter wrote:
> Pointers declared with the __free(kfree) attribute need to be initialized
> because they will be passed to kfree() on every return path.  There are
> two return statement before the "cmd" pointer is initialized so this
> leads to an uninitialized variable bug.
> 
> Fixes: d1e879ec600f ("wifi: iwlwifi: add iwlmld sub-driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/net/wireless/intel/iwlwifi/mld/debugfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> index c759c5c68dc0..1d4b2ad5d388 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> @@ -556,8 +556,8 @@ iwl_dbgfs_vif_twt_setup_write(struct iwl_mld *mld, char *buf, size_t count,
>  	};
>  	struct ieee80211_vif *vif = data;
>  	struct iwl_mld_vif *mld_vif = iwl_mld_vif_from_mac80211(vif);
> +	struct iwl_dhc_cmd *cmd __free(kfree) = NULL;

hmm, I thought the recommended convention was to define __free() pointers at
the point of allocation. cleanup.h explicitly says:

 * Given that the "__free(...) = NULL" pattern for variables defined at
 * the top of the function poses this potential interdependency problem
 * the recommendation is to always define and assign variables in one
 * statement and not group variable definitions at the top of the
 * function when __free() is used.

>  	struct iwl_dhc_twt_operation *dhc_twt_cmd;
> -	struct iwl_dhc_cmd *cmd __free(kfree);
>  	u64 target_wake_time;
>  	u32 twt_operation, interval_exp, interval_mantissa, min_wake_duration;
>  	u8 trigger, flow_type, flow_id, protection, tenth_param;

Re: [PATCH next] wifi: iwlwifi: Fix uninitialized variable with __free()

[Dan Carpenter]

On Wed, Mar 12, 2025 at 08:15:18AM -0700, Jeff Johnson wrote:
> On 3/12/2025 1:31 AM, Dan Carpenter wrote:
> > Pointers declared with the __free(kfree) attribute need to be initialized
> > because they will be passed to kfree() on every return path.  There are
> > two return statement before the "cmd" pointer is initialized so this
> > leads to an uninitialized variable bug.
> > 
> > Fixes: d1e879ec600f ("wifi: iwlwifi: add iwlmld sub-driver")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >  drivers/net/wireless/intel/iwlwifi/mld/debugfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> > index c759c5c68dc0..1d4b2ad5d388 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> > @@ -556,8 +556,8 @@ iwl_dbgfs_vif_twt_setup_write(struct iwl_mld *mld, char *buf, size_t count,
> >  	};
> >  	struct ieee80211_vif *vif = data;
> >  	struct iwl_mld_vif *mld_vif = iwl_mld_vif_from_mac80211(vif);
> > +	struct iwl_dhc_cmd *cmd __free(kfree) = NULL;
> 
> hmm, I thought the recommended convention was to define __free() pointers at
> the point of allocation. cleanup.h explicitly says:
> 
>  * Given that the "__free(...) = NULL" pattern for variables defined at
>  * the top of the function poses this potential interdependency problem
>  * the recommendation is to always define and assign variables in one
>  * statement and not group variable definitions at the top of the
>  * function when __free() is used.
> 

People do it either way.  I'm agnostic so long as it doesn't have bugs.

regards,
dan carpenter

Re: [PATCH next] wifi: iwlwifi: Fix uninitialized variable with __free()

[Jeff Johnson]

On 3/12/2025 8:24 AM, Dan Carpenter wrote:
> On Wed, Mar 12, 2025 at 08:15:18AM -0700, Jeff Johnson wrote:
>> On 3/12/2025 1:31 AM, Dan Carpenter wrote:
>>> Pointers declared with the __free(kfree) attribute need to be initialized
>>> because they will be passed to kfree() on every return path.  There are
>>> two return statement before the "cmd" pointer is initialized so this
>>> leads to an uninitialized variable bug.
>>>
>>> Fixes: d1e879ec600f ("wifi: iwlwifi: add iwlmld sub-driver")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> ---
>>>  drivers/net/wireless/intel/iwlwifi/mld/debugfs.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
>>> index c759c5c68dc0..1d4b2ad5d388 100644
>>> --- a/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
>>> +++ b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
>>> @@ -556,8 +556,8 @@ iwl_dbgfs_vif_twt_setup_write(struct iwl_mld *mld, char *buf, size_t count,
>>>  	};
>>>  	struct ieee80211_vif *vif = data;
>>>  	struct iwl_mld_vif *mld_vif = iwl_mld_vif_from_mac80211(vif);
>>> +	struct iwl_dhc_cmd *cmd __free(kfree) = NULL;
>>
>> hmm, I thought the recommended convention was to define __free() pointers at
>> the point of allocation. cleanup.h explicitly says:
>>
>>  * Given that the "__free(...) = NULL" pattern for variables defined at
>>  * the top of the function poses this potential interdependency problem
>>  * the recommendation is to always define and assign variables in one
>>  * statement and not group variable definitions at the top of the
>>  * function when __free() is used.
>>
> 
> People do it either way.  I'm agnostic so long as it doesn't have bugs.

I've been doing it with the allocation since that's what Linus said he wanted:

https://lore.kernel.org/all/CAHk-=whO1+-4ALjFWSE0kzytz1kEbWPvy3xWvcUP1dJ4t-QqkA@mail.gmail.com/

But this patch is already in wireless-next, so it's moot now.

RE: [PATCH next] wifi: iwlwifi: Fix uninitialized variable with __free()

[Korenblit, Miriam Rachel]

> -----Original Message-----
> From: Jeff Johnson <jeff.johnson@oss.qualcomm.com>
> Sent: Wednesday, 12 March 2025 17:15
> To: Dan Carpenter <dan.carpenter@linaro.org>; Korenblit, Miriam Rachel
> <miriam.rachel.korenblit@intel.com>
> Cc: Berg, Johannes <johannes.berg@intel.com>; Anjaneyulu, Pagadala Yesu
> <pagadala.yesu.anjaneyulu@intel.com>; Grumbach, Emmanuel
> <emmanuel.grumbach@intel.com>; Stern, Avraham
> <avraham.stern@intel.com>; Ben Shimol, Yedidya
> <yedidya.ben.shimol@intel.com>; Gabay, Daniel <daniel.gabay@intel.com>;
> linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-
> janitors@vger.kernel.org
> Subject: Re: [PATCH next] wifi: iwlwifi: Fix uninitialized variable with __free()
> 
> On 3/12/2025 1:31 AM, Dan Carpenter wrote:
> > Pointers declared with the __free(kfree) attribute need to be
> > initialized because they will be passed to kfree() on every return
> > path.  There are two return statement before the "cmd" pointer is
> > initialized so this leads to an uninitialized variable bug.
> >
> > Fixes: d1e879ec600f ("wifi: iwlwifi: add iwlmld sub-driver")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >  drivers/net/wireless/intel/iwlwifi/mld/debugfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> > b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> > index c759c5c68dc0..1d4b2ad5d388 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> > @@ -556,8 +556,8 @@ iwl_dbgfs_vif_twt_setup_write(struct iwl_mld *mld,
> char *buf, size_t count,
> >  	};
> >  	struct ieee80211_vif *vif = data;
> >  	struct iwl_mld_vif *mld_vif = iwl_mld_vif_from_mac80211(vif);
> > +	struct iwl_dhc_cmd *cmd __free(kfree) = NULL;
> 
> hmm, I thought the recommended convention was to define __free() pointers at
> the point of allocation. cleanup.h explicitly says:
> 
>  * Given that the "__free(...) = NULL" pattern for variables defined at
>  * the top of the function poses this potential interdependency problem
>  * the recommendation is to always define and assign variables in one
>  * statement and not group variable definitions at the top of the
>  * function when __free() is used.

This is used if you have multiple kfree variables (and the order of freeing matters)
or a guard() in that same function.
This is not the case here (and no reason that it will be in the future since this entire function is running under the wiphy lock),
and it is just a recommendation?
> 
> >  	struct iwl_dhc_twt_operation *dhc_twt_cmd;
> > -	struct iwl_dhc_cmd *cmd __free(kfree);
> >  	u64 target_wake_time;
> >  	u32 twt_operation, interval_exp, interval_mantissa, min_wake_duration;
> >  	u8 trigger, flow_type, flow_id, protection, tenth_param;

Re: [PATCH] wifi: iwlwifi: Fix uninitialized variable with __free()

[Markus Elfring]

…
> +++ b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> @@ -556,8 +556,8 @@ iwl_dbgfs_vif_twt_setup_write(struct iwl_mld *mld, char *buf, size_t count,
>  	};
>  	struct ieee80211_vif *vif = data;
>  	struct iwl_mld_vif *mld_vif = iwl_mld_vif_from_mac80211(vif);
> +	struct iwl_dhc_cmd *cmd __free(kfree) = NULL;
>  	struct iwl_dhc_twt_operation *dhc_twt_cmd;
> -	struct iwl_dhc_cmd *cmd __free(kfree);
…

Can it be recommended to reduce the scope for the affected local variable instead?
https://elixir.bootlin.com/linux/v6.14-rc6/source/include/linux/cleanup.h#L129-L133

Regards,
Markus

RE: [PATCH] wifi: iwlwifi: Fix uninitialized variable with __free()

[Korenblit, Miriam Rachel]

> -----Original Message-----
> From: Markus Elfring <Markus.Elfring@web.de>
> Sent: Wednesday, 12 March 2025 17:42
> To: Dan Carpenter <dan.carpenter@linaro.org>; linux-wireless@vger.kernel.org
> Cc: kernel-janitors@vger.kernel.org; LKML <linux-kernel@vger.kernel.org>;
> Anjaneyulu, Pagadala Yesu <pagadala.yesu.anjaneyulu@intel.com>; Stern,
> Avraham <avraham.stern@intel.com>; Gabay, Daniel <daniel.gabay@intel.com>;
> Grumbach, Emmanuel <emmanuel.grumbach@intel.com>; Jeff Johnson
> <jeff.johnson@oss.qualcomm.com>; Berg, Johannes <johannes.berg@intel.com>;
> Korenblit, Miriam Rachel <miriam.rachel.korenblit@intel.com>; Ben Shimol,
> Yedidya <yedidya.ben.shimol@intel.com>
> Subject: Re: [PATCH] wifi: iwlwifi: Fix uninitialized variable with __free()
> 
> …
> > +++ b/drivers/net/wireless/intel/iwlwifi/mld/debugfs.c
> > @@ -556,8 +556,8 @@ iwl_dbgfs_vif_twt_setup_write(struct iwl_mld *mld,
> char *buf, size_t count,
> >  	};
> >  	struct ieee80211_vif *vif = data;
> >  	struct iwl_mld_vif *mld_vif = iwl_mld_vif_from_mac80211(vif);
> > +	struct iwl_dhc_cmd *cmd __free(kfree) = NULL;
> >  	struct iwl_dhc_twt_operation *dhc_twt_cmd;
> > -	struct iwl_dhc_cmd *cmd __free(kfree);
> …
> 
> Can it be recommended to reduce the scope for the affected local variable
> instead?
> https://elixir.bootlin.com/linux/v6.14-rc6/source/include/linux/cleanup.h#L129-
> L133

As said in reply to Jeff's review -
This is recommended if you have multiple __kfree variables (and the order of freeing matters)
Or if there is a guard() later in the function.
This function is not the case and will probably never be since the entire function is running under the relevant lock.

Miri

Re: wifi: iwlwifi: Fix uninitialized variable with __free()

[Markus Elfring]

>> Can it be recommended to reduce the scope for the affected local variable instead?
>> https://elixir.bootlin.com/linux/v6.14-rc6/source/include/linux/cleanup.h#L129-L133
>
> As said in reply to Jeff's review -
> This is recommended if you have multiple __kfree variables (and the order of freeing matters)
> Or if there is a guard() later in the function.
How does such a view fit to the documentation of this programming interface?

Regards,
Markus

RE: wifi: iwlwifi: Fix uninitialized variable with __free()

[Korenblit, Miriam Rachel]

> -----Original Message-----
> From: Markus Elfring <Markus.Elfring@web.de>
> Sent: Wednesday, 12 March 2025 22:17
> To: Korenblit, Miriam Rachel <miriam.rachel.korenblit@intel.com>; Dan
> Carpenter <dan.carpenter@linaro.org>; linux-wireless@vger.kernel.org
> Cc: kernel-janitors@vger.kernel.org; LKML <linux-kernel@vger.kernel.org>;
> Anjaneyulu, Pagadala Yesu <pagadala.yesu.anjaneyulu@intel.com>; Stern,
> Avraham <avraham.stern@intel.com>; Gabay, Daniel <daniel.gabay@intel.com>;
> Grumbach, Emmanuel <emmanuel.grumbach@intel.com>; Jeff Johnson
> <jeff.johnson@oss.qualcomm.com>; Berg, Johannes <johannes.berg@intel.com>;
> Ben Shimol, Yedidya <yedidya.ben.shimol@intel.com>
> Subject: Re: wifi: iwlwifi: Fix uninitialized variable with __free()
> 
> >> Can it be recommended to reduce the scope for the affected local variable
> instead?
> >> https://elixir.bootlin.com/linux/v6.14-rc6/source/include/linux/clean
> >> up.h#L129-L133
> >
> > As said in reply to Jeff's review -
> > This is recommended if you have multiple __kfree variables (and the
> > order of freeing matters) Or if there is a guard() later in the function.
> How does such a view fit to the documentation of this programming interface?

Did you read about the possible bug that leads to this recommendation?
https://elixir.bootlin.com/linux/v6.14-rc6/source/include/linux/cleanup.h#L64-L127