* re: mac80211: add generic cipher scheme support
@ 2013-12-03 16:37 Dan Carpenter
2013-12-04 13:07 ` Stepanov, Max
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2013-12-03 16:37 UTC (permalink / raw)
To: Max.Stepanov; +Cc: linux-wireless
Hello Max Stepanov,
The patch 2475b1cc0d52: "mac80211: add generic cipher scheme support"
from Mar 24, 2013, leads to the following
static checker warning: "net/mac80211/cfg.c:305 ieee80211_get_key()
warn: buffer overflow 'sta->ptk' 4 <= 5"
net/mac80211/cfg.c
279 static int ieee80211_get_key(struct wiphy *wiphy, struct net_device *dev,
280 u8 key_idx, bool pairwise, const u8 *mac_addr,
281 void *cookie,
282 void (*callback)(void *cookie,
283 struct key_params *params))
284 {
285 struct ieee80211_sub_if_data *sdata;
286 struct sta_info *sta = NULL;
287 u8 seq[6] = {0};
288 struct key_params params;
289 struct ieee80211_key *key = NULL;
290 u64 pn64;
291 u32 iv32;
292 u16 iv16;
293 int err = -ENOENT;
294
295 sdata = IEEE80211_DEV_TO_SUB_IF(dev);
296
297 rcu_read_lock();
298
299 if (mac_addr) {
300 sta = sta_info_get_bss(sdata, mac_addr);
301 if (!sta)
302 goto out;
303
304 if (pairwise)
305 key = rcu_dereference(sta->ptk[key_idx]);
306 else if (key_idx < NUM_DEFAULT_KEYS)
307 key = rcu_dereference(sta->gtk[key_idx]);
key_idx is a number between 0 and 5.
NUM_DEFAULT_KEYS is 4.
->ptk has 4 elements.
->gtk has 6 elements.
I looked but I didn't see that "pairwise" implied that key_idx is less
than 4. These are set in nl80211_get_key().
308 } else
309 key = rcu_dereference(sdata->keys[key_idx]);
310
311 if (!key)
312 goto out;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: mac80211: add generic cipher scheme support
2013-12-03 16:37 Dan Carpenter
@ 2013-12-04 13:07 ` Stepanov, Max
2013-12-04 13:19 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Stepanov, Max @ 2013-12-04 13:07 UTC (permalink / raw)
To: 'Dan Carpenter'; +Cc: linux-wireless@vger.kernel.org
> 304 if (pairwise)
> 305 key = rcu_dereference(sta->ptk[key_idx]);
> 306 else if (key_idx < NUM_DEFAULT_KEYS)
> 307 key = rcu_dereference(sta->gtk[key_idx]);
>
>key_idx is a number between 0 and 5.
>NUM_DEFAULT_KEYS is 4.
>->ptk has 4 elements.
>->gtk has 6 elements.
>
>I looked but I didn't see that "pairwise" implied that key_idx is less than 4.
>These are set in nl80211_get_key().
Hi Dan,
1. ptk - I think you are right here - need to verify that key_idx doesn't exceed sta->ptk array boundaries. I'll prepare the fix
2. gtk - frankly I'm not sure about key_idx < NUM_DEFAULT_KEYS. I understand why it's here: not to return management keys, but I don't see a reason why not to do it... In any case I'll prepare the fix for this case too
Max
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mac80211: add generic cipher scheme support
2013-12-04 13:07 ` Stepanov, Max
@ 2013-12-04 13:19 ` Dan Carpenter
2013-12-04 13:30 ` Stepanov, Max
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2013-12-04 13:19 UTC (permalink / raw)
To: Stepanov, Max; +Cc: linux-wireless@vger.kernel.org
On Wed, Dec 04, 2013 at 01:07:29PM +0000, Stepanov, Max wrote:
> > 304 if (pairwise)
> > 305 key = rcu_dereference(sta->ptk[key_idx]);
> > 306 else if (key_idx < NUM_DEFAULT_KEYS)
> > 307 key = rcu_dereference(sta->gtk[key_idx]);
> >
> >key_idx is a number between 0 and 5.
> >NUM_DEFAULT_KEYS is 4.
> >->ptk has 4 elements.
> >->gtk has 6 elements.
> >
> >I looked but I didn't see that "pairwise" implied that key_idx is less than 4.
> >These are set in nl80211_get_key().
>
> Hi Dan,
>
> 1. ptk - I think you are right here - need to verify that key_idx doesn't exceed sta->ptk array boundaries. I'll prepare the fix
> 2. gtk - frankly I'm not sure about key_idx < NUM_DEFAULT_KEYS. I understand why it's here: not to return management keys, but I don't see a reason why not to do it... In any case I'll prepare the fix for this case too
It worries me that we are doing #2 without being sure... I have no
idea about this code, I'm just doing static analysis without a deep
understanding at all.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: mac80211: add generic cipher scheme support
2013-12-04 13:19 ` Dan Carpenter
@ 2013-12-04 13:30 ` Stepanov, Max
0 siblings, 0 replies; 6+ messages in thread
From: Stepanov, Max @ 2013-12-04 13:30 UTC (permalink / raw)
To: 'Dan Carpenter'; +Cc: linux-wireless@vger.kernel.org
>It worries me that we are doing #2 without being sure... I have no idea about
>this code, I'm just doing static analysis without a deep understanding at all.
I understand that. I'll submit the second fix as RFC to get more input...
Regards,
Max
^ permalink raw reply [flat|nested] 6+ messages in thread
* re: mac80211: add generic cipher scheme support
@ 2014-07-08 13:43 Dan Carpenter
2014-07-08 14:31 ` Stepanov, Max
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2014-07-08 13:43 UTC (permalink / raw)
To: Max.Stepanov; +Cc: linux-wireless
Hello Max Stepanov,
This is a semi-automatic email about new static checker warnings.
The patch 2475b1cc0d52: "mac80211: add generic cipher scheme support"
from Mar 24, 2013, leads to the following Smatch complaint:
net/mac80211/rx.c:1551 ieee80211_rx_h_decrypt()
error: we previously assumed 'rx->sta' could be null (see line 1401)
net/mac80211/rx.c
1400
1401 if (rx->sta) {
^^^^^^^
Many places in this function assume that ->sta can be NULL.
1402 int keyid = rx->sta->ptk_idx;
1403
1404 if (ieee80211_has_protected(fc) && rx->sta->cipher_scheme) {
1405 cs = rx->sta->cipher_scheme;
1406 keyid = iwl80211_get_cs_keyid(cs, rx->skb);
[ snip ]
1547 case WLAN_CIPHER_SUITE_AES_CMAC:
1548 result = ieee80211_crypto_aes_cmac_decrypt(rx);
1549 break;
1550 default:
1551 result = ieee80211_crypto_hw_decrypt(rx);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
But if it's NULL here then we will Oops.
1552 }
1553
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: mac80211: add generic cipher scheme support
2014-07-08 13:43 mac80211: add generic cipher scheme support Dan Carpenter
@ 2014-07-08 14:31 ` Stepanov, Max
0 siblings, 0 replies; 6+ messages in thread
From: Stepanov, Max @ 2014-07-08 14:31 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-wireless@vger.kernel.org
> This is a semi-automatic email about new static checker warnings.
>
> The patch 2475b1cc0d52: "mac80211: add generic cipher scheme support"
> from Mar 24, 2013, leads to the following Smatch complaint:
>
> net/mac80211/rx.c:1551 ieee80211_rx_h_decrypt()
> error: we previously assumed 'rx->sta' could be null (see line 1401)
> ...
> 1551 result = ieee80211_crypto_hw_decrypt(rx);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ But if it's NULL here then we will Oops.
Thank you Dan. I'll add verification of rx->sta == NULL to ieee80211_crypto_hw_decrypt().
Max
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-08 14:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-08 13:43 mac80211: add generic cipher scheme support Dan Carpenter
2014-07-08 14:31 ` Stepanov, Max
-- strict thread matches above, loose matches on Subject: below --
2013-12-03 16:37 Dan Carpenter
2013-12-04 13:07 ` Stepanov, Max
2013-12-04 13:19 ` Dan Carpenter
2013-12-04 13:30 ` Stepanov, Max
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox