| Message ID | 20220504014440.3697851-13-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Introduce flexible array struct memcpy() helpers | expand |
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? johannes
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
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 68713388b617..fa236015f6ef 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -2600,9 +2600,9 @@ struct cfg80211_inform_bss { struct cfg80211_bss_ies { u64 tsf; struct rcu_head rcu_head; - int len; + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(int, len); bool from_beacon; - u8 data[]; + DECLARE_FLEX_ARRAY_ELEMENTS(u8, data); }; /** diff --git a/net/wireless/scan.c b/net/wireless/scan.c index 4a6d86432910..9f53d05c6aaa 100644 --- a/net/wireless/scan.c +++ b/net/wireless/scan.c @@ -1932,7 +1932,7 @@ cfg80211_inform_single_bss_data(struct wiphy *wiphy, gfp_t gfp) { struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy); - struct cfg80211_bss_ies *ies; + struct cfg80211_bss_ies *ies = NULL; struct ieee80211_channel *channel; struct cfg80211_internal_bss tmp = {}, *res; int bss_type; @@ -1978,13 +1978,10 @@ cfg80211_inform_single_bss_data(struct wiphy *wiphy, * override the IEs pointer should we have received an earlier * indication of Probe Response data. */ - ies = kzalloc(sizeof(*ies) + ielen, gfp); - if (!ies) + if (mem_to_flex_dup(&ies, ie, ielen, gfp)) return NULL; - ies->len = ielen; ies->tsf = tsf; ies->from_beacon = false; - memcpy(ies->data, ie, ielen); switch (ftype) { case CFG80211_BSS_FTYPE_BEACON: @@ -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); if (ieee80211_is_probe_resp(mgmt->frame_control)) { old = rcu_access_pointer(nontrans_bss->proberesp_ies); rcu_assign_pointer(nontrans_bss->proberesp_ies, new_ies); @@ -2363,7 +2357,7 @@ cfg80211_inform_single_bss_frame_data(struct wiphy *wiphy, gfp_t gfp) { struct cfg80211_internal_bss tmp = {}, *res; - struct cfg80211_bss_ies *ies; + struct cfg80211_bss_ies *ies = NULL; struct ieee80211_channel *channel; bool signal_valid; struct ieee80211_ext *ext = NULL; @@ -2442,14 +2436,11 @@ cfg80211_inform_single_bss_frame_data(struct wiphy *wiphy, capability = le16_to_cpu(mgmt->u.probe_resp.capab_info); } - ies = kzalloc(sizeof(*ies) + ielen, gfp); - if (!ies) + if (mem_to_flex_dup(&ies, variable, ielen, gfp)) return NULL; - ies->len = ielen; ies->tsf = le64_to_cpu(mgmt->u.probe_resp.timestamp); ies->from_beacon = ieee80211_is_beacon(mgmt->frame_control) || ieee80211_is_s1g_beacon(mgmt->frame_control); - memcpy(ies->data, variable, ielen); if (ieee80211_is_probe_resp(mgmt->frame_control)) rcu_assign_pointer(tmp.pub.proberesp_ies, ies);
As part of the work to perform bounds checking on all memcpy() uses, replace the open-coded a deserialization of bytes out of memory into a trailing flexible array by using a flex_array.h helper to perform the allocation, bounds checking, and copying. Cc: Johannes Berg <johannes@sipsolutions.net> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Eric Dumazet <edumazet@google.com> Cc: linux-wireless@vger.kernel.org Cc: netdev@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- include/net/cfg80211.h | 4 ++-- net/wireless/scan.c | 21 ++++++--------------- 2 files changed, 8 insertions(+), 17 deletions(-)