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 <[email protected]>
Reported-by: Kaloyan Nikolov <[email protected]>
Tested-by: Kaloyan Nikolov <[email protected]>
---
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
Hi,
On Tue, Aug 25, 2020 at 8:38 AM Maximilian Luz <[email protected]> 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 <[email protected]>
> Reported-by: Kaloyan Nikolov <[email protected]>
> Tested-by: Kaloyan Nikolov <[email protected]>
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 <[email protected]>
Tested-by: Brian Norris <[email protected]>
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
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
Brian Norris <[email protected]> writes:
> Hi,
>
> On Tue, Aug 25, 2020 at 8:38 AM Maximilian Luz <[email protected]> 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 <[email protected]>
>> Reported-by: Kaloyan Nikolov <[email protected]>
>> Tested-by: Kaloyan Nikolov <[email protected]>
>
> 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 <[email protected]>
Reported-by: Kaloyan Nikolov <[email protected]>
Tested-by: Kaloyan Nikolov <[email protected]>
Reviewed-by: Dan Carpenter <[email protected]>
Reviewed-by: Brian Norris <[email protected]>
Tested-by: Brian Norris <[email protected]>
--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches