On Thu, May 30, 2019 at 11:10:46AM -0700, Eric Caruso wrote:
> Add D-Bus property:
> * MACAddressRandomizationMask: a{say}
>
> which configure random MAC address functionality in the Wi-Fi
> driver via netlink. This also fixes weird pointer ownership
> that was causing memory issues.
It would be nice to get bit more detailed description of that pointer
ownership issue and ideally, have it separated into its own patch if it
is indeed fixing something that already exists and not something that
is only exposed with the changes here.
> diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c
> @@ -9626,59 +9626,10 @@ static int wpas_ctrl_iface_mac_rand_scan(struct wpa_supplicant *wpa_s,
> - if (!enable) {
> - wpas_mac_addr_rand_scan_clear(wpa_s, type);
> - if (wpa_s->pno) {
> - if (type & MAC_ADDR_RAND_PNO) {
> - wpas_stop_pno(wpa_s);
> - wpas_start_pno(wpa_s);
> - }
> - } else if (wpa_s->sched_scanning &&
> - (type & MAC_ADDR_RAND_SCHED_SCAN)) {
> - wpas_scan_restart_sched_scan(wpa_s);
> - }
> - return 0;
> - }
> + if (!enable)
> + return wpas_disable_mac_addr_randomization(wpa_s, type);
..
This part about moving this functionality into a helper function sounds
reasonable, but should be done without functional changes and in its own
patch. I applied this now with the changed behavior removed in this
commit:
https://w1.fi/cgit/hostap/commit/?id=91b6eba7732354ed3dfe0aa9715dc4c0746e3336
> - if (type & MAC_ADDR_RAND_SCHED_SCAN) {
> - if (wpas_mac_addr_rand_scan_set(wpa_s, MAC_ADDR_RAND_SCHED_SCAN,
> - addr, mask))
> - return -1;
The changes were to reintroduce some of the return -1 statements that
had been removed in your patch without any clear explanation of why they
were removed. If such changes are needed, please provide a separate
patch with those changes and a commit log that explains the need.
> 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,
> + const u8 *mac_addr)
> +{
> + u8 *tmp;
> +
> + if (!mac_addr)
> + return 0;
> +
> + 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;
> +}
This would result in memory leaks since that allocated heap memory is
not freed anywhere. I'd assume this part is related to that "weird
pointer ownership". It should really be handled in a separate patch and
with clear explanation of why it is needed and changes to clearly
describe how this new allocated buffer can be safely freed.
I'm attaching the patch for the remaining changes that I did not yet
apply due to that memory leak and hope of getting it split into two
patches. This has some coding style cleanup compared to the version you
sent to get rid of a compiler warnings and to be a bit more consistent
with the coding style used in hostap.git.
--
Jouni Malinen PGP id EFC895FA
>From a4f6ec4dacfa08496ed9901999cbc069d35c81a3 Mon Sep 17 00:00:00 2001
From: Eric Caruso <ejcaruso@xxxxxxxxxxxx>
Date: Thu, 30 May 2019 11:10:46 -0700
Subject: [PATCH] Add MAC address randomization functionality
Add D-Bus property:
* MACAddressRandomizationMask: a{say}
which configure random MAC address functionality in the Wi-Fi
driver via netlink. This also fixes weird pointer ownership
that was causing memory issues.
Signed-off-by: Eric Caruso <ejcaruso@xxxxxxxxxxxx>
---
doc/dbus.doxygen | 5 +
wpa_supplicant/dbus/dbus_new.c | 6 +
wpa_supplicant/dbus/dbus_new_handlers.c | 154 ++++++++++++++++++++++++
wpa_supplicant/dbus/dbus_new_handlers.h | 2 +
wpa_supplicant/scan.c | 90 ++++++++------
wpa_supplicant/scan.h | 2 +
6 files changed, 225 insertions(+), 34 deletions(-)
diff --git a/doc/dbus.doxygen b/doc/dbus.doxygen
index 072ed3486..9f6af52b1 100644
--- a/doc/dbus.doxygen
+++ b/doc/dbus.doxygen
@@ -1043,6 +1043,11 @@ fi.w1.wpa_supplicant1.CreateInterface.
<h3>WpsPriority - s - (read/write)</h3>
<p>Priority for the networks added through WPS</p>
</li>
+
+ <li>
+ <h3>MACAddressRandomizationMask - a{say} - (read/write)</h3>
+ <p>Masks to show which bits not to randomize with MAC address randomization. Possible keys are "scan", "sched_scan", and "pno". Values must be an array of 6 bytes.</p>
+ </li>
</ul>
\subsection dbus_interface_signals Signals
diff --git a/wpa_supplicant/dbus/dbus_new.c b/wpa_supplicant/dbus/dbus_new.c
index fc2fc2ef1..f277c04fb 100644
--- a/wpa_supplicant/dbus/dbus_new.c
+++ b/wpa_supplicant/dbus/dbus_new.c
@@ -3803,6 +3803,12 @@ static const struct wpa_dbus_property_desc wpas_dbus_interface_properties[] = {
NULL,
NULL
},
+ { "MACAddressRandomizationMask", WPAS_DBUS_NEW_IFACE_INTERFACE,
+ "a{say}",
+ wpas_dbus_getter_mac_address_randomization_mask,
+ wpas_dbus_setter_mac_address_randomization_mask,
+ NULL
+ },
{ NULL, NULL, NULL, NULL, NULL, NULL }
};
diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c b/wpa_supplicant/dbus/dbus_new_handlers.c
index 6c36d91a0..42c663c9a 100644
--- a/wpa_supplicant/dbus/dbus_new_handlers.c
+++ b/wpa_supplicant/dbus/dbus_new_handlers.c
@@ -3989,6 +3989,160 @@ out:
}
+/**
+ * wpas_dbus_setter_mac_address_randomization_mask - Set masks used for
+ * MAC address randomization
+ * @iter: Pointer to incoming dbus message iter
+ * @error: Location to store error on failure
+ * @user_data: Function specific data
+ * Returns: TRUE on success, FALSE on failure
+ *
+ * Setter for "MACAddressRandomizationMask" property.
+ */
+dbus_bool_t wpas_dbus_setter_mac_address_randomization_mask(
+ const struct wpa_dbus_property_desc *property_desc,
+ DBusMessageIter *iter, DBusError *error, void *user_data)
+{
+ struct wpa_supplicant *wpa_s = user_data;
+ DBusMessageIter variant_iter, dict_iter, entry_iter, array_iter;
+ const char *key;
+ unsigned int rand_type = 0;
+ const u8 *mask;
+ int mask_len;
+ unsigned int rand_types_to_disable = MAC_ADDR_RAND_ALL;
+
+ dbus_message_iter_recurse(iter, &variant_iter);
+ dbus_message_iter_recurse(&variant_iter, &dict_iter);
+ while (dbus_message_iter_get_arg_type(&dict_iter) ==
+ DBUS_TYPE_DICT_ENTRY) {
+ dbus_message_iter_recurse(&dict_iter, &entry_iter);
+ if (dbus_message_iter_get_arg_type(&entry_iter) != DBUS_TYPE_STRING) {
+ dbus_set_error(error, DBUS_ERROR_FAILED,
+ "%s: key not a string", __func__);
+ return FALSE;
+ }
+ dbus_message_iter_get_basic(&entry_iter, &key);
+ dbus_message_iter_next(&entry_iter);
+ if (dbus_message_iter_get_arg_type(&entry_iter) != DBUS_TYPE_ARRAY ||
+ dbus_message_iter_get_element_type(&entry_iter) != DBUS_TYPE_BYTE) {
+ dbus_set_error(error, DBUS_ERROR_FAILED,
+ "%s: mask was not a byte array", __func__);
+ return FALSE;
+ }
+ dbus_message_iter_recurse(&entry_iter, &array_iter);
+ dbus_message_iter_get_fixed_array(&array_iter, &mask, &mask_len);
+
+ if (os_strcmp(key, "scan") == 0) {
+ rand_type = MAC_ADDR_RAND_SCAN;
+ } else if (os_strcmp(key, "sched_scan") == 0) {
+ rand_type = MAC_ADDR_RAND_SCHED_SCAN;
+ } else if (os_strcmp(key, "pno") == 0) {
+ rand_type = MAC_ADDR_RAND_PNO;
+ } else {
+ dbus_set_error(error, DBUS_ERROR_FAILED,
+ "%s: bad scan type \"%s\"", __func__, key);
+ return FALSE;
+ }
+
+ if (mask_len != ETH_ALEN) {
+ dbus_set_error(error, DBUS_ERROR_FAILED,
+ "%s: malformed MAC mask given", __func__);
+ return FALSE;
+ }
+
+ if (wpas_enable_mac_addr_randomization(
+ wpa_s, rand_type, wpa_s->perm_addr, mask)) {
+ dbus_set_error(error, DBUS_ERROR_FAILED,
+ "%s: failed to set up MAC address randomization for %s",
+ __func__, key);
+ return FALSE;
+ }
+
+ wpa_printf(MSG_DEBUG, "%s: Enabled MAC address randomization for %s with mask: "
+ MACSTR, wpa_s->ifname, key, MAC2STR(mask));
+ rand_types_to_disable &= ~rand_type;
+ dbus_message_iter_next(&dict_iter);
+ }
+
+ if (rand_types_to_disable &&
+ wpas_disable_mac_addr_randomization(wpa_s, rand_types_to_disable)) {
+ dbus_set_error(error, DBUS_ERROR_FAILED,
+ "%s: failed to disable MAC address randomization",
+ __func__);
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
+
+dbus_bool_t wpas_dbus_getter_mac_address_randomization_mask(
+ const struct wpa_dbus_property_desc *property_desc,
+ DBusMessageIter *iter, DBusError *error, void *user_data)
+{
+ struct wpa_supplicant *wpa_s = user_data;
+ DBusMessageIter variant_iter, dict_iter, entry_iter, array_iter;
+ unsigned int i;
+ u8 mask_buf[ETH_ALEN];
+ /* Read docs on dbus_message_iter_append_fixed_array for why this
+ * is necessary... */
+ u8 *mask = mask_buf;
+ static const struct {
+ const char *key;
+ unsigned int type;
+ } types[] = {
+ { "scan", MAC_ADDR_RAND_SCAN },
+ { "sched_scan", MAC_ADDR_RAND_SCHED_SCAN },
+ { "pno", MAC_ADDR_RAND_PNO }
+ };
+
+ if (!dbus_message_iter_open_container(iter, DBUS_TYPE_VARIANT,
+ "a{say}", &variant_iter) ||
+ !dbus_message_iter_open_container(&variant_iter, DBUS_TYPE_ARRAY,
+ "{say}", &dict_iter)) {
+ dbus_set_error_const(error, DBUS_ERROR_NO_MEMORY, "no memory");
+ return FALSE;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(types); i++) {
+ if (wpas_mac_addr_rand_scan_get_mask(wpa_s, types[i].type,
+ mask))
+ continue;
+
+ if (!dbus_message_iter_open_container(&dict_iter,
+ DBUS_TYPE_DICT_ENTRY,
+ NULL, &entry_iter) ||
+ !dbus_message_iter_append_basic(&entry_iter,
+ DBUS_TYPE_STRING,
+ &types[i].key) ||
+ !dbus_message_iter_open_container(&entry_iter,
+ DBUS_TYPE_ARRAY,
+ DBUS_TYPE_BYTE_AS_STRING,
+ &array_iter) ||
+ !dbus_message_iter_append_fixed_array(&array_iter,
+ DBUS_TYPE_BYTE,
+ &mask,
+ ETH_ALEN) ||
+ !dbus_message_iter_close_container(&entry_iter,
+ &array_iter) ||
+ !dbus_message_iter_close_container(&dict_iter,
+ &entry_iter)) {
+ dbus_set_error_const(error, DBUS_ERROR_NO_MEMORY,
+ "no memory");
+ return FALSE;
+ }
+ }
+
+ if (!dbus_message_iter_close_container(&variant_iter, &dict_iter) ||
+ !dbus_message_iter_close_container(iter, &variant_iter)) {
+ dbus_set_error_const(error, DBUS_ERROR_NO_MEMORY, "no memory");
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
+
/**
* wpas_dbus_getter_sta_address - Return the address of a connected station
* @iter: Pointer to incoming dbus message iter
diff --git a/wpa_supplicant/dbus/dbus_new_handlers.h b/wpa_supplicant/dbus/dbus_new_handlers.h
index d922ce1b4..afa26efed 100644
--- a/wpa_supplicant/dbus/dbus_new_handlers.h
+++ b/wpa_supplicant/dbus/dbus_new_handlers.h
@@ -177,6 +177,8 @@ DECLARE_ACCESSOR(wpas_dbus_getter_pkcs11_engine_path);
DECLARE_ACCESSOR(wpas_dbus_getter_pkcs11_module_path);
DECLARE_ACCESSOR(wpas_dbus_getter_blobs);
DECLARE_ACCESSOR(wpas_dbus_getter_stas);
+DECLARE_ACCESSOR(wpas_dbus_getter_mac_address_randomization_mask);
+DECLARE_ACCESSOR(wpas_dbus_setter_mac_address_randomization_mask);
DECLARE_ACCESSOR(wpas_dbus_getter_sta_address);
DECLARE_ACCESSOR(wpas_dbus_getter_sta_aid);
DECLARE_ACCESSOR(wpas_dbus_getter_sta_caps);
diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c
index 7abb028dd..251e0642e 100644
--- a/wpa_supplicant/scan.c
+++ b/wpa_supplicant/scan.c
@@ -79,6 +79,27 @@ 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;
+
+ 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 +190,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 +1235,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 +1684,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 +2549,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 +2739,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);
@@ -2843,6 +2839,32 @@ int wpas_mac_addr_rand_scan_set(struct wpa_supplicant *wpa_s,
}
+int wpas_mac_addr_rand_scan_get_mask(struct wpa_supplicant *wpa_s,
+ unsigned int type, u8 *mask)
+{
+ u8 *to_copy = NULL;
+
+ if ((wpa_s->mac_addr_rand_enable & type) != type)
+ return -1;
+
+ if (type == MAC_ADDR_RAND_SCAN) {
+ to_copy = wpa_s->mac_addr_scan;
+ } else if (type == MAC_ADDR_RAND_SCHED_SCAN) {
+ to_copy = wpa_s->mac_addr_sched_scan;
+ } else if (type == MAC_ADDR_RAND_PNO) {
+ to_copy = wpa_s->mac_addr_pno;
+ } else {
+ wpa_printf(MSG_DEBUG,
+ "scan: Invalid MAC randomization type=0x%x",
+ type);
+ return -1;
+ }
+
+ os_memcpy(mask, to_copy + ETH_ALEN, ETH_ALEN);
+ return 0;
+}
+
+
int wpas_abort_ongoing_scan(struct wpa_supplicant *wpa_s)
{
struct wpa_radio_work *work;
diff --git a/wpa_supplicant/scan.h b/wpa_supplicant/scan.h
index 2aa0a8be0..58caa7818 100644
--- a/wpa_supplicant/scan.h
+++ b/wpa_supplicant/scan.h
@@ -52,6 +52,8 @@ void wpas_mac_addr_rand_scan_clear(struct wpa_supplicant *wpa_s,
int wpas_mac_addr_rand_scan_set(struct wpa_supplicant *wpa_s,
unsigned int type, const u8 *addr,
const u8 *mask);
+int wpas_mac_addr_rand_scan_get_mask(struct wpa_supplicant *wpa_s,
+ unsigned int type, u8 *mask);
int wpas_abort_ongoing_scan(struct wpa_supplicant *wpa_s);
void filter_scan_res(struct wpa_supplicant *wpa_s,
struct wpa_scan_results *res);
--
2.20.1
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
