2023-07-17 09:46:19

by Wang Ming

Subject: [PATCH net v1] net: wireless: Use kfree_sensitive instead of kfree

key might contain 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 <[email protected]>
---
drivers/net/wireless/microchip/wilc1000/cfg80211.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

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);
priv->wilc_igtk[key_index]->key = NULL;
kfree(priv->wilc_igtk[key_index]->seq);
priv->wilc_igtk[key_index]->seq = NULL;
@@ -665,7 +665,7 @@ static int del_key(struct wiphy *wiphy, struct net_device *netdev, int link_id,
}
} else {
if (priv->wilc_gtk[key_index]) {
- kfree(priv->wilc_gtk[key_index]->key);
+ kfree_sensitive(priv->wilc_gtk[key_index]->key);
priv->wilc_gtk[key_index]->key = NULL;
kfree(priv->wilc_gtk[key_index]->seq);
priv->wilc_gtk[key_index]->seq = NULL;
@@ -674,7 +674,7 @@ static int del_key(struct wiphy *wiphy, struct net_device *netdev, int link_id,
priv->wilc_gtk[key_index] = NULL;
}
if (priv->wilc_ptk[key_index]) {
- kfree(priv->wilc_ptk[key_index]->key);
+ kfree_sensitive(priv->wilc_ptk[key_index]->key);
priv->wilc_ptk[key_index]->key = NULL;
kfree(priv->wilc_ptk[key_index]->seq);
priv->wilc_ptk[key_index]->seq = NULL;
--
2.25.1

2023-07-17 14:50:58

by Greg Kroah-Hartman

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

On Mon, Jul 17, 2023 at 05:34:43PM +0800, Wang Ming wrote:
> key might contain private part of the key, so better use
> kfree_sensitive to free it.

"might"? What determines if it does, or does not, contain the private
part of the key?

Shouldn't this always be known?

> Fixes: 7cec84fdfd88 ("staging: wilc1000: split add_key() to avoid line over 80 chars")
> Signed-off-by: Wang Ming <[email protected]>
> ---
> drivers/net/wireless/microchip/wilc1000/cfg80211.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> 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);

For most systems, this would have been wiped when kfree() was called due
to allocation being zeroed out, right?

But, if you want to be safe, and you know this was a private key, that's
fine to do as well, but please figure out if this really is, or is not,
a private key.

thanks,

greg k-h