On Fri, 2023-09-15 at 11:29 +0300, Dan Carpenter wrote:
> Hello Johannes Berg,
>
> The patch fdf7cb4185b6: "mac80211: accept key reinstall without
> changing anything" from Sep 5, 2017 (linux-next),
Huh that's kind of old :-)
> leads to the
> following Smatch static checker warning:
>
> net/mac80211/key.c:1385 ieee80211_gtk_rekey_add()
> warn: 'key' was already freed.
>
> net/mac80211/key.c
> 899
> 900 /*
> 901 * Silently accept key re-installation without really installing the
> 902 * new version of the key to avoid nonce reuse or replay issues.
> 903 */
> 904 if (ieee80211_key_identical(sdata, old_key, key)) {
> 905 ieee80211_key_free_unused(key);
> ^^^
> key is freed here.
>
> 906 return 0;
> 907 }
> 908
>
> net/mac80211/key.c
> 1375 return ERR_CAST(key);
> 1376
> 1377 if (sdata->u.mgd.mfp != IEEE80211_MFP_DISABLED)
> 1378 key->conf.flags |= IEEE80211_KEY_FLAG_RX_MGMT;
> 1379
> 1380 /* FIXME: this function needs to get a link ID */
> 1381 err = ieee80211_key_link(key, &sdata->deflink, NULL);
> 1382 if (err)
> 1383 return ERR_PTR(err);
> 1384
> --> 1385 return &key->conf;
> ^^^^^^^^^^
> Use after free.
>
Indeed. The good thing is that basically this really shouldn't happen
since this latter function is only used by iwlwifi, and that already
checks for identical keys (i.e. KRACK protection).
Nevertheless, we should of course fix it, just not really exactly sure
how - I guess ieee80211_gtk_rekey_add() should possibly just return the
old key in that case, need to see how it's used.
johannes
