Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-332-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
To: linux-bluetooth@vger.kernel.org
Subject: [PATCH BlueZ v2 6/7] shared/bap: Fix possible crash when freeing requests
Date: Fri,  1 Dec 2023 17:38:59 -0500
Message-ID: <20231201223900.2498565-6-luiz.dentz@gmail.com>
In-Reply-To: <20231201223900.2498565-1-luiz.dentz@gmail.com>
References: <20231201223900.2498565-1-luiz.dentz@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Requests maybe queued using a stream so when detaching it needs to be
canceled otherwise they can lead crashes like bellow:

 Invalid read of size 8
    at 0x1C3247: stream_stop_complete (bap.c:1230)
    by 0x1BB2A3: bap_req_complete (bap.c:3863)
    by 0x1C7BB3: bap_req_detach (bap.c:4219)
    by 0x1C7BB3: bt_bap_detach (bap.c:4231)
    by 0x1C7BB3: bt_bap_detach (bap.c:4222)
    by 0x1C7E67: bap_free (bap.c:2937)
    by 0x1C7E67: bt_bap_unref (bap.c:3090)
    by 0x1C7E67: bt_bap_unref (bap.c:3082)
    by 0x18CCC2: test_teardown (test-bap.c:513)
    by 0x1D826A: teardown_callback (tester.c:434)
    by 0x491E4FC: g_idle_dispatch (gmain.c:6163)
    by 0x49224FB: UnknownInlinedFun (gmain.c:3460)
    by 0x49224FB: g_main_context_dispatch (gmain.c:4200)
    by 0x49806B7: g_main_context_iterate.isra.0 (gmain.c:4276)
    by 0x4921AFE: g_main_loop_run (gmain.c:4479)
    by 0x1E8EF4: mainloop_run (mainloop-glib.c:66)
    by 0x1E93F7: mainloop_run_with_signal (mainloop-notify.c:188)
  Address 0x57b9ad0 is 0 bytes inside a block of size 120 free'd
    at 0x4845B2C: free (vg_replace_malloc.c:985)
    by 0x1CBAB7: bap_stream_state_changed (bap.c:1290)
    by 0x1D3104: bap_ep_set_status (bap.c:3673)
    by 0x1DC746: queue_foreach (queue.c:207)
    by 0x1A5320: notify_cb (gatt-client.c:2271)
    by 0x19A1EF: handle_notify (att.c:1012)
    by 0x19A1EF: can_read_data (att.c:1096)
    by 0x1D68CF: watch_callback (io-glib.c:157)
    by 0x49224FB: UnknownInlinedFun (gmain.c:3460)
    by 0x49224FB: g_main_context_dispatch (gmain.c:4200)
    by 0x49806B7: g_main_context_iterate.isra.0 (gmain.c:4276)
    by 0x4921AFE: g_main_loop_run (gmain.c:4479)
    by 0x1E8EF4: mainloop_run (mainloop-glib.c:66)
    by 0x1E93F7: mainloop_run_with_signal (mainloop-notify.c:188)
---
 src/shared/bap.c | 91 ++++++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 41 deletions(-)

diff --git a/src/shared/bap.c b/src/shared/bap.c
index d8a3adde60ca..a1495ca84bcc 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -1232,6 +1232,47 @@ static void stream_stop_complete(struct bt_bap_stream *stream, uint8_t code,
 		bap_stream_io_detach(stream);
 }
 
+static void bap_req_free(void *data)
+{
+	struct bt_bap_req *req = data;
+	size_t i;
+
+	queue_destroy(req->group, bap_req_free);
+
+	for (i = 0; i < req->len; i++)
+		free(req->iov[i].iov_base);
+
+	free(req->iov);
+	free(req);
+}
+
+static void bap_req_complete(struct bt_bap_req *req,
+				const struct bt_ascs_ase_rsp *rsp)
+{
+	struct queue *group;
+
+	if (!req->func)
+		goto done;
+
+	if (rsp)
+		req->func(req->stream, rsp->code, rsp->reason, req->user_data);
+	else
+		req->func(req->stream, BT_ASCS_RSP_UNSPECIFIED, 0x00,
+						req->user_data);
+
+done:
+	/* Detach from request so it can be freed separately */
+	group = req->group;
+	req->group = NULL;
+
+	queue_foreach(group, (queue_foreach_func_t)bap_req_complete,
+							(void *)rsp);
+
+	queue_destroy(group, NULL);
+
+	bap_req_free(req);
+}
+
 static void bap_stream_state_changed(struct bt_bap_stream *stream)
 {
 	struct bt_bap *bap = stream->bap;
@@ -1286,6 +1327,10 @@ static void bap_stream_state_changed(struct bt_bap_stream *stream)
 	/* Post notification updates */
 	switch (stream->ep->state) {
 	case BT_ASCS_ASE_STATE_IDLE:
+		if (bap->req && bap->req->stream == stream) {
+			bap_req_complete(bap->req, NULL);
+			bap->req = NULL;
+		}
 		bap_stream_detach(stream);
 		break;
 	case BT_ASCS_ASE_STATE_QOS:
@@ -2905,20 +2950,6 @@ static void bap_state_free(void *data)
 	free(state);
 }
 
-static void bap_req_free(void *data)
-{
-	struct bt_bap_req *req = data;
-	size_t i;
-
-	queue_destroy(req->group, bap_req_free);
-
-	for (i = 0; i < req->len; i++)
-		free(req->iov[i].iov_base);
-
-	free(req->iov);
-	free(req);
-}
-
 static void bap_detached(void *data, void *user_data)
 {
 	struct bt_bap_cb *cb = data;
@@ -3809,6 +3840,11 @@ static bool bap_send(struct bt_bap *bap, struct bt_bap_req *req)
 
 	DBG(bap, "req %p len %u", req, iov.iov_len);
 
+	if (req->stream && !queue_find(bap->streams, NULL, req->stream)) {
+		DBG(bap, "stream %p detached, aborting op 0x%02x", req->op);
+		return false;
+	}
+
 	if (!gatt_db_attribute_get_char_data(ascs->ase_cp, NULL, &handle,
 						NULL, NULL, NULL)) {
 		DBG(bap, "Unable to find Control Point");
@@ -3843,33 +3879,6 @@ static bool bap_send(struct bt_bap *bap, struct bt_bap_req *req)
 	return true;
 }
 
-static void bap_req_complete(struct bt_bap_req *req,
-				const struct bt_ascs_ase_rsp *rsp)
-{
-	struct queue *group;
-
-	if (!req->func)
-		goto done;
-
-	if (rsp)
-		req->func(req->stream, rsp->code, rsp->reason, req->user_data);
-	else
-		req->func(req->stream, BT_ASCS_RSP_UNSPECIFIED, 0x00,
-						req->user_data);
-
-done:
-	/* Detach from request so it can be freed separately */
-	group = req->group;
-	req->group = NULL;
-
-	queue_foreach(group, (queue_foreach_func_t)bap_req_complete,
-							(void *)rsp);
-
-	queue_destroy(group, NULL);
-
-	bap_req_free(req);
-}
-
 static bool bap_process_queue(void *data)
 {
 	struct bt_bap *bap = data;
-- 
2.43.0