[PATCH v3 1/5] shared/bap: Fix not detaching streams when PAC is removed

[PATCH v3 1/5] shared/bap: Fix not detaching streams when PAC is removed

[Luiz Augusto von Dentz]

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

When local PAC is removed we attempt to release the streams but we left
it still attached to the endpoint, so this makes sure the stream is
properly detached by setting its state to idle.

Fixes: https://github.com/bluez/bluez/issues/457
---
 src/shared/bap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/shared/bap.c b/src/shared/bap.c
index db7def7999b7..4ba65cbaa8f9 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -2478,8 +2478,10 @@ static void remove_streams(void *data, void *user_data)
 	struct bt_bap_stream *stream;
 
 	stream = queue_remove_if(bap->streams, match_stream_lpac, pac);
-	if (stream)
+	if (stream) {
 		bt_bap_stream_release(stream, NULL, NULL);
+		stream_set_state(stream, BT_BAP_STREAM_STATE_IDLE);
+	}
 }
 
 bool bt_bap_remove_pac(struct bt_bap_pac *pac)
-- 
2.37.3

[PATCH v3 2/5] bap: Fix not setting stream to NULL

[Luiz Augusto von Dentz]

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

If the stream state is idle the ep->stream shall be set to NULL
otherwise it may be reused causing the following trace:

==32623==ERROR: AddressSanitizer: heap-use-after-free on address ...
 READ of size 8 at 0x60b000103550 thread T0
    #0 0x7bf7b7 in bap_stream_valid src/shared/bap.c:4065
    #1 0x7bf981 in bt_bap_stream_config src/shared/bap.c:4082
    #2 0x51a7c8 in bap_config profiles/audio/bap.c:584
    #3 0x71b907 in queue_foreach src/shared/queue.c:207
    #4 0x51b61f in select_cb profiles/audio/bap.c:626
    #5 0x4691ed in pac_select_cb profiles/audio/media.c:884
    #6 0x4657ea in endpoint_reply profiles/audio/media.c:369

Fixes: https://github.com/bluez/bluez/issues/457#issuecomment-1399232486
---
 profiles/audio/bap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index ae944b617bb4..8f24117681d2 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -998,9 +998,10 @@ static void bap_state(struct bt_bap_stream *stream, uint8_t old_state,
 	switch (new_state) {
 	case BT_BAP_STREAM_STATE_IDLE:
 		/* Release stream if idle */
-		if (ep)
+		if (ep) {
 			bap_io_close(ep);
-		else
+			ep->stream = NULL;
+		} else
 			queue_remove(data->streams, stream);
 		break;
 	case BT_BAP_STREAM_STATE_CONFIG:
-- 
2.37.3

[PATCH v3 3/5] bap: Fix not removing endpoint if local PAC is unregistered

[Luiz Augusto von Dentz]

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

If local PAC is unregistered it would also notify via pac_removed
callback which shall unregister the endpoint D-Bus object.

Fixes: https://github.com/bluez/bluez/issues/457#issuecomment-1402178691
---
 profiles/audio/bap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index 8f24117681d2..5a50a2cc6105 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -1049,12 +1049,12 @@ static void pac_added(struct bt_bap_pac *pac, void *user_data)
 	bt_bap_foreach_pac(data->bap, BT_BAP_SINK, pac_found, service);
 }
 
-static bool ep_match_rpac(const void *data, const void *match_data)
+static bool ep_match_pac(const void *data, const void *match_data)
 {
 	const struct bap_ep *ep = data;
 	const struct bt_bap_pac *pac = match_data;
 
-	return ep->rpac == pac;
+	return ep->rpac == pac || ep->lpac == pac;
 }
 
 static void pac_removed(struct bt_bap_pac *pac, void *user_data)
@@ -1082,7 +1082,7 @@ static void pac_removed(struct bt_bap_pac *pac, void *user_data)
 		return;
 	}
 
-	ep = queue_remove_if(queue, ep_match_rpac, pac);
+	ep = queue_remove_if(queue, ep_match_pac, pac);
 	if (!ep)
 		return;
 
-- 
2.37.3

[PATCH v3 4/5] bap: Fix not checking if request fits when grouping

[Luiz Augusto von Dentz]

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

When grouping requests with the same opcode the code was queueing them
without attempt to check that that would fit in the ATT MTU causing the
following trace:

stack-buffer-overflow on address 0x7fffdba951f0 at pc 0x7fc15fc49d21 bp
0x7fffdba95020 sp 0x7fffdba947d0
WRITE of size 9 at 0x7fffdba951f0 thread T0
   #0 0x7fc15fc49d20 in __interceptor_memcpy
(/lib64/libasan.so.8+0x49d20)
   #1 0x71f698 in util_iov_push_mem src/shared/util.c:266
   #2 0x7b9312 in append_group src/shared/bap.c:3424
   #3 0x71ba01 in queue_foreach src/shared/queue.c:207
   #4 0x7b9b66 in bap_send src/shared/bap.c:3459
   #5 0x7ba594 in bap_process_queue src/shared/bap.c:351

Fixes: https://github.com/bluez/bluez/issues/457#issuecomment-1403924708
---
 src/shared/bap.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 4ba65cbaa8f9..22f2e67146fb 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -3425,20 +3425,34 @@ static void append_group(void *data, void *user_data)
 					req->iov[i].iov_base);
 }
 
+static uint16_t bap_req_len(struct bt_bap_req *req)
+{
+	uint16_t len = 0;
+	size_t i;
+	const struct queue_entry *e;
+
+	for (i = 0; i < req->len; i++)
+		len += req->iov[i].iov_len;
+
+	e = queue_get_entries(req->group);
+	for (; e; e = e->next)
+		len += bap_req_len(e->data);
+
+	return len;
+}
+
 static bool bap_send(struct bt_bap *bap, struct bt_bap_req *req)
 {
 	struct bt_ascs *ascs = bap_get_ascs(bap);
 	int ret;
 	uint16_t handle;
-	uint8_t buf[64];
 	struct bt_ascs_ase_hdr hdr;
-	struct iovec iov  = {
-		.iov_base = buf,
-		.iov_len = 0,
-	};
+	struct iovec iov;
 	size_t i;
 
-	DBG(bap, "req %p", req);
+	iov.iov_len = sizeof(hdr) + bap_req_len(req);
+
+	DBG(bap, "req %p len %u", req, iov.iov_len);
 
 	if (!gatt_db_attribute_get_char_data(ascs->ase_cp, NULL, &handle,
 						NULL, NULL, NULL)) {
@@ -3446,6 +3460,9 @@ static bool bap_send(struct bt_bap *bap, struct bt_bap_req *req)
 		return false;
 	}
 
+	iov.iov_base = alloca(iov.iov_len);
+	iov.iov_len = 0;
+
 	hdr.op = req->op;
 	hdr.num = 1 + queue_length(req->group);
 
@@ -3531,9 +3548,19 @@ static bool bap_queue_req(struct bt_bap *bap, struct bt_bap_req *req)
 {
 	struct bt_bap_req *pend;
 	struct queue *queue;
+	struct bt_att *att = bt_bap_get_att(bap);
+	uint16_t mtu = bt_att_get_mtu(att);
+	uint16_t len = 2 + bap_req_len(req);
+
+	if (len > mtu) {
+		DBG(bap, "Unable to queue request: req len %u > %u mtu", len,
+									mtu);
+		return false;
+	}
 
 	pend = queue_find(bap->reqs, match_req, req);
-	if (pend) {
+	/* Check if req can be grouped together and it fits in the MTU */
+	if (pend && (bap_req_len(pend) + len < mtu)) {
 		if (!pend->group)
 			pend->group = queue_new();
 		/* Group requests with the same opcode */
-- 
2.37.3

[PATCH v3 5/5] bap: Fix registering multiple endpoint for the same PAC set

[Luiz Augusto von Dentz]

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

This makes sure there is only one endpoint representing a local and
remote PAC set.
---
 profiles/audio/bap.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index 5a50a2cc6105..e5ffb7230580 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -510,6 +510,22 @@ static void ep_free(void *data)
 	free(ep);
 }
 
+struct match_ep {
+	struct bt_bap_pac *lpac;
+	struct bt_bap_pac *rpac;
+};
+
+static bool match_ep(const void *data, const void *user_data)
+{
+	const struct bap_ep *ep = data;
+	const struct match_ep *match = user_data;
+
+	if (ep->lpac != match->lpac)
+		return false;
+
+	return ep->rpac == match->rpac;
+}
+
 static struct bap_ep *ep_register(struct btd_service *service,
 					struct bt_bap_pac *lpac,
 					struct bt_bap_pac *rpac)
@@ -520,6 +536,7 @@ static struct bap_ep *ep_register(struct btd_service *service,
 	struct queue *queue;
 	int i, err;
 	const char *suffix;
+	struct match_ep match = { lpac, rpac };
 
 	switch (bt_bap_pac_get_type(rpac)) {
 	case BT_BAP_SINK:
@@ -536,6 +553,10 @@ static struct bap_ep *ep_register(struct btd_service *service,
 		return NULL;
 	}
 
+	ep = queue_find(queue, match_ep, &match);
+	if (ep)
+		return ep;
+
 	ep = new0(struct bap_ep, 1);
 	ep->data = data;
 	ep->lpac = lpac;
-- 
2.37.3

RE: [v3,1/5] shared/bap: Fix not detaching streams when PAC is removed

[bluez.test.bot]

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

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=715998

---Test result---

Test Summary:
CheckPatch                    FAIL      2.59 seconds
GitLint                       PASS      1.37 seconds
BuildEll                      PASS      28.29 seconds
BluezMake                     PASS      884.46 seconds
MakeCheck                     PASS      11.06 seconds
MakeDistcheck                 PASS      151.11 seconds
CheckValgrind                 PASS      249.55 seconds
CheckSmatch                   PASS      331.28 seconds
bluezmakeextell               PASS      99.03 seconds
IncrementalBuild              PASS      3699.68 seconds
ScanBuild                     PASS      1031.10 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v3,4/5] bap: Fix not checking if request fits when grouping
WARNING:REPEATED_WORD: Possible repeated word: 'that'
#86: 
without attempt to check that that would fit in the ATT MTU causing the

/github/workspace/src/src/13117701.patch total: 0 errors, 1 warnings, 69 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13117701.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.




---
Regards,
Linux Bluetooth

Re: [PATCH v3 1/5] shared/bap: Fix not detaching streams when PAC is removed

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Thu, 26 Jan 2023 12:12:38 -0800 you wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> When local PAC is removed we attempt to release the streams but we left
> it still attached to the endpoint, so this makes sure the stream is
> properly detached by setting its state to idle.
> 
> Fixes: https://github.com/bluez/bluez/issues/457
> 
> [...]

Here is the summary with links:
  - [v3,1/5] shared/bap: Fix not detaching streams when PAC is removed
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=d159973ecec8
  - [v3,2/5] bap: Fix not setting stream to NULL
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=414c8650acfe
  - [v3,3/5] bap: Fix not removing endpoint if local PAC is unregistered
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=8aed9db4b13f
  - [v3,4/5] bap: Fix not checking if request fits when grouping
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=814e3535a1bc
  - [v3,5/5] bap: Fix registering multiple endpoint for the same PAC set
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=b61044d52917

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html