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

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

[e-mail] April 13, 2024, 11:38 a.m. UTC | #1

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=844243

---Test result---

Test Summary:
CheckPatch                    FAIL      0.62 seconds
GitLint                       FAIL      0.47 seconds
BuildEll                      PASS      24.57 seconds
BluezMake                     PASS      1686.19 seconds
MakeCheck                     PASS      13.47 seconds
MakeDistcheck                 PASS      177.72 seconds
CheckValgrind                 PASS      247.89 seconds
CheckSmatch                   PASS      352.18 seconds
bluezmakeextell               PASS      119.70 seconds
IncrementalBuild              PASS      1507.66 seconds
ScanBuild                     PASS      1014.47 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ] set: don't modify input sirk key in btd_set_add_device()
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#103: 
0x60600001c068 is located 40 bytes inside of 56-byte region [0x60600001c040,0x60600001c078)

/github/workspace/src/src/13628665.patch total: 0 errors, 1 warnings, 40 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/13628665.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.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ] set: don't modify input sirk key in btd_set_add_device()

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
23: B1 Line exceeds max length (91>80): "0x60600001c068 is located 40 bytes inside of 56-byte region [0x60600001c040,0x60600001c078)"


---
Regards,
Linux Bluetooth

[e-mail] April 16, 2024, 3:40 p.m. UTC | #2

Hello:

This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

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!

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);