[PATCH BlueZ v1] shared/bap: Fix not handling read offset

[PATCH BlueZ v1] shared/bap: Fix not handling read offset

[Luiz Augusto von Dentz]

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

RE: [BlueZ,v1] shared/bap: Fix not handling read offset

[bluez.test.bot]

[-- 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

Re: [PATCH BlueZ v1] shared/bap: Fix not handling read offset

[patchwork-bot+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

Re: [PATCH BlueZ v1] shared/bap: Fix not handling read offset

[Ye He]

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.

Re: [PATCH BlueZ v1] shared/bap: Fix not handling read offset

[Luiz Augusto von Dentz]

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