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@xxxxxxxxx>
> Reported-by: Kaloyan Nikolov <konik98@xxxxxxxxx>
> Tested-by: Kaloyan Nikolov <konik98@xxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
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
