[PATCH] Use default IEs in wpa_supplicant_trigger_scan

[Matthew Wang]

> This sounds a bit scary when most existing calls to
> wpa_supplicant_trigger_scan() were modified to use default_ies == true.

I've run all the hwsim tests, and it's been in production in ChromeOS
for the past few months with no issues. The extra IEs are just
capability indications (which end up being included in association
requests anyway), so including them in the scan shouldn't break
anything.

> How can this be sure that params->extra_ies (const u8 pointer for a
> reason to imply it might not be allocated from heap) is actually
> allocated with os_malloc/zalloc() and something that is not referenced
> from somewhere else?

params->extra_ies is only either allocated from the heap, or a pointer
to wpabuf_head of wpa_supplicant_extra_ies. In the latter case, there
are a couple instance where trigger_scan is called with
params->extra_ies already allocated via wpa_supplicant_extra_ies, and
these all use default_ies = false (note the comment about the
default_ies parameter). So I guess the answer to your question is that
it's the caller's responsibility to set default_ies based on whether
or not params->extra_ies is expected to already be set.

> Should this set params->extra_ies to NULL after it was freed?

Yes, you're right. I've updated this to set it to NULL and set the
extra_ies_len field to 0. This should address your last comment as
well.

On Thu, Aug 19, 2021 at 7:16 AM Jouni Malinen <j at w1.fi> wrote:
>
> On Wed, Mar 31, 2021 at 11:52:21AM -0700, Matthew Wang wrote:
> > wpa_supplicant_trigger_scan previously wouldn't include any of the IEs
> > generated by wpa_supplicant_extra_ies. Instruct it to do so in most
> > cases. This is necessary because MBO STAs are required to include MBO
> > capabilities in their probe requests.
>
> > diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c
> > @@ -278,19 +278,40 @@ static void wpas_trigger_scan_cb(struct wpa_radio_work *work, int deinit)
> >   * wpa_supplicant_trigger_scan - Request driver to start a scan
> >   * @wpa_s: Pointer to wpa_supplicant data
> >   * @params: Scan parameters
> > + * @default_ies: Whether or not to use the default IEs in the probe request.
> > + * Note that this will free any existing IEs set in @params, so this shouldn't
> > + * be set if the IEs have already been set with wpa_supplicant_extra_ies.
> > + * Otherwise, wpabuf_free will lead to a double-free.
>
> This sounds a bit scary when most existing calls to
> wpa_supplicant_trigger_scan() were modified to use default_ies == true.
>
> > +     if (default_ies) {
> > +             if (params->extra_ies_len) {
> > +                     os_free((u8 *) params->extra_ies);
> > +             }
>
> How can this be sure that params->extra_ies (const u8 pointer for a
> reason to imply it might not be allocated from heap) is actually
> allocated with os_malloc/zalloc() and something that is not referenced
> from somewhere else? Should this set params->extra_ies to NULL after it
> was freed?
>
> > +             ies = wpa_supplicant_extra_ies(wpa_s);
> > +             if (ies) {
> > +                     params->extra_ies = wpabuf_head(ies);
> > +                     params->extra_ies_len = wpabuf_len(ies);
> > +             }
>
> This would seem to leave params->extra_ies_len > 0 and params->extra_ies
> pointing to freed memory if that wpa_supplicant_extra_ies() call fails..
>
> --
> Jouni Malinen                                            PGP id EFC895FA