[v4,1/2] Move ownership of MAC address randomization mask to scan params
diff mbox series

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
Related show

Commit Message

Eric Caruso June 27, 2019, 11:22 p.m. UTC
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(-)

Comments

Jouni Malinen Aug. 1, 2019, 3:59 p.m. UTC | #1
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(&params, 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
Eric Caruso Aug. 5, 2019, 10:40 p.m. UTC | #2
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.
Brian Norris Oct. 4, 2019, 12:53 a.m. UTC | #3
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
Brian Norris Oct. 4, 2019, 8:26 p.m. UTC | #4
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

Patch
diff mbox series

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(&params, 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(&params, 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(&params, wpa_s->mac_addr_pno);
 	}
 
 	wpa_scan_set_relative_rssi_params(wpa_s, &params);