Re: [PATCH net] mwifiex: Increase AES key storage size to 256 bits

[Dan Carpenter <dan.carpenter@oracle.com>]

On Tue, Aug 25, 2020 at 05:38:29PM +0200, Maximilian Luz wrote:
> Following commit e18696786548 ("mwifiex: Prevent memory corruption
> handling keys") the mwifiex driver fails to authenticate with certain
> networks, specifically networks with 256 bit keys, and repeatedly asks
> for the password. The kernel log repeats the following lines (id and
> bssid redacted):
> 
>     mwifiex_pcie 0000:01:00.0: info: trying to associate to '<id>' bssid <bssid>
>     mwifiex_pcie 0000:01:00.0: info: associated to bssid <bssid> successfully
>     mwifiex_pcie 0000:01:00.0: crypto keys added
>     mwifiex_pcie 0000:01:00.0: info: successfully disconnected from <bssid>: reason code 3
> 
> Tracking down this problem lead to the overflow check introduced by the
> aforementioned commit into mwifiex_ret_802_11_key_material_v2(). This
> check fails on networks with 256 bit keys due to the current storage
> size for AES keys in struct mwifiex_aes_param being only 128 bit.
> 
> To fix this issue, increase the storage size for AES keys to 256 bit.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> Reported-by: Kaloyan Nikolov <konik98@gmail.com>
> Tested-by: Kaloyan Nikolov <konik98@gmail.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/fw.h          | 2 +-
>  drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
> index 8047e307892e3..d9f8bdbc817b2 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -954,7 +954,7 @@ struct mwifiex_tkip_param {
>  struct mwifiex_aes_param {
>  	u8 pn[WPA_PN_SIZE];
>  	__le16 key_len;
> -	u8 key[WLAN_KEY_LEN_CCMP];
> +	u8 key[WLAN_KEY_LEN_CCMP_256];
>  } __packed;
>  
>  struct mwifiex_wapi_param {
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> index 962d8bfe6f101..119ccacd1fcc4 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> @@ -619,7 +619,7 @@ static int mwifiex_ret_802_11_key_material_v2(struct mwifiex_private *priv,
>  	key_v2 = &resp->params.key_material_v2;
>  
>  	len = le16_to_cpu(key_v2->key_param_set.key_params.aes.key_len);
> -	if (len > WLAN_KEY_LEN_CCMP)
> +	if (len > sizeof(key_v2->key_param_set.key_params.aes.key))
>  		return -EINVAL;
>  
>  	if (le16_to_cpu(key_v2->action) == HostCmd_ACT_GEN_SET) {
> @@ -635,7 +635,7 @@ static int mwifiex_ret_802_11_key_material_v2(struct mwifiex_private *priv,
>  		return 0;
>  
>  	memset(priv->aes_key_v2.key_param_set.key_params.aes.key, 0,
> -	       WLAN_KEY_LEN_CCMP);
> +	       sizeof(key_v2->key_param_set.key_params.aes.key));
>  	priv->aes_key_v2.key_param_set.key_params.aes.key_len =
>  				cpu_to_le16(len);
>  	memcpy(priv->aes_key_v2.key_param_set.key_params.aes.key,

It's good to get the sizes correct.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

I sort of feel like the code was broken before I added the bounds
checking but it's also okay if the Fixes tag points to my change as
well just to make backporting easier.

Fixes: e18696786548 ("mwifiex: Prevent memory corruption handling keys")

Another question would be if it would be better to move the bounds
check after the "if (key_v2->key_param_set.key_type != KEY_TYPE_ID_AES)"
check?  Do we care if the length is invalid on the other paths?

regards,
dan carpenter