[PATCH 12/32] cfg80211: Use mem_to_flex_dup() with struct cfg80211_bss_ies

[Kees Cook]

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