| Message ID | 20200825153829.38043-1-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4afc850e2e9e781976fb2c7852ce7bac374af938 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [net] mwifiex: Increase AES key storage size to 256 bits | expand |
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
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
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
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
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>
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
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,
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(-)