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

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

[Maximilian Luz]

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,
-- 
2.28.0

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

[Dan Carpenter]

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

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

[Brian Norris]

Hi,

On Tue, Aug 25, 2020 at 8:38 AM Maximilian Luz <luzmaximilian@gmail.com> 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>

Thanks for this! I just happened to notice this breakage here, as we
just merged the relevant -stable updates. I think it would be wise to
get the Fixes tag Dan noted, when Kalle lands this.

Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>

Also, while technically the regressing commit (e18696786548 ("mwifiex:
Prevent memory corruption handling keys")) was fixing a potential
overflow, the encasing command structure (struct host_cmd_ds_command)
is a union of a ton of other command layouts, and likely had plenty of
padding at the end, which would at least explain why non-malicious
scenarios weren't problematic pre-commit-e18696786548. It's also not
clear to me how much the network can directly determine this length,
but I suppose that's beside the point now -- it's good to fix both of
these bugs.

Regards,
Brian

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

[Maximilian Luz]

On 8/25/20 8:51 PM, Dan Carpenter wrote:
> 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.

I'd argue the same. Any critical out-of-bounds access was just never
discovered (at least for 256 bit keys) due to the struct containing the
key being a union member, as Brian observed.

> 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?

Given that I have pretty much no knowledge of the driver, I
unfortunately can't answer this. But I agree that this should be looked
at. I think this has the potential to work now (as long as the maximum
key size is 256 bit) but break in the future if the maximum key size
ever gets larger and the check excludes some key types that would be
valid in this context (if there are even any?).

Regards,
Max

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

[Maximilian Luz]

On 8/25/20 9:30 PM, Brian Norris wrote:
> Also, while technically the regressing commit (e18696786548 ("mwifiex:
> Prevent memory corruption handling keys")) was fixing a potential
> overflow, the encasing command structure (struct host_cmd_ds_command)
> is a union of a ton of other command layouts, and likely had plenty of
> padding at the end, which would at least explain why non-malicious
> scenarios weren't problematic pre-commit-e18696786548.

This is pretty much spot on, although as far as I can tell, the padding
comes from struct mwifiex_ie_type_key_param_set_v2. That contains a
key_params member, which is a union over all supported key types,
including other 256 bit types (like struct mwifiex_wapi_param).

I should also note that this fix also affects mwifiex_set_aes_key_v2(),
where sizeof(struct mwifiex_aes_param) is used to calculate the command
length of what looks like a command being sent to the chip. This should
probably be reviewed by someone with a bit more inside knowledge about
the driver, as this could potentially break something due to the commit
changing it from 16 to 32. I think, however, that this might actually
also fix a potential issue when setting 256 bit AES keys.

Regards,
Max

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

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

> Hi,
>
> On Tue, Aug 25, 2020 at 8:38 AM Maximilian Luz <luzmaximilian@gmail.com> 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>
>
> Thanks for this! I just happened to notice this breakage here, as we
> just merged the relevant -stable updates. I think it would be wise to
> get the Fixes tag Dan noted, when Kalle lands this.

Ok, I'll queue this for v5.9 and add the Fixes tag.

If anyone is bored it would be great to get patchwork automatically
pickup the Fixes tags :) It already does that Acked-by, Reported-by and
Tested-by tags:

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
Reported-by: Kaloyan Nikolov <konik98@gmail.com>
Tested-by: Kaloyan Nikolov <konik98@gmail.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Kalle Valo]

Maximilian Luz <luzmaximilian@gmail.com> 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.
> 
> Fixes: e18696786548 ("mwifiex: Prevent memory corruption handling keys")
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> Reported-by: Kaloyan Nikolov <konik98@gmail.com>
> Tested-by: Kaloyan Nikolov <konik98@gmail.com>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Tested-by: Brian Norris <briannorris@chromium.org>

Patch applied to wireless-drivers.git, thanks.

4afc850e2e9e mwifiex: Increase AES key storage size to 256 bits

-- 
https://patchwork.kernel.org/patch/11735929/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches