Re: [PATCH net v2] net: wireless: Use kfree_sensitive instead of kfree

Re: [PATCH net v2] net: wireless: Use kfree_sensitive instead of kfree

[Greg Kroah-Hartman]

On Wed, Jul 19, 2023 at 02:21:16AM +0000, 王明-软件底层技术部 wrote:
> key contains private part of the key, so better use
> kfree_sensitive to free it.
> 
> Fixes: 7cec84fdfd88 ("staging: wilc1000: split add_key() to avoid line over 80 chars")
> Signed-off-by: Wang Ming <machel@vivo.com>

Why not also use 王明-软件底层技术部 <machel@vivo.com> here as well?


> ---
>  drivers/net/wireless/microchip/wilc1000/cfg80211.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

No change log from what changed from version 1?



> 
> diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> index b545d93c6e37..45bcadeba2da 100644
> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> @@ -518,7 +518,7 @@ static int wilc_wfi_cfg_allocate_wpa_igtk_entry(struct wilc_priv *priv, u8 idx)
>  static int wilc_wfi_cfg_copy_wpa_info(struct wilc_wfi_key *key_info,
>  				      struct key_params *params)
>  {
> -	kfree(key_info->key);
> +	kfree_sensitive(key_info->key);
>  
>  	key_info->key = kmemdup(params->key, params->key_len, GFP_KERNEL);
>  	if (!key_info->key)
> @@ -656,7 +656,7 @@ static int del_key(struct wiphy *wiphy, struct net_device *netdev, int link_id,
>  	if (!pairwise && (key_index == 4 || key_index == 5)) {
>  		key_index -= 4;
>  		if (priv->wilc_igtk[key_index]) {
> -			kfree(priv->wilc_igtk[key_index]->key);
> +			kfree_sensitive(priv->wilc_igtk[key_index]->key);

Normally "kfree_sensitive()" is used at the end of a function for when
kfree() of a local variable might not be called because the compiler
thinks it is smarter than us and optimizes it away.

Putting it here, in the normal operation, really doesn't do anything,
right?  There's always going to be odd data in the heap and normal
distros/users who care about that, always wipe the heap when doing new
allocations as that's a kernel config option.

So what exactly is this "fixing" here?  What is the bug?

thanks,

greg k-h

Re: [PATCH net v2] net: wireless: Use kfree_sensitive instead of kfree

[Pavel Machek]

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

Hi!

> > diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> > index b545d93c6e37..45bcadeba2da 100644
> > --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> > +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> > @@ -518,7 +518,7 @@ static int wilc_wfi_cfg_allocate_wpa_igtk_entry(struct wilc_priv *priv, u8 idx)
> >  static int wilc_wfi_cfg_copy_wpa_info(struct wilc_wfi_key *key_info,
> >  				      struct key_params *params)
> >  {
> > -	kfree(key_info->key);
> > +	kfree_sensitive(key_info->key);
> >  
> >  	key_info->key = kmemdup(params->key, params->key_len, GFP_KERNEL);
> >  	if (!key_info->key)
> > @@ -656,7 +656,7 @@ static int del_key(struct wiphy *wiphy, struct net_device *netdev, int link_id,
> >  	if (!pairwise && (key_index == 4 || key_index == 5)) {
> >  		key_index -= 4;
> >  		if (priv->wilc_igtk[key_index]) {
> > -			kfree(priv->wilc_igtk[key_index]->key);
> > +			kfree_sensitive(priv->wilc_igtk[key_index]->key);
> 
> Normally "kfree_sensitive()" is used at the end of a function for when
> kfree() of a local variable might not be called because the compiler
> thinks it is smarter than us and optimizes it away.
> 
> Putting it here, in the normal operation, really doesn't do anything,
> right?  There's always going to be odd data in the heap and normal

It does memzero.

https://elixir.bootlin.com/linux/latest/source/mm/slab_common.c#L1411
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

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

Re: [PATCH net v2] net: wireless: Use kfree_sensitive instead of kfree

[Greg Kroah-Hartman]

On Thu, Jul 20, 2023 at 11:06:36AM +0200, Pavel Machek wrote:
> Hi!
> 
> > > diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> > > index b545d93c6e37..45bcadeba2da 100644
> > > --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> > > +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> > > @@ -518,7 +518,7 @@ static int wilc_wfi_cfg_allocate_wpa_igtk_entry(struct wilc_priv *priv, u8 idx)
> > >  static int wilc_wfi_cfg_copy_wpa_info(struct wilc_wfi_key *key_info,
> > >  				      struct key_params *params)
> > >  {
> > > -	kfree(key_info->key);
> > > +	kfree_sensitive(key_info->key);
> > >  
> > >  	key_info->key = kmemdup(params->key, params->key_len, GFP_KERNEL);
> > >  	if (!key_info->key)
> > > @@ -656,7 +656,7 @@ static int del_key(struct wiphy *wiphy, struct net_device *netdev, int link_id,
> > >  	if (!pairwise && (key_index == 4 || key_index == 5)) {
> > >  		key_index -= 4;
> > >  		if (priv->wilc_igtk[key_index]) {
> > > -			kfree(priv->wilc_igtk[key_index]->key);
> > > +			kfree_sensitive(priv->wilc_igtk[key_index]->key);
> > 
> > Normally "kfree_sensitive()" is used at the end of a function for when
> > kfree() of a local variable might not be called because the compiler
> > thinks it is smarter than us and optimizes it away.
> > 
> > Putting it here, in the normal operation, really doesn't do anything,
> > right?  There's always going to be odd data in the heap and normal
> 
> It does memzero.
> 
> https://elixir.bootlin.com/linux/latest/source/mm/slab_common.c#L1411

I know what it does, I'm saying "why is this required".