[PATCH BlueZ 1/2] gatt-server: Add support for signed write command

[PATCH BlueZ 1/2] gatt-server: Add support for signed write command

[Frédéric Danis]

GAP/SEC/CSIGN/BV-02-C request the ability to check that signed write has
been performed successfully.
---
 src/shared/gatt-server.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index c7ce3ec1f..0e399ceb1 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -106,6 +106,7 @@ struct bt_gatt_server {
 	unsigned int read_multiple_vl_id;
 	unsigned int prep_write_id;
 	unsigned int exec_write_id;
+	unsigned int signed_write_cmd_id;
 
 	uint8_t min_enc_size;
 
@@ -155,6 +156,7 @@ static void bt_gatt_server_free(struct bt_gatt_server *server)
 	bt_att_unregister(server->att, server->read_multiple_vl_id);
 	bt_att_unregister(server->att, server->prep_write_id);
 	bt_att_unregister(server->att, server->exec_write_id);
+	bt_att_unregister(server->att, server->signed_write_cmd_id);
 
 	queue_destroy(server->prep_queue, prep_write_data_destroy);
 
@@ -777,7 +779,8 @@ static void write_complete_cb(struct gatt_db_attribute *attr, int err,
 	struct bt_gatt_server *server = op->server;
 	uint16_t handle;
 
-	if (op->opcode == BT_ATT_OP_WRITE_CMD) {
+	if (op->opcode == BT_ATT_OP_WRITE_CMD ||
+			op->opcode == BT_ATT_OP_SIGNED_WRITE_CMD) {
 		async_write_op_destroy(op);
 		return;
 	}
@@ -1628,6 +1631,14 @@ static bool gatt_server_register_att_handlers(struct bt_gatt_server *server)
 	if (!server->exec_write_id)
 		return NULL;
 
+	/* Signed Write Command */
+	server->signed_write_cmd_id = bt_att_register(server->att,
+						BT_ATT_OP_SIGNED_WRITE_CMD,
+						write_cb,
+						server, NULL);
+	if (!server->signed_write_cmd_id)
+		return false;
+
 	return true;
 }
 
-- 
2.34.1

[PATCH BlueZ 2/2] device: Update local and remote CSRK on management event

[Frédéric Danis]

The local and remote CSRK keys are only loaded from storage during start.

Those keys should be updated on MGMT_EV_NEW_CSRK event to be able to
perform signed write for GAP/SEC/CSIGN/BV-02-C.
---
 src/adapter.c |  2 ++
 src/device.c  | 16 ++++++++++++++++
 src/device.h  |  2 ++
 3 files changed, 20 insertions(+)

diff --git a/src/adapter.c b/src/adapter.c
index 022390f0d..fb71ef83e 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -8882,6 +8882,8 @@ static void new_csrk_callback(uint16_t index, uint16_t length,
 		return;
 	}
 
+	device_set_csrk(device, key->val, key->type & 0x01);
+
 	if (!ev->store_hint)
 		return;
 
diff --git a/src/device.c b/src/device.c
index 17bcfbc49..34f64ca5b 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1955,6 +1955,22 @@ bool btd_device_get_ltk(struct btd_device *device, uint8_t key[16],
 	return true;
 }
 
+void device_set_csrk(struct btd_device *device, const uint8_t val[16],
+				bool remote)
+{
+	if (remote) {
+		g_free(device->remote_csrk);
+		device->remote_csrk = g_new0(struct csrk_info, 1);
+		memcpy(device->remote_csrk->key, val,
+		       sizeof(device->remote_csrk->key));
+	} else {
+		g_free(device->local_csrk);
+		device->local_csrk = g_new0(struct csrk_info, 1);
+		memcpy(device->local_csrk->key, val,
+		       sizeof(device->local_csrk->key));
+	}
+}
+
 static bool match_sirk(const void *data, const void *match_data)
 {
 	const struct sirk_info *sirk = data;
diff --git a/src/device.h b/src/device.h
index 8bb38669d..d00c002c3 100644
--- a/src/device.h
+++ b/src/device.h
@@ -134,6 +134,8 @@ void device_set_ltk(struct btd_device *device, const uint8_t val[16],
 				bool central, uint8_t enc_size);
 bool btd_device_get_ltk(struct btd_device *device, uint8_t val[16],
 				bool *central, uint8_t *enc_size);
+void device_set_csrk(struct btd_device *device, const uint8_t val[16],
+				bool remote);
 bool btd_device_add_set(struct btd_device *device, bool encrypted,
 				uint8_t sirk[16], uint8_t size, uint8_t rank);
 void device_store_svc_chng_ccc(struct btd_device *device, uint8_t bdaddr_type,
-- 
2.34.1

RE: [BlueZ,1/2] gatt-server: Add support for signed write command

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1705 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=819093

---Test result---

Test Summary:
CheckPatch                    PASS      9.83 seconds
GitLint                       PASS      0.80 seconds
BuildEll                      PASS      24.20 seconds
BluezMake                     PASS      714.71 seconds
MakeCheck                     PASS      12.29 seconds
MakeDistcheck                 PASS      160.48 seconds
CheckValgrind                 PASS      223.80 seconds
CheckSmatch                   WARNING   355.01 seconds
bluezmakeextell               PASS      107.03 seconds
IncrementalBuild              PASS      1321.14 seconds
ScanBuild                     PASS      944.83 seconds

Details
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/gatt-server.c:278:25: warning: Variable length array is used.src/shared/gatt-server.c:621:25: warning: Variable length array is used.src/shared/gatt-server.c:720:25: warning: Variable length array is used.src/shared/gatt-server.c:278:25: warning: Variable length array is used.src/shared/gatt-server.c:621:25: warning: Variable length array is used.src/shared/gatt-server.c:720:25: warning: Variable length array is used.src/shared/gatt-server.c:278:25: warning: Variable length array is used.src/shared/gatt-server.c:621:25: warning: Variable length array is used.src/shared/gatt-server.c:720:25: warning: Variable length array is used.


---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ 2/2] device: Update local and remote CSRK on management event

[Luiz Augusto von Dentz]

Hi Frédéric,

On Tue, Jan 23, 2024 at 7:15 AM Frédéric Danis
<frederic.danis@collabora.com> wrote:
>
> The local and remote CSRK keys are only loaded from storage during start.
>
> Those keys should be updated on MGMT_EV_NEW_CSRK event to be able to
> perform signed write for GAP/SEC/CSIGN/BV-02-C.
> ---
>  src/adapter.c |  2 ++
>  src/device.c  | 16 ++++++++++++++++
>  src/device.h  |  2 ++
>  3 files changed, 20 insertions(+)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 022390f0d..fb71ef83e 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -8882,6 +8882,8 @@ static void new_csrk_callback(uint16_t index, uint16_t length,
>                 return;
>         }
>
> +       device_set_csrk(device, key->val, key->type & 0x01);
> +
>         if (!ev->store_hint)
>                 return;
>
> diff --git a/src/device.c b/src/device.c
> index 17bcfbc49..34f64ca5b 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -1955,6 +1955,22 @@ bool btd_device_get_ltk(struct btd_device *device, uint8_t key[16],
>         return true;
>  }
>
> +void device_set_csrk(struct btd_device *device, const uint8_t val[16],
> +                               bool remote)
> +{
> +       if (remote) {
> +               g_free(device->remote_csrk);
> +               device->remote_csrk = g_new0(struct csrk_info, 1);
> +               memcpy(device->remote_csrk->key, val,
> +                      sizeof(device->remote_csrk->key));
> +       } else {
> +               g_free(device->local_csrk);
> +               device->local_csrk = g_new0(struct csrk_info, 1);
> +               memcpy(device->local_csrk->key, val,
> +                      sizeof(device->local_csrk->key));
> +       }
> +}
> +
>  static bool match_sirk(const void *data, const void *match_data)
>  {
>         const struct sirk_info *sirk = data;
> diff --git a/src/device.h b/src/device.h
> index 8bb38669d..d00c002c3 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -134,6 +134,8 @@ void device_set_ltk(struct btd_device *device, const uint8_t val[16],
>                                 bool central, uint8_t enc_size);
>  bool btd_device_get_ltk(struct btd_device *device, uint8_t val[16],
>                                 bool *central, uint8_t *enc_size);
> +void device_set_csrk(struct btd_device *device, const uint8_t val[16],
> +                               bool remote);

Looks like there is only one use of this function and it is always set
for the remote, actually the fact that this is on the device object
already means it is for the remote so I wonder if we really need to
store the local as well?

>  bool btd_device_add_set(struct btd_device *device, bool encrypted,
>                                 uint8_t sirk[16], uint8_t size, uint8_t rank);
>  void device_store_svc_chng_ccc(struct btd_device *device, uint8_t bdaddr_type,
> --
> 2.34.1
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/2] device: Update local and remote CSRK on management event

[Frédéric Danis]

Hi Luiz,

On 23/01/2024 14:53, Luiz Augusto von Dentz wrote:
> Hi Frédéric,
>
> On Tue, Jan 23, 2024 at 7:15 AM Frédéric Danis
> <frederic.danis@collabora.com> wrote:
>> The local and remote CSRK keys are only loaded from storage during start.
>>
>> Those keys should be updated on MGMT_EV_NEW_CSRK event to be able to
>> perform signed write for GAP/SEC/CSIGN/BV-02-C.
>> ---
>>   src/adapter.c |  2 ++
>>   src/device.c  | 16 ++++++++++++++++
>>   src/device.h  |  2 ++
>>   3 files changed, 20 insertions(+)
>>
>> diff --git a/src/adapter.c b/src/adapter.c
>> index 022390f0d..fb71ef83e 100644
>> --- a/src/adapter.c
>> +++ b/src/adapter.c
>> @@ -8882,6 +8882,8 @@ static void new_csrk_callback(uint16_t index, uint16_t length,
>>                  return;
>>          }
>>
>> +       device_set_csrk(device, key->val, key->type & 0x01);
>> +
>>          if (!ev->store_hint)
>>                  return;
>>
>> diff --git a/src/device.c b/src/device.c
>> index 17bcfbc49..34f64ca5b 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -1955,6 +1955,22 @@ bool btd_device_get_ltk(struct btd_device *device, uint8_t key[16],
>>          return true;
>>   }
>>
>> +void device_set_csrk(struct btd_device *device, const uint8_t val[16],
>> +                               bool remote)
>> +{
>> +       if (remote) {
>> +               g_free(device->remote_csrk);
>> +               device->remote_csrk = g_new0(struct csrk_info, 1);
>> +               memcpy(device->remote_csrk->key, val,
>> +                      sizeof(device->remote_csrk->key));
>> +       } else {
>> +               g_free(device->local_csrk);
>> +               device->local_csrk = g_new0(struct csrk_info, 1);
>> +               memcpy(device->local_csrk->key, val,
>> +                      sizeof(device->local_csrk->key));
>> +       }
>> +}
>> +
>>   static bool match_sirk(const void *data, const void *match_data)
>>   {
>>          const struct sirk_info *sirk = data;
>> diff --git a/src/device.h b/src/device.h
>> index 8bb38669d..d00c002c3 100644
>> --- a/src/device.h
>> +++ b/src/device.h
>> @@ -134,6 +134,8 @@ void device_set_ltk(struct btd_device *device, const uint8_t val[16],
>>                                  bool central, uint8_t enc_size);
>>   bool btd_device_get_ltk(struct btd_device *device, uint8_t val[16],
>>                                  bool *central, uint8_t *enc_size);
>> +void device_set_csrk(struct btd_device *device, const uint8_t val[16],
>> +                               bool remote);
> Looks like there is only one use of this function and it is always set
> for the remote, actually the fact that this is on the device object
> already means it is for the remote so I wonder if we really need to
> store the local as well?

As device is able to store and load both keys from storage I think it 
could be better to keep them in sync, no?

>>   bool btd_device_add_set(struct btd_device *device, bool encrypted,
>>                                  uint8_t sirk[16], uint8_t size, uint8_t rank);
>>   void device_store_svc_chng_ccc(struct btd_device *device, uint8_t bdaddr_type,
>> --
>> 2.34.1
>>
>>
>

-- 
Frédéric Danis
Senior Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, United Kingdom
Registered in England & Wales, no. 5513718

Re: [PATCH BlueZ 2/2] device: Update local and remote CSRK on management event

[Luiz Augusto von Dentz]

Hi Frédéric,

On Tue, Jan 23, 2024 at 10:00 AM Frédéric Danis
<frederic.danis@collabora.com> wrote:
>
> Hi Luiz,
>
> On 23/01/2024 14:53, Luiz Augusto von Dentz wrote:
> > Hi Frédéric,
> >
> > On Tue, Jan 23, 2024 at 7:15 AM Frédéric Danis
> > <frederic.danis@collabora.com> wrote:
> >> The local and remote CSRK keys are only loaded from storage during start.
> >>
> >> Those keys should be updated on MGMT_EV_NEW_CSRK event to be able to
> >> perform signed write for GAP/SEC/CSIGN/BV-02-C.
> >> ---
> >>   src/adapter.c |  2 ++
> >>   src/device.c  | 16 ++++++++++++++++
> >>   src/device.h  |  2 ++
> >>   3 files changed, 20 insertions(+)
> >>
> >> diff --git a/src/adapter.c b/src/adapter.c
> >> index 022390f0d..fb71ef83e 100644
> >> --- a/src/adapter.c
> >> +++ b/src/adapter.c
> >> @@ -8882,6 +8882,8 @@ static void new_csrk_callback(uint16_t index, uint16_t length,
> >>                  return;
> >>          }
> >>
> >> +       device_set_csrk(device, key->val, key->type & 0x01);
> >> +
> >>          if (!ev->store_hint)
> >>                  return;
> >>
> >> diff --git a/src/device.c b/src/device.c
> >> index 17bcfbc49..34f64ca5b 100644
> >> --- a/src/device.c
> >> +++ b/src/device.c
> >> @@ -1955,6 +1955,22 @@ bool btd_device_get_ltk(struct btd_device *device, uint8_t key[16],
> >>          return true;
> >>   }
> >>
> >> +void device_set_csrk(struct btd_device *device, const uint8_t val[16],
> >> +                               bool remote)
> >> +{
> >> +       if (remote) {
> >> +               g_free(device->remote_csrk);
> >> +               device->remote_csrk = g_new0(struct csrk_info, 1);
> >> +               memcpy(device->remote_csrk->key, val,
> >> +                      sizeof(device->remote_csrk->key));
> >> +       } else {
> >> +               g_free(device->local_csrk);
> >> +               device->local_csrk = g_new0(struct csrk_info, 1);
> >> +               memcpy(device->local_csrk->key, val,
> >> +                      sizeof(device->local_csrk->key));
> >> +       }
> >> +}
> >> +
> >>   static bool match_sirk(const void *data, const void *match_data)
> >>   {
> >>          const struct sirk_info *sirk = data;
> >> diff --git a/src/device.h b/src/device.h
> >> index 8bb38669d..d00c002c3 100644
> >> --- a/src/device.h
> >> +++ b/src/device.h
> >> @@ -134,6 +134,8 @@ void device_set_ltk(struct btd_device *device, const uint8_t val[16],
> >>                                  bool central, uint8_t enc_size);
> >>   bool btd_device_get_ltk(struct btd_device *device, uint8_t val[16],
> >>                                  bool *central, uint8_t *enc_size);
> >> +void device_set_csrk(struct btd_device *device, const uint8_t val[16],
> >> +                               bool remote);
> > Looks like there is only one use of this function and it is always set
> > for the remote, actually the fact that this is on the device object
> > already means it is for the remote so I wonder if we really need to
> > store the local as well?
>
> As device is able to store and load both keys from storage I think it
> could be better to keep them in sync, no?

Don't we have the CSRK stored at adapter level though? Or we have to
generate a pair of local/remote CSRK for each device, if we do I'm not
seeing when we store the local CSRK.

> >>   bool btd_device_add_set(struct btd_device *device, bool encrypted,
> >>                                  uint8_t sirk[16], uint8_t size, uint8_t rank);
> >>   void device_store_svc_chng_ccc(struct btd_device *device, uint8_t bdaddr_type,
> >> --
> >> 2.34.1
> >>
> >>
> >
>
> --
> Frédéric Danis
> Senior Software Engineer
>
> Collabora Ltd.
> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, United Kingdom
> Registered in England & Wales, no. 5513718
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/2] device: Update local and remote CSRK on management event

[Frédéric Danis]

Hi Luiz,

On 23/01/2024 16:14, Luiz Augusto von Dentz wrote:
> Hi Frédéric,
>
> On Tue, Jan 23, 2024 at 10:00 AM Frédéric Danis
> <frederic.danis@collabora.com> wrote:
>> Hi Luiz,
>>
>> On 23/01/2024 14:53, Luiz Augusto von Dentz wrote:
>>> Hi Frédéric,
>>>
>>> On Tue, Jan 23, 2024 at 7:15 AM Frédéric Danis
>>> <frederic.danis@collabora.com> wrote:
>>>> The local and remote CSRK keys are only loaded from storage during start.
>>>>
>>>> Those keys should be updated on MGMT_EV_NEW_CSRK event to be able to
>>>> perform signed write for GAP/SEC/CSIGN/BV-02-C.
>>>> ---
>>>>    src/adapter.c |  2 ++
>>>>    src/device.c  | 16 ++++++++++++++++
>>>>    src/device.h  |  2 ++
>>>>    3 files changed, 20 insertions(+)
>>>>
>>>> diff --git a/src/adapter.c b/src/adapter.c
>>>> index 022390f0d..fb71ef83e 100644
>>>> --- a/src/adapter.c
>>>> +++ b/src/adapter.c
>>>> @@ -8882,6 +8882,8 @@ static void new_csrk_callback(uint16_t index, uint16_t length,
>>>>                   return;
>>>>           }
>>>>
>>>> +       device_set_csrk(device, key->val, key->type & 0x01);
>>>> +
>>>>           if (!ev->store_hint)
>>>>                   return;
>>>>
>>>> diff --git a/src/device.c b/src/device.c
>>>> index 17bcfbc49..34f64ca5b 100644
>>>> --- a/src/device.c
>>>> +++ b/src/device.c
>>>> @@ -1955,6 +1955,22 @@ bool btd_device_get_ltk(struct btd_device *device, uint8_t key[16],
>>>>           return true;
>>>>    }
>>>>
>>>> +void device_set_csrk(struct btd_device *device, const uint8_t val[16],
>>>> +                               bool remote)
>>>> +{
>>>> +       if (remote) {
>>>> +               g_free(device->remote_csrk);
>>>> +               device->remote_csrk = g_new0(struct csrk_info, 1);
>>>> +               memcpy(device->remote_csrk->key, val,
>>>> +                      sizeof(device->remote_csrk->key));
>>>> +       } else {
>>>> +               g_free(device->local_csrk);
>>>> +               device->local_csrk = g_new0(struct csrk_info, 1);
>>>> +               memcpy(device->local_csrk->key, val,
>>>> +                      sizeof(device->local_csrk->key));
>>>> +       }
>>>> +}
>>>> +
>>>>    static bool match_sirk(const void *data, const void *match_data)
>>>>    {
>>>>           const struct sirk_info *sirk = data;
>>>> diff --git a/src/device.h b/src/device.h
>>>> index 8bb38669d..d00c002c3 100644
>>>> --- a/src/device.h
>>>> +++ b/src/device.h
>>>> @@ -134,6 +134,8 @@ void device_set_ltk(struct btd_device *device, const uint8_t val[16],
>>>>                                   bool central, uint8_t enc_size);
>>>>    bool btd_device_get_ltk(struct btd_device *device, uint8_t val[16],
>>>>                                   bool *central, uint8_t *enc_size);
>>>> +void device_set_csrk(struct btd_device *device, const uint8_t val[16],
>>>> +                               bool remote);
>>> Looks like there is only one use of this function and it is always set
>>> for the remote, actually the fact that this is on the device object
>>> already means it is for the remote so I wonder if we really need to
>>> store the local as well?
>> As device is able to store and load both keys from storage I think it
>> could be better to keep them in sync, no?
> Don't we have the CSRK stored at adapter level though? Or we have to
> generate a pair of local/remote CSRK for each device, if we do I'm not
> seeing when we store the local CSRK.

Afaiu both CSRK are stored in <adapter>/<device>/info from adapter on 
mgmt event 
(https://github.com/bluez/bluez/blob/master/src/adapter.c#L8888) and 
from device when store_device_info_cb() 
(https://github.com/bluez/bluez/blob/master/src/device.c#L423) is called.

Key and Authenticated values are saved from adapter (with Counter set to 
0), while Key and Counter are saved from device.
Both counter for local and remote are managed in device, so I wonder if 
both CSRK shouldn't be stored only from device.

What's your opinion?

>>>>    bool btd_device_add_set(struct btd_device *device, bool encrypted,
>>>>                                   uint8_t sirk[16], uint8_t size, uint8_t rank);
>>>>    void device_store_svc_chng_ccc(struct btd_device *device, uint8_t bdaddr_type,
>>>> --
>>>> 2.34.1
>>>>
>>>>
>> --
>> Frédéric Danis
>> Senior Software Engineer
>>
>> Collabora Ltd.
>> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, United Kingdom
>> Registered in England & Wales, no. 5513718
>>
>

-- 
Frédéric Danis
Senior Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, United Kingdom
Registered in England & Wales, no. 5513718