[PATCH 1/4] frame-xchg: Don't call frame_xchg_destroy directly

[PATCH 1/4] frame-xchg: Don't call frame_xchg_destroy directly

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

frame_xchg_destroy is passed as the wiphy radio work's destroy callback
to wiphy.c.  If it's also called directly in frame_xchg_exit, there's
going to be a use-after-free when it's called again from wiphy_exit, so
instead use wiphy_radio_work_done which will call frame_xchg_destroy and
forget the frame_xchg record.
---
 src/frame-xchg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/frame-xchg.c b/src/frame-xchg.c
index 1ba2d907..f07a4d8d 100644
--- a/src/frame-xchg.c
+++ b/src/frame-xchg.c
@@ -1337,7 +1337,7 @@ static void destroy_xchg_data(void *user_data)
 {
 	struct frame_xchg_data *fx = user_data;
 
-	frame_xchg_destroy(&fx->work);
+	wiphy_radio_work_done(wiphy_find_by_wdev(fx->wdev_id), fx->work.id);
 }
 
 static void frame_xchg_exit(void)
@@ -1359,3 +1359,4 @@ static void frame_xchg_exit(void)
 }
 
 IWD_MODULE(frame_xchg, frame_xchg_init, frame_xchg_exit);
+IWD_MODULE_DEPENDS(frame_xchg, wiphy)
-- 
2.25.1

[PATCH 2/4] p2p: Update call after frame-xchg changes

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 558 bytes --]

---
 src/p2p.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/p2p.c b/src/p2p.c
index 92e3c65d..58027fc6 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -3186,7 +3186,7 @@ static void p2p_device_send_probe_resp(struct p2p_device *dev,
 
 	freq = scan_channel_to_freq(dev->listen_channel, SCAN_BAND_2_4_GHZ);
 	frame_xchg_start(dev->wdev_id, iov, freq, 0, 0, false, 0,
-				p2p_probe_resp_done, dev, NULL);
+				p2p_probe_resp_done, dev, NULL, NULL);
 	l_debug("Probe Response tx queued");
 
 	l_free(p2p_ie);
-- 
2.25.1

[PATCH 3/4] frame-xchg: Fix group removal inside frame callback

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1720 bytes --]

When a frame registered in a given group Id triggers a callback and that
callback ends up calling frame_watch_group_remove for that group Id,
that call will happen inside WATCHLIST_NOTIFY_MATCHES and will free the
memory used by the watchlist.  watchlist.h has protection against the
watchlist being "destroyed" inside WATCHLIST_NOTIFY_MATCHES, but not
against its memory being freed -- the memory where it stores the in_notify
and destroy_pending flags.  Free the group immediately after
WATCHLIST_NOTIFY_MATCHES to avoid reads/writes to those flags triggering
valgrind warnings.
---
 src/frame-xchg.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/frame-xchg.c b/src/frame-xchg.c
index f07a4d8d..53bda4b4 100644
--- a/src/frame-xchg.c
+++ b/src/frame-xchg.c
@@ -231,6 +231,10 @@ static void frame_watch_unicast_notify(struct l_genl_msg *msg, void *user_data)
 	WATCHLIST_NOTIFY_MATCHES(&group->watches, frame_watch_match_prefix,
 					&info, frame_watch_cb_t, mpdu,
 					info.body, info.body_len, rssi);
+
+	/* Has frame_watch_group_destroy been called inside a frame CB? */
+	if (group->watches.pending_destroy)
+		l_free(group);
 }
 
 static void frame_watch_group_destroy(void *data)
@@ -244,7 +248,16 @@ static void frame_watch_group_destroy(void *data)
 	l_io_destroy(group->io);
 	l_queue_destroy(group->write_queue,
 			(l_queue_destroy_func_t) l_genl_msg_unref);
+
+	/*
+	 * We may be inside a frame notification but even then use
+	 * watchlist_destroy to prevent any more notifications from
+	 * being dispatched.
+	 */
 	watchlist_destroy(&group->watches);
+	if (group->watches.in_notify)
+		return;
+
 	l_free(group);
 }
 
-- 
2.25.1

[PATCH 4/4] frame-xchg: Drop the BSSID check for incoming frames

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1541 bytes --]

The BSSID (address_3) in response frames was being checked to be the
same as in the request frame, or all-zeros for faulty drivers.  At least
one Wi-Fi Display device sends a GO Negotiation Response with the BSSID
different from its Device Address (by 1 bit) and I didn't see an easy
way to obtain that address beforhand so we can "whitelist" it for this
check, so just drop that check for now.

ANQP didn't have this check before it started using frame-xchg so it
shouldn't be critical.
---
 src/frame-xchg.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/src/frame-xchg.c b/src/frame-xchg.c
index 53bda4b4..3c1fc279 100644
--- a/src/frame-xchg.c
+++ b/src/frame-xchg.c
@@ -992,14 +992,11 @@ static bool frame_xchg_resp_handle(const struct mmpdu_header *mpdu,
 		return false;
 
 	/*
-	 * Is the received frame's BSSID same as the transmitted frame's
-	 * BSSID, may have to be moved to the user callback if there are
-	 * usages where this is false.  Some drivers (brcmfmac) can't
-	 * report the BSSID so check for all-zeros too.
+	 * BSSID (address_3) check not practical because some Linux
+	 * drivers report all-zero values and some remote devices send
+	 * wrong addresses.  But the frame callback is free to perform
+	 * its own check.
 	 */
-	if (memcmp(mpdu->address_3, fx->tx_mpdu->address_3, 6) &&
-			!util_mem_is_zero(mpdu->address_3, 6))
-		return false;
 
 	for (entry = l_queue_get_entries(fx->rx_watches);
 			entry; entry = entry->next) {
-- 
2.25.1

Re: [PATCH 1/4] frame-xchg: Don't call frame_xchg_destroy directly

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

Hi Andrew,

On 7/20/20 7:45 PM, Andrew Zaborowski wrote:
> frame_xchg_destroy is passed as the wiphy radio work's destroy callback
> to wiphy.c.  If it's also called directly in frame_xchg_exit, there's
> going to be a use-after-free when it's called again from wiphy_exit, so
> instead use wiphy_radio_work_done which will call frame_xchg_destroy and
> forget the frame_xchg record.
> ---
>   src/frame-xchg.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 

All applied, thanks.

Regards,
-Denis