| Message ID | 1561677722-48395-1-git-send-email-ejcaruso@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4,1/2] Move ownership of MAC address randomization mask to scan params | expand |
On Thu, Jun 27, 2019 at 04:22:01PM -0700, Eric Caruso wrote: > This array can be freed either from the scan parameters or from > clearing the MAC address randomization parameters from the > wpa_supplicant struct. To make this ownership more clear, we have > each struct own its own copy of the parameters. This breaks random address use (e.g., results in failure in the hwsim test case scan_random_mac) and opens a memory leak. > diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c > +static int wpa_setup_mac_addr_rand_params(struct wpa_driver_scan_params *params, > + tmp = os_malloc(2 * ETH_ALEN); > + params->mac_addr = tmp; That allocation is lost in some paths. > @@ -169,7 +195,9 @@ static void wpas_trigger_scan_cb(struct wpa_radio_work *work, int deinit) > return; > } > > - if (wpas_update_random_addr_disassoc(wpa_s) < 0) { > + if (wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_SCAN) > + wpa_setup_mac_addr_rand_params(params, wpa_s->mac_addr_scan); > + else if (wpas_update_random_addr_disassoc(wpa_s) < 0) { What is this trying to do and why? The commit message seems to imply that this should not have any changes in behavior, but this looks like a potential change. > if ((wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_SCAN) && > wpa_s->wpa_state <= WPA_SCANNING) { > - params.mac_addr_rand = 1; > - if (wpa_s->mac_addr_scan) { > - params.mac_addr = wpa_s->mac_addr_scan; > - params.mac_addr_mask = wpa_s->mac_addr_scan + ETH_ALEN; > - } > + wpa_setup_mac_addr_rand_params(¶ms, wpa_s->mac_addr_scan); > } This can result in params.mac_addr pointing to allocated memory that does not get cleared when wpa_supplicant_scan() returns. I can fix this easily, but I could not find easily why this patch breaks the test case I use to confirm that scanning uses a random MAC address. As such, I cannot apply this or the patch 2/2 since it depends on this patch. The test failure: START scan_random_mac 1/1 Exception: Real address used to transmit Probe Request frame FAIL scan_random_mac 0.221036 2019-08-01 16:15:53.359258
For the memory issues -- it looks like other functions that declare the params on the stack free whatever they are working with by themselves. Would you rather see this also done for the mac_addr member, or factoring the cleanup minus the freeing of the whole params struct into a separate function (which will then mean the wpa_scan_free_params function just calls this cleanup function, and then os_free on the struct itself)? > > @@ -169,7 +195,9 @@ static void wpas_trigger_scan_cb(struct wpa_radio_work *work, int deinit) > > return; > > } > > > > - if (wpas_update_random_addr_disassoc(wpa_s) < 0) { > > + if (wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_SCAN) > > + wpa_setup_mac_addr_rand_params(params, wpa_s->mac_addr_scan); > > + else if (wpas_update_random_addr_disassoc(wpa_s) < 0) { > > What is this trying to do and why? The commit message seems to imply > that this should not have any changes in behavior, but this looks like a > potential change. Both of these are employing MAC address randomization in different ways. I can make it so we try both here, but as far as I can tell, since this implementation asks the driver to randomize the address per-probe then if it is active it will take priority over the existing implementation anyway. Now that you point this out, though, we could set this bit even if the driver or module doesn't actually support the hardware random MAC implementation, so it's probably worth doing both here instead. I am not sure if this is why the hwsim test breaks but I will investigate.
This is a very dragged-out thread, and I'm not intimately familiar with all of this but... On Mon, Aug 5, 2019 at 3:40 PM Eric Caruso <ejcaruso@chromium.org> wrote: > > > @@ -169,7 +195,9 @@ static void wpas_trigger_scan_cb(struct wpa_radio_work *work, int deinit) > > > return; > > > } > > > > > > - if (wpas_update_random_addr_disassoc(wpa_s) < 0) { > > > + if (wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_SCAN) > > > + wpa_setup_mac_addr_rand_params(params, wpa_s->mac_addr_scan); > > > + else if (wpas_update_random_addr_disassoc(wpa_s) < 0) { > > > > What is this trying to do and why? The commit message seems to imply > > that this should not have any changes in behavior, but this looks like a > > potential change. We've been using this version of the patch in some cases for a bit now, but I'm now running into problems that I think trace back to here: these two lines are not checking for wpa_state, and so we call this even when we're already associated -- and that totally breaks, as the kernel tells us (rightly so) that's not supported. I think this hunk needs to die. > Both of these are employing MAC address randomization in different > ways. I can make it so we try both here, but as far as I can tell, I'm not really sure what you mean by "both of these". But in case this is what you're talking about: wpas_update_random_addr_disassoc() isn't actually related to MAC address randomization for scanning AFAICT. > since this implementation asks the driver to randomize the address > per-probe then if it is active it will take priority over the existing The 'mac_addr_rand_enable' feature flag is not per-probe -- it's a sticky setting in wpa_s, and it only means that *if we're not associated*, we should use a random address in probes. And the parameter setup is done properly earlier, in wpa_supplicant_scan() -- I don't think you need to repeat (and override) it here. > implementation anyway. Now that you point this out, though, we could > set this bit even if the driver or module doesn't actually support the > hardware random MAC implementation, so it's probably worth doing both > here instead. > > I am not sure if this is why the hwsim test breaks but I will investigate. I'm not sure which tests are failing, but I would not be surprised if the stuff I pointed out (closely aligned with Jouni's complaint) is breaking things. Brian
On Thu, Oct 3, 2019 at 5:53 PM Brian Norris <briannorris@chromium.org> wrote: > On Mon, Aug 5, 2019 at 3:40 PM Eric Caruso <ejcaruso@chromium.org> wrote: > > > > @@ -169,7 +195,9 @@ static void wpas_trigger_scan_cb(struct wpa_radio_work *work, int deinit) > > > > return; > > > > } > > > > > > > > - if (wpas_update_random_addr_disassoc(wpa_s) < 0) { > > > > + if (wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_SCAN) > > > > + wpa_setup_mac_addr_rand_params(params, wpa_s->mac_addr_scan); > > > > + else if (wpas_update_random_addr_disassoc(wpa_s) < 0) { > > > > > > What is this trying to do and why? The commit message seems to imply > > > that this should not have any changes in behavior, but this looks like a > > > potential change. ... > I think this hunk needs to die. Local testing shows that killing it is not sufficient, in local testing. I haven't quite figured out exactly why though... ...but we definitely should at least be checking for "wpa_s->wpa_state < WPA_AUTHENTICATING", at least. > > Both of these are employing MAC address randomization in different > > ways. I can make it so we try both here, but as far as I can tell, > > I'm not really sure what you mean by "both of these". But in case this > is what you're talking about: wpas_update_random_addr_disassoc() isn't > actually related to MAC address randomization for scanning AFAICT. And I was wrong here: wpas_update_random_addr_disassoc() does deploy a similar feature, based off the 'preassoc_mac_addr' config setting. So I guess that's what you meant by "both of these." > > since this implementation asks the driver to randomize the address > > per-probe then if it is active it will take priority over the existing > > The 'mac_addr_rand_enable' feature flag is not per-probe -- it's a > sticky setting in wpa_s, and it only means that *if we're not > associated*, we should use a random address in probes. And the > parameter setup is done properly earlier, in wpa_supplicant_scan() -- > I don't think you need to repeat (and override) it here. > > > implementation anyway. Now that you point this out, though, we could > > set this bit even if the driver or module doesn't actually support the > > hardware random MAC implementation, so it's probably worth doing both > > here instead. Yes, the "else if" seems a little off -- we should probably give the option of doing both. Brian
diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c index 7abb028dd..08ded3fdb 100644 --- a/wpa_supplicant/scan.c +++ b/wpa_supplicant/scan.c @@ -79,6 +79,32 @@ static int wpas_wps_in_use(struct wpa_supplicant *wpa_s, #endif /* CONFIG_WPS */ +static int wpa_setup_mac_addr_rand_params(struct wpa_driver_scan_params *params, + const u8 *mac_addr) +{ + u8 *tmp; + + if (!mac_addr) + return 0; + + if (params->mac_addr) { + os_free((u8 *) params->mac_addr); + params->mac_addr = NULL; + } + + params->mac_addr_rand = 1; + + tmp = os_malloc(2 * ETH_ALEN); + if (!tmp) + return -1; + + os_memcpy(tmp, mac_addr, 2 * ETH_ALEN); + params->mac_addr = tmp; + params->mac_addr_mask = tmp + ETH_ALEN; + return 0; +} + + /** * wpa_supplicant_enabled_networks - Check whether there are enabled networks * @wpa_s: Pointer to wpa_supplicant data @@ -169,7 +195,9 @@ static void wpas_trigger_scan_cb(struct wpa_radio_work *work, int deinit) return; } - if (wpas_update_random_addr_disassoc(wpa_s) < 0) { + if (wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_SCAN) + wpa_setup_mac_addr_rand_params(params, wpa_s->mac_addr_scan); + else if (wpas_update_random_addr_disassoc(wpa_s) < 0) { wpa_msg(wpa_s, MSG_INFO, "Failed to assign random MAC address for a scan"); wpa_scan_free_params(params); @@ -1212,11 +1240,7 @@ ssid_list_set: if ((wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_SCAN) && wpa_s->wpa_state <= WPA_SCANNING) { - params.mac_addr_rand = 1; - if (wpa_s->mac_addr_scan) { - params.mac_addr = wpa_s->mac_addr_scan; - params.mac_addr_mask = wpa_s->mac_addr_scan + ETH_ALEN; - } + wpa_setup_mac_addr_rand_params(¶ms, wpa_s->mac_addr_scan); } if (!is_zero_ether_addr(wpa_s->next_scan_bssid)) { @@ -1665,12 +1689,7 @@ scan: if ((wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_SCHED_SCAN) && wpa_s->wpa_state <= WPA_SCANNING) { - params.mac_addr_rand = 1; - if (wpa_s->mac_addr_sched_scan) { - params.mac_addr = wpa_s->mac_addr_sched_scan; - params.mac_addr_mask = wpa_s->mac_addr_sched_scan + - ETH_ALEN; - } + wpa_setup_mac_addr_rand_params(¶ms, wpa_s->mac_addr_sched_scan); } wpa_scan_set_relative_rssi_params(wpa_s, scan_params); @@ -2535,23 +2554,9 @@ wpa_scan_clone_params(const struct wpa_driver_scan_params *src) params->sched_scan_plans_num = src->sched_scan_plans_num; } - if (src->mac_addr_rand) { - params->mac_addr_rand = src->mac_addr_rand; - - if (src->mac_addr && src->mac_addr_mask) { - u8 *mac_addr; - - mac_addr = os_malloc(2 * ETH_ALEN); - if (!mac_addr) - goto failed; - - os_memcpy(mac_addr, src->mac_addr, ETH_ALEN); - os_memcpy(mac_addr + ETH_ALEN, src->mac_addr_mask, - ETH_ALEN); - params->mac_addr = mac_addr; - params->mac_addr_mask = mac_addr + ETH_ALEN; - } - } + if (src->mac_addr_rand && + wpa_setup_mac_addr_rand_params(params, (const u8 *)src->mac_addr)) + goto failed; if (src->bssid) { u8 *bssid; @@ -2739,11 +2744,7 @@ int wpas_start_pno(struct wpa_supplicant *wpa_s) if ((wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_PNO) && wpa_s->wpa_state <= WPA_SCANNING) { - params.mac_addr_rand = 1; - if (wpa_s->mac_addr_pno) { - params.mac_addr = wpa_s->mac_addr_pno; - params.mac_addr_mask = wpa_s->mac_addr_pno + ETH_ALEN; - } + wpa_setup_mac_addr_rand_params(¶ms, wpa_s->mac_addr_pno); } wpa_scan_set_relative_rssi_params(wpa_s, ¶ms);
This array can be freed either from the scan parameters or from clearing the MAC address randomization parameters from the wpa_supplicant struct. To make this ownership more clear, we have each struct own its own copy of the parameters. Signed-off-by: Eric Caruso <ejcaruso@chromium.org> --- wpa_supplicant/scan.c | 69 ++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 34 deletions(-)