| Message ID | 20210331185221.61841-1-matthewmwang@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Use default IEs in wpa_supplicant_trigger_scan | expand |
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..
> 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@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
diff --git a/doc/dbus.doxygen b/doc/dbus.doxygen index 8231aac4180..1d93a1d7899 100644 --- a/doc/dbus.doxygen +++ b/doc/dbus.doxygen @@ -206,7 +206,7 @@ fi.w1.wpa_supplicant1.CreateInterface. <tr><th>Key</th><th>Value type</th><th>Description</th><th>Required</th> <tr><td>Type</td><td>s</td><td>Type of the scan. Possible values: "active", "passive"</td><td>Yes</td> <tr><td>SSIDs</td><td>aay</td><td>Array of SSIDs to scan for (applies only if scan type is active)</td><td>No</td> - <tr><td>IEs</td><td>aay</td><td>Information elements to used in active scan (applies only if scan type is active)</td><td>No</td> + <tr><td>IEs</td><td>aay</td><td>Information elements to used in active scan (applies only if scan type is active). Default IEs will be used in absence of this option.</td><td>No</td> <tr><td>Channels</td><td>a(uu)</td><td>Array of frequencies to scan in form of (center, width) in MHz.</td><td>No</td> <tr><td>AllowRoam</td><td>b</td><td>TRUE (or absent) to allow a roaming decision based on the results of this scan, FALSE to prevent a roaming decision.</td><td>No</td> </table> diff --git a/wpa_supplicant/bgscan_learn.c b/wpa_supplicant/bgscan_learn.c index cb732f709b9..3d10d911853 100644 --- a/wpa_supplicant/bgscan_learn.c +++ b/wpa_supplicant/bgscan_learn.c @@ -305,7 +305,7 @@ static void bgscan_learn_timeout(void *eloop_ctx, void *timeout_ctx) } wpa_printf(MSG_DEBUG, "bgscan learn: Request a background scan"); - if (wpa_supplicant_trigger_scan(wpa_s, ¶ms)) { + if (wpa_supplicant_trigger_scan(wpa_s, ¶ms, true)) { wpa_printf(MSG_DEBUG, "bgscan learn: Failed to trigger scan"); eloop_register_timeout(data->scan_interval, 0, bgscan_learn_timeout, data, NULL); diff --git a/wpa_supplicant/bgscan_simple.c b/wpa_supplicant/bgscan_simple.c index 41a26df0d63..4dd212b2197 100644 --- a/wpa_supplicant/bgscan_simple.c +++ b/wpa_supplicant/bgscan_simple.c @@ -49,7 +49,7 @@ static void bgscan_simple_timeout(void *eloop_ctx, void *timeout_ctx) */ wpa_printf(MSG_DEBUG, "bgscan simple: Request a background scan"); - if (wpa_supplicant_trigger_scan(wpa_s, ¶ms)) { + if (wpa_supplicant_trigger_scan(wpa_s, ¶ms, true)) { wpa_printf(MSG_DEBUG, "bgscan simple: Failed to trigger scan"); eloop_register_timeout(data->scan_interval, 0, bgscan_simple_timeout, data, NULL); diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c b/wpa_supplicant/dbus/dbus_new_handlers.c index db9f30c9aab..08be9da3b9e 100644 --- a/wpa_supplicant/dbus/dbus_new_handlers.c +++ b/wpa_supplicant/dbus/dbus_new_handlers.c @@ -1371,6 +1371,7 @@ DBusMessage * wpas_dbus_handler_scan(DBusMessage *message, struct wpa_driver_scan_params params; size_t i; dbus_bool_t allow_roam = 1; + bool custom_ies = false; os_memset(¶ms, 0, sizeof(params)); @@ -1397,6 +1398,7 @@ DBusMessage * wpas_dbus_handler_scan(DBusMessage *message, if (wpas_dbus_get_scan_ies(message, &variant_iter, ¶ms, &reply) < 0) goto out; + custom_ies = true; } else if (os_strcmp(key, "Channels") == 0) { if (wpas_dbus_get_scan_channels(message, &variant_iter, ¶ms, &reply) < 0) @@ -1444,7 +1446,7 @@ DBusMessage * wpas_dbus_handler_scan(DBusMessage *message, if (params.freqs && params.freqs[0]) { wpa_s->last_scan_req = MANUAL_SCAN_REQ; if (wpa_supplicant_trigger_scan(wpa_s, - ¶ms)) { + ¶ms, false)) { reply = wpas_dbus_error_scan_error( message, "Scan request rejected"); @@ -1470,7 +1472,7 @@ DBusMessage * wpas_dbus_handler_scan(DBusMessage *message, } wpa_s->last_scan_req = MANUAL_SCAN_REQ; - if (wpa_supplicant_trigger_scan(wpa_s, ¶ms)) { + if (wpa_supplicant_trigger_scan(wpa_s, ¶ms, !custom_ies)) { reply = wpas_dbus_error_scan_error( message, "Scan request rejected"); } diff --git a/wpa_supplicant/rrm.c b/wpa_supplicant/rrm.c index cf107ebaf63..0e079380b73 100644 --- a/wpa_supplicant/rrm.c +++ b/wpa_supplicant/rrm.c @@ -1034,7 +1034,7 @@ static void wpas_rrm_scan_timeout(void *eloop_ctx, void *timeout_ctx) } os_get_reltime(&wpa_s->beacon_rep_scan); if (wpa_s->scanning || wpas_p2p_in_progress(wpa_s) || - wpa_supplicant_trigger_scan(wpa_s, params)) + wpa_supplicant_trigger_scan(wpa_s, params, true)) wpas_rrm_refuse_request(wpa_s); params->duration = prev_duration; } diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c index c53474dae26..94231e417cd 100644 --- 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. * Returns: 0 on success, -1 on failure */ int wpa_supplicant_trigger_scan(struct wpa_supplicant *wpa_s, - struct wpa_driver_scan_params *params) + struct wpa_driver_scan_params *params, + bool default_ies) { struct wpa_driver_scan_params *ctx; + struct wpabuf *ies = NULL; if (wpa_s->scan_work) { wpa_dbg(wpa_s, MSG_INFO, "Reject scan trigger since one is already pending"); return -1; } + if (default_ies) { + if (params->extra_ies_len) { + os_free((u8 *) params->extra_ies); + } + ies = wpa_supplicant_extra_ies(wpa_s); + if (ies) { + params->extra_ies = wpabuf_head(ies); + params->extra_ies_len = wpabuf_len(ies); + } + } ctx = wpa_scan_clone_params(params); + if (ies) { + wpabuf_free(ies); + params->extra_ies = NULL; + params->extra_ies_len = 0; + } if (!ctx || radio_add_work(wpa_s, 0, "scan", 0, wpas_trigger_scan_cb, ctx) < 0) { @@ -566,7 +587,7 @@ void wpa_supplicant_set_default_scan_ies(struct wpa_supplicant *wpa_s) } -static struct wpabuf * wpa_supplicant_extra_ies(struct wpa_supplicant *wpa_s) +struct wpabuf * wpa_supplicant_extra_ies(struct wpa_supplicant *wpa_s) { struct wpabuf *extra_ie = NULL; u8 ext_capab[18]; @@ -1339,7 +1360,7 @@ scan: } #endif /* CONFIG_P2P */ - ret = wpa_supplicant_trigger_scan(wpa_s, scan_params); + ret = wpa_supplicant_trigger_scan(wpa_s, scan_params, false); if (ret && wpa_s->last_scan_req == MANUAL_SCAN_REQ && params.freqs && !wpa_s->manual_scan_freqs) { diff --git a/wpa_supplicant/scan.h b/wpa_supplicant/scan.h index 8eb5c73e275..d1f26eccec3 100644 --- a/wpa_supplicant/scan.h +++ b/wpa_supplicant/scan.h @@ -43,7 +43,8 @@ void wpa_supplicant_notify_scanning(struct wpa_supplicant *wpa_s, int scanning); struct wpa_driver_scan_params; int wpa_supplicant_trigger_scan(struct wpa_supplicant *wpa_s, - struct wpa_driver_scan_params *params); + struct wpa_driver_scan_params *params, + bool default_ies); struct wpa_scan_results * wpa_supplicant_get_scan_results(struct wpa_supplicant *wpa_s, struct scan_info *info, int new_scan); @@ -90,5 +91,6 @@ int wpa_add_scan_freqs_list(struct wpa_supplicant *wpa_s, enum hostapd_hw_mode band, struct wpa_driver_scan_params *params, bool is_6ghz); +struct wpabuf * wpa_supplicant_extra_ies(struct wpa_supplicant *wpa_s); #endif /* SCAN_H */ diff --git a/wpa_supplicant/sme.c b/wpa_supplicant/sme.c index dde80863a98..bf1576ac31f 100644 --- a/wpa_supplicant/sme.c +++ b/wpa_supplicant/sme.c @@ -2543,7 +2543,7 @@ static void sme_obss_scan_timeout(void *eloop_ctx, void *timeout_ctx) params.low_priority = 1; wpa_printf(MSG_DEBUG, "SME OBSS: Request an OBSS scan"); - if (wpa_supplicant_trigger_scan(wpa_s, ¶ms)) + if (wpa_supplicant_trigger_scan(wpa_s, ¶ms, true)) wpa_printf(MSG_DEBUG, "SME OBSS: Failed to trigger scan"); else wpa_s->sme.sched_obss_scan = 1;
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. Signed-off-by: Matthew Wang <matthewmwang@chromium.org> --- doc/dbus.doxygen | 2 +- wpa_supplicant/bgscan_learn.c | 2 +- wpa_supplicant/bgscan_simple.c | 2 +- wpa_supplicant/dbus/dbus_new_handlers.c | 6 ++++-- wpa_supplicant/rrm.c | 2 +- wpa_supplicant/scan.c | 27 ++++++++++++++++++++++--- wpa_supplicant/scan.h | 4 +++- wpa_supplicant/sme.c | 2 +- 8 files changed, 36 insertions(+), 11 deletions(-)