* [PATCH BlueZ v1] shared/bap: Fix not handling read offset
@ 2025-05-27 13:28 Luiz Augusto von Dentz
2025-05-27 14:56 ` [BlueZ,v1] " bluez.test.bot
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2025-05-27 13:28 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Some attributes may need to handle offset other than 0 in case read long
procedure is used, so this properly handles that for PAC_SINK_CHRC_UUID
and PAC_SOURCE_CHRC_UUID.
In addition to PAC record this also uses
gatt_db_attribute_set_fixed_length for attributes that are considered of
fixed size so gatt_db can handle offset directly.
Fixes: https://github.com/bluez/bluez/issues/1294
---
src/shared/bap.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/shared/bap.c b/src/shared/bap.c
index 4bb6e08b5379..f0c6f64855c4 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -516,8 +516,15 @@ static void pacs_sink_read(struct gatt_db_attribute *attrib,
queue_foreach(bdb->sinks, pac_foreach, &iov);
queue_foreach(bdb->broadcast_sinks, pac_foreach, &iov);
- gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base,
- iov.iov_len);
+ if (offset > iov.iov_len) {
+ gatt_db_attribute_read_result(attrib, id,
+ BT_ATT_ERROR_INVALID_OFFSET,
+ NULL, 0);
+ return;
+ }
+
+ gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base + offset,
+ iov.iov_len - offset);
}
static void pacs_sink_loc_read(struct gatt_db_attribute *attrib,
@@ -549,8 +556,15 @@ static void pacs_source_read(struct gatt_db_attribute *attrib,
queue_foreach(bdb->sources, pac_foreach, &iov);
- gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base,
- iov.iov_len);
+ if (offset > iov.iov_len) {
+ gatt_db_attribute_read_result(attrib, id,
+ BT_ATT_ERROR_INVALID_OFFSET,
+ NULL, 0);
+ return;
+ }
+
+ gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base + offset,
+ iov.iov_len - offset);
}
static void pacs_source_loc_read(struct gatt_db_attribute *attrib,
@@ -627,6 +641,7 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
BT_GATT_CHRC_PROP_NOTIFY,
pacs_sink_loc_read, NULL,
pacs);
+ gatt_db_attribute_set_fixed_length(pacs->sink_loc, sizeof(uint32_t));
pacs->sink_loc_ccc = gatt_db_service_add_ccc(pacs->service,
BT_ATT_PERM_READ | BT_ATT_PERM_WRITE);
@@ -649,6 +664,7 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
BT_GATT_CHRC_PROP_NOTIFY,
pacs_source_loc_read, NULL,
pacs);
+ gatt_db_attribute_set_fixed_length(pacs->source_loc, sizeof(uint32_t));
pacs->source_loc_ccc = gatt_db_service_add_ccc(pacs->service,
BT_ATT_PERM_READ | BT_ATT_PERM_WRITE);
@@ -659,6 +675,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
BT_GATT_CHRC_PROP_READ |
BT_GATT_CHRC_PROP_NOTIFY,
pacs_context_read, NULL, pacs);
+ gatt_db_attribute_set_fixed_length(pacs->context,
+ sizeof(struct bt_pacs_context));
pacs->context_ccc = gatt_db_service_add_ccc(pacs->service,
BT_ATT_PERM_READ | BT_ATT_PERM_WRITE);
@@ -671,6 +689,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
BT_GATT_CHRC_PROP_NOTIFY,
pacs_supported_context_read, NULL,
pacs);
+ gatt_db_attribute_set_fixed_length(pacs->supported_context,
+ sizeof(struct bt_pacs_context));
pacs->supported_context_ccc = gatt_db_service_add_ccc(pacs->service,
BT_ATT_PERM_READ | BT_ATT_PERM_WRITE);
--
2.49.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* RE: [BlueZ,v1] shared/bap: Fix not handling read offset
2025-05-27 13:28 [PATCH BlueZ v1] shared/bap: Fix not handling read offset Luiz Augusto von Dentz
@ 2025-05-27 14:56 ` bluez.test.bot
2025-05-27 21:20 ` [PATCH BlueZ v1] " patchwork-bot+bluetooth
2025-05-28 5:21 ` Ye He
2 siblings, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2025-05-27 14:56 UTC (permalink / raw)
To: linux-bluetooth, luiz.dentz
[-- Attachment #1: Type: text/plain, Size: 1864 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=966677
---Test result---
Test Summary:
CheckPatch PENDING 0.27 seconds
GitLint PENDING 0.31 seconds
BuildEll PASS 20.32 seconds
BluezMake PASS 2679.68 seconds
MakeCheck PASS 20.51 seconds
MakeDistcheck PASS 201.13 seconds
CheckValgrind PASS 274.93 seconds
CheckSmatch WARNING 301.75 seconds
bluezmakeextell PASS 128.23 seconds
IncrementalBuild PENDING 0.35 seconds
ScanBuild PASS 914.91 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/bap.c:315:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:315:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:315:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structures
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH BlueZ v1] shared/bap: Fix not handling read offset
2025-05-27 13:28 [PATCH BlueZ v1] shared/bap: Fix not handling read offset Luiz Augusto von Dentz
2025-05-27 14:56 ` [BlueZ,v1] " bluez.test.bot
@ 2025-05-27 21:20 ` patchwork-bot+bluetooth
2025-05-28 5:21 ` Ye He
2 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+bluetooth @ 2025-05-27 21:20 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hello:
This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:
On Tue, 27 May 2025 09:28:31 -0400 you wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Some attributes may need to handle offset other than 0 in case read long
> procedure is used, so this properly handles that for PAC_SINK_CHRC_UUID
> and PAC_SOURCE_CHRC_UUID.
>
> In addition to PAC record this also uses
> gatt_db_attribute_set_fixed_length for attributes that are considered of
> fixed size so gatt_db can handle offset directly.
>
> [...]
Here is the summary with links:
- [BlueZ,v1] shared/bap: Fix not handling read offset
https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=088de118d17c
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH BlueZ v1] shared/bap: Fix not handling read offset
2025-05-27 13:28 [PATCH BlueZ v1] shared/bap: Fix not handling read offset Luiz Augusto von Dentz
2025-05-27 14:56 ` [BlueZ,v1] " bluez.test.bot
2025-05-27 21:20 ` [PATCH BlueZ v1] " patchwork-bot+bluetooth
@ 2025-05-28 5:21 ` Ye He
2025-05-28 15:14 ` Luiz Augusto von Dentz
2 siblings, 1 reply; 5+ messages in thread
From: Ye He @ 2025-05-28 5:21 UTC (permalink / raw)
To: Luiz Augusto von Dentz, linux-bluetooth
Hi Luiz,
> [ EXTERNAL EMAIL ]
>
> From: Luiz Augusto von Dentz<luiz.von.dentz@intel.com>
>
> Some attributes may need to handle offset other than 0 in case read long
> procedure is used, so this properly handles that for PAC_SINK_CHRC_UUID
> and PAC_SOURCE_CHRC_UUID.
>
> In addition to PAC record this also uses
> gatt_db_attribute_set_fixed_length for attributes that are considered of
> fixed size so gatt_db can handle offset directly.
>
> Fixes:https://github.com/bluez/bluez/issues/1294
> ---
> src/shared/bap.c | 28 ++++++++++++++++++++++++----
> 1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 4bb6e08b5379..f0c6f64855c4 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -516,8 +516,15 @@ static void pacs_sink_read(struct gatt_db_attribute *attrib,
> queue_foreach(bdb->sinks, pac_foreach, &iov);
> queue_foreach(bdb->broadcast_sinks, pac_foreach, &iov);
>
> - gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base,
> - iov.iov_len);
> + if (offset > iov.iov_len) {
> + gatt_db_attribute_read_result(attrib, id,
> + BT_ATT_ERROR_INVALID_OFFSET,
> + NULL, 0);
> + return;
> + }
> +
> + gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base + offset,
> + iov.iov_len - offset);
> }
>
> static void pacs_sink_loc_read(struct gatt_db_attribute *attrib,
> @@ -549,8 +556,15 @@ static void pacs_source_read(struct gatt_db_attribute *attrib,
>
> queue_foreach(bdb->sources, pac_foreach, &iov);
>
> - gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base,
> - iov.iov_len);
> + if (offset > iov.iov_len) {
> + gatt_db_attribute_read_result(attrib, id,
> + BT_ATT_ERROR_INVALID_OFFSET,
> + NULL, 0);
> + return;
> + }
> +
> + gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base + offset,
> + iov.iov_len - offset);
> }
>
> static void pacs_source_loc_read(struct gatt_db_attribute *attrib,
> @@ -627,6 +641,7 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
> BT_GATT_CHRC_PROP_NOTIFY,
> pacs_sink_loc_read, NULL,
> pacs);
> + gatt_db_attribute_set_fixed_length(pacs->sink_loc, sizeof(uint32_t));
>
> pacs->sink_loc_ccc = gatt_db_service_add_ccc(pacs->service,
> BT_ATT_PERM_READ | BT_ATT_PERM_WRITE);
> @@ -649,6 +664,7 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
> BT_GATT_CHRC_PROP_NOTIFY,
> pacs_source_loc_read, NULL,
> pacs);
> + gatt_db_attribute_set_fixed_length(pacs->source_loc, sizeof(uint32_t));
>
> pacs->source_loc_ccc = gatt_db_service_add_ccc(pacs->service,
> BT_ATT_PERM_READ | BT_ATT_PERM_WRITE);
> @@ -659,6 +675,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
> BT_GATT_CHRC_PROP_READ |
> BT_GATT_CHRC_PROP_NOTIFY,
> pacs_context_read, NULL, pacs);
> + gatt_db_attribute_set_fixed_length(pacs->context,
> + sizeof(struct bt_pacs_context));
>
> pacs->context_ccc = gatt_db_service_add_ccc(pacs->service,
> BT_ATT_PERM_READ | BT_ATT_PERM_WRITE);
> @@ -671,6 +689,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
> BT_GATT_CHRC_PROP_NOTIFY,
> pacs_supported_context_read, NULL,
> pacs);
> + gatt_db_attribute_set_fixed_length(pacs->supported_context,
> + sizeof(struct bt_pacs_context));
>
> pacs->supported_context_ccc = gatt_db_service_add_ccc(pacs->service,
> BT_ATT_PERM_READ | BT_ATT_PERM_WRITE);
> --
> 2.49.0
>
>
I am concerned about this solution. It can indeed solve the problem of
using blob procedure to read sink pacs or source pacs, but if blob
procedure is used to read other services, will the problem occur again?
I think we should fix the issue at its root.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH BlueZ v1] shared/bap: Fix not handling read offset
2025-05-28 5:21 ` Ye He
@ 2025-05-28 15:14 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2025-05-28 15:14 UTC (permalink / raw)
To: Ye He; +Cc: linux-bluetooth
Hi,
On Wed, May 28, 2025 at 1:21 AM Ye He <ye.he@amlogic.com> wrote:
>
> Hi Luiz,
>
> > [ EXTERNAL EMAIL ]
> >
> > From: Luiz Augusto von Dentz<luiz.von.dentz@intel.com>
> >
> > Some attributes may need to handle offset other than 0 in case read long
> > procedure is used, so this properly handles that for PAC_SINK_CHRC_UUID
> > and PAC_SOURCE_CHRC_UUID.
> >
> > In addition to PAC record this also uses
> > gatt_db_attribute_set_fixed_length for attributes that are considered of
> > fixed size so gatt_db can handle offset directly.
> >
> > Fixes:https://github.com/bluez/bluez/issues/1294
> > ---
> > src/shared/bap.c | 28 ++++++++++++++++++++++++----
> > 1 file changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > index 4bb6e08b5379..f0c6f64855c4 100644
> > --- a/src/shared/bap.c
> > +++ b/src/shared/bap.c
> > @@ -516,8 +516,15 @@ static void pacs_sink_read(struct gatt_db_attribute *attrib,
> > queue_foreach(bdb->sinks, pac_foreach, &iov);
> > queue_foreach(bdb->broadcast_sinks, pac_foreach, &iov);
> >
> > - gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base,
> > - iov.iov_len);
> > + if (offset > iov.iov_len) {
> > + gatt_db_attribute_read_result(attrib, id,
> > + BT_ATT_ERROR_INVALID_OFFSET,
> > + NULL, 0);
> > + return;
> > + }
> > +
> > + gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base + offset,
> > + iov.iov_len - offset);
> > }
> >
> > static void pacs_sink_loc_read(struct gatt_db_attribute *attrib,
> > @@ -549,8 +556,15 @@ static void pacs_source_read(struct gatt_db_attribute *attrib,
> >
> > queue_foreach(bdb->sources, pac_foreach, &iov);
> >
> > - gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base,
> > - iov.iov_len);
> > + if (offset > iov.iov_len) {
> > + gatt_db_attribute_read_result(attrib, id,
> > + BT_ATT_ERROR_INVALID_OFFSET,
> > + NULL, 0);
> > + return;
> > + }
> > +
> > + gatt_db_attribute_read_result(attrib, id, 0, iov.iov_base + offset,
> > + iov.iov_len - offset);
> > }
> >
> > static void pacs_source_loc_read(struct gatt_db_attribute *attrib,
> > @@ -627,6 +641,7 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
> > BT_GATT_CHRC_PROP_NOTIFY,
> > pacs_sink_loc_read, NULL,
> > pacs);
> > + gatt_db_attribute_set_fixed_length(pacs->sink_loc, sizeof(uint32_t));
> >
> > pacs->sink_loc_ccc = gatt_db_service_add_ccc(pacs->service,
> > BT_ATT_PERM_READ | BT_ATT_PERM_WRITE);
> > @@ -649,6 +664,7 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
> > BT_GATT_CHRC_PROP_NOTIFY,
> > pacs_source_loc_read, NULL,
> > pacs);
> > + gatt_db_attribute_set_fixed_length(pacs->source_loc, sizeof(uint32_t));
> >
> > pacs->source_loc_ccc = gatt_db_service_add_ccc(pacs->service,
> > BT_ATT_PERM_READ | BT_ATT_PERM_WRITE);
> > @@ -659,6 +675,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
> > BT_GATT_CHRC_PROP_READ |
> > BT_GATT_CHRC_PROP_NOTIFY,
> > pacs_context_read, NULL, pacs);
> > + gatt_db_attribute_set_fixed_length(pacs->context,
> > + sizeof(struct bt_pacs_context));
> >
> > pacs->context_ccc = gatt_db_service_add_ccc(pacs->service,
> > BT_ATT_PERM_READ | BT_ATT_PERM_WRITE);
> > @@ -671,6 +689,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
> > BT_GATT_CHRC_PROP_NOTIFY,
> > pacs_supported_context_read, NULL,
> > pacs);
> > + gatt_db_attribute_set_fixed_length(pacs->supported_context,
> > + sizeof(struct bt_pacs_context));
> >
> > pacs->supported_context_ccc = gatt_db_service_add_ccc(pacs->service,
> > BT_ATT_PERM_READ | BT_ATT_PERM_WRITE);
> > --
> > 2.49.0
> >
> >
> I am concerned about this solution. It can indeed solve the problem of
> using blob procedure to read sink pacs or source pacs, but if blob
> procedure is used to read other services, will the problem occur again?
> I think we should fix the issue at its root.
Well Ive already explained that doesn't work since the offset is
already part of the interface, so both internal and external attribute
are expected to return that data following the offset:
https://github.com/bluez/bluez/blob/master/doc/org.bluez.GattCharacteristic.rst#arraybyte-readvaluedict-options
Also note that although normally the offset means the offset of data
read with a read long operation I wouldn't be surprised if the vendor
does use for something else, e.g. as control point index or something
like that.
What is perhaps more efficient is to cache the read operations so we
can detect when it is a read long operation taking place we just use
the cache to respond, that said Read Blob that doesn't continue from
previous offset shall still be sent to the callback to respond.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-05-28 15:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-27 13:28 [PATCH BlueZ v1] shared/bap: Fix not handling read offset Luiz Augusto von Dentz
2025-05-27 14:56 ` [BlueZ,v1] " bluez.test.bot
2025-05-27 21:20 ` [PATCH BlueZ v1] " patchwork-bot+bluetooth
2025-05-28 5:21 ` Ye He
2025-05-28 15:14 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox