On Wed, May 04, 2022 at 09:28:46AM +0200, Johannes Berg wrote: > On Tue, 2022-05-03 at 18:44 -0700, Kees Cook wrote: > > > > @@ -2277,7 +2274,7 @@ cfg80211_update_notlisted_nontrans(struct wiphy *wiphy, > > size_t ielen = len - offsetof(struct ieee80211_mgmt, > > u.probe_resp.variable); > > size_t new_ie_len; > > - struct cfg80211_bss_ies *new_ies; > > + struct cfg80211_bss_ies *new_ies = NULL; > > const struct cfg80211_bss_ies *old; > > u8 cpy_len; > > > > @@ -2314,8 +2311,7 @@ cfg80211_update_notlisted_nontrans(struct wiphy *wiphy, > > if (!new_ie) > > return; > > > > - new_ies = kzalloc(sizeof(*new_ies) + new_ie_len, GFP_ATOMIC); > > - if (!new_ies) > > + if (mem_to_flex_dup(&new_ies, new_ie, new_ie_len, GFP_ATOMIC)) > > goto out_free; > > > > pos = new_ie; > > @@ -2333,10 +2329,8 @@ cfg80211_update_notlisted_nontrans(struct wiphy *wiphy, > > memcpy(pos, mbssid + cpy_len, ((ie + ielen) - (mbssid + cpy_len))); > > > > /* update ie */ > > - new_ies->len = new_ie_len; > > new_ies->tsf = le64_to_cpu(mgmt->u.probe_resp.timestamp); > > new_ies->from_beacon = ieee80211_is_beacon(mgmt->frame_control); > > - memcpy(new_ies->data, new_ie, new_ie_len); > > This introduces a bug, "new_ie" is modified between the kzalloc() and > the memcpy(), but you've moved the memcpy() into the allocation. In > fact, new_ie is completely freshly kzalloc()'ed at this point. So you > need to change the ordering here, but since new_ie is freed pretty much > immediately, we can probably just build the stuff directly inside > new_ies->data, though then of course we cannot use your helper anymore? Eek, yes, thanks. My attempt to locate the alloc/memcpy pattern failed to take into account anything touch the source between alloc and memcpy. I'll double check the other examples. -Kees -- Kees Cook
