diff mbox series

[BlueZ] set: don't modify input sirk key in btd_set_add_device()

Message ID b468e20424d271295c84c72059952acaf65968a5.1713002640.git.pav@iki.fi
State New
Headers show
Series [BlueZ] set: don't modify input sirk key in btd_set_add_device() | expand

Commit Message

Pauli Virtanen April 13, 2024, 10:04 a.m. UTC 
Currently, btd_set_add_device decrypts the sirk in-place, modifying the
key passed to it.

This causes store_sirk() later on to save the wrong (decrypted) key
value, resulting to invalid duplicate device set.

It also allows devices->sirk list to contain same set multiple times,
which crashes later on as sirks-set are assumed to be 1-to-1 in
btd_set_add/remove_device().

Fixes:
=======================================================================
ERROR: AddressSanitizer: heap-use-after-free on address 0x60600001c068
READ of size 8 at 0x60600001c068 thread T0
    #0 0x762721 in btd_set_remove_device src/set.c:347
    #1 0x7341e7 in remove_sirk_info src/device.c:7145
    #2 0x7f2cee in queue_foreach src/shared/queue.c:207
    #3 0x734499 in btd_device_unref src/device.c:7159
    #4 0x719f65 in device_remove src/device.c:4788
    #5 0x682382 in adapter_remove src/adapter.c:6959
    ...
0x60600001c068 is located 40 bytes inside of 56-byte region [0x60600001c040,0x60600001c078)
freed by thread T0 here:
    #1 0x7605a6 in set_free src/set.c:170
    #2 0x7d4eff in remove_interface gdbus/object.c:660
    #3 0x7dcbb3 in g_dbus_unregister_interface gdbus/object.c:1394
    #4 0x762990 in btd_set_remove_device src/set.c:362
    #5 0x7341e7 in remove_sirk_info src/device.c:7145
    #6 0x7f2cee in queue_foreach src/shared/queue.c:207
    #7 0x734499 in btd_device_unref src/device.c:7159
    #8 0x719f65 in device_remove src/device.c:4788
    #9 0x682382 in adapter_remove src/adapter.c:6959
    ...
previously allocated by thread T0 here:
    #1 0x7f5429 in util_malloc src/shared/util.c:46
    #2 0x7605f1 in set_new src/set.c:178
    #3 0x7625b9 in btd_set_add_device src/set.c:324
    #4 0x6f8fc8 in add_set src/device.c:1916
    #5 0x7f2cee in queue_foreach src/shared/queue.c:207
    #6 0x6f982c in device_set_ltk src/device.c:1940
    #7 0x667b97 in load_ltks src/adapter.c:4478
    ...
=======================================================================
---
 src/set.c | 10 +++++++---
 src/set.h |  3 ++-
 2 files changed, 9 insertions(+), 4 deletions(-)

Comments

patchwork-bot+bluetooth@kernel.org April 16, 2024, 3:40 p.m. UTC | #1 
Hello:

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

On Sat, 13 Apr 2024 13:04:26 +0300 you wrote:
> Currently, btd_set_add_device decrypts the sirk in-place, modifying the
> key passed to it.
> 
> This causes store_sirk() later on to save the wrong (decrypted) key
> value, resulting to invalid duplicate device set.
> 
> It also allows devices->sirk list to contain same set multiple times,
> which crashes later on as sirks-set are assumed to be 1-to-1 in
> btd_set_add/remove_device().
> 
> [...]

Here is the summary with links:
  - [BlueZ] set: don't modify input sirk key in btd_set_add_device()
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=b411b98bf4f5

You are awesome, thank you!
diff mbox series

Patch

diff --git a/src/set.c b/src/set.c
index ad64fb163..bf35ee403 100644
--- a/src/set.c
+++ b/src/set.c
@@ -171,7 +171,7 @@  static void set_free(void *data)
 }
 
 static struct btd_device_set *set_new(struct btd_device *device,
-					uint8_t sirk[16], uint8_t size)
+					const uint8_t sirk[16], uint8_t size)
 {
 	struct btd_device_set *set;
 
@@ -206,7 +206,7 @@  static struct btd_device_set *set_new(struct btd_device *device,
 }
 
 static struct btd_device_set *set_find(struct btd_device *device,
-						uint8_t sirk[16])
+						const uint8_t sirk[16])
 {
 	struct btd_adapter *adapter = device_get_adapter(device);
 	const struct queue_entry *entry;
@@ -295,10 +295,14 @@  static void foreach_device(struct btd_device *device, void *data)
 }
 
 struct btd_device_set *btd_set_add_device(struct btd_device *device,
-						uint8_t *key, uint8_t sirk[16],
+						const uint8_t *key,
+						const uint8_t sirk_value[16],
 						uint8_t size)
 {
 	struct btd_device_set *set;
+	uint8_t sirk[16];
+
+	memcpy(sirk, sirk_value, sizeof(sirk));
 
 	/* In case key has been set it means SIRK is encrypted */
 	if (key) {
diff --git a/src/set.h b/src/set.h
index 67177e8c7..2307218c4 100644
--- a/src/set.h
+++ b/src/set.h
@@ -13,7 +13,8 @@ 
 struct btd_device_set;
 
 struct btd_device_set *btd_set_add_device(struct btd_device *device,
-						uint8_t *ltk, uint8_t sirk[16],
+						const uint8_t *ltk,
+						const uint8_t sirk[16],
 						uint8_t size);
 bool btd_set_remove_device(struct btd_device_set *set,
 						struct btd_device *device);