hog->attr does not own pointer, so it may be invalid when hog object
gets re-attached at bt_hog_attach(). To solve this, this patch
intentionally clears hog->attr at bt_hog_detach() to mark it as invalid
so that it can be repopulated with the valid pointer at bt_hog_attach().
The same applies to all sub-instances.
Sample stack trace:
* thread #1, stop reason = signal SIGSEGV
* frame #0: 0x05ad49f2 bluetoothd`<name omitted> at gatt-db.c:1428
frame #1: 0x05a91922 bluetoothd`bt_hog_attach at hog-lib.c:1694
frame #2: 0x05a9160e bluetoothd`hog_accept at hog.c:212
frame #3: 0x05ab4784 bluetoothd`service_accept at service.c:203
frame #4: 0x05aba1e6 bluetoothd`device_attach_att at device.c:4542
frame #5: 0x05a9c4a2 bluetoothd`connect_cb at gatt-database.c:656
frame #6: 0x05a98e8c bluetoothd`server_cb at btio.c:264
frame #7: 0xec8e6a1a libglib-2.0.so.0`g_main_context_dispatch at gmain.c:3325
frame #8: 0xec8e6c58 libglib-2.0.so.0`g_main_context_iterate at gmain.c:4119
frame #9: 0xec8e6e52 libglib-2.0.so.0`g_main_loop_run at gmain.c:4317
frame #10: 0x05ad582e bluetoothd`mainloop_run at mainloop-glib.c:79
frame #11: 0x05ad5a64 bluetoothd`mainloop_run_with_signal at mainloop-notify.c:201
frame #12: 0x05ac35ac bluetoothd`main at main.c:1103
frame #13: 0xec6ed0a2 libc.so.6`__libc_start_main at libc-start.c:308
---
profiles/input/hog-lib.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
index 1f132aa4c..711bda73c 100644
--- a/profiles/input/hog-lib.c
+++ b/profiles/input/hog-lib.c
@@ -1651,12 +1651,19 @@ static void primary_cb(uint8_t status, GSList *services, void *user_data)
bool bt_hog_attach(struct bt_hog *hog, void *gatt)
{
GSList *l;
+ bt_uuid_t uuid;
if (hog->attrib)
return false;
hog->attrib = g_attrib_ref(gatt);
+ if (!hog->attr && hog->gatt_db) {
+ bt_uuid16_create(&uuid, HOG_UUID16);
+ gatt_db_foreach_service(hog->gatt_db, &uuid,
+ foreach_hog_service, hog);
+ }
+
if (!hog->attr && !hog->primary) {
discover_primary(hog, hog->attrib, NULL, primary_cb, hog);
return true;
@@ -1744,6 +1751,15 @@ void bt_hog_detach(struct bt_hog *hog)
bt_hog_detach(instance);
}
+ /* hog->attr doesn't own pointer, so it may be invalid when this hog
+ * object gets re-attached with bt_hog_attach(). So intentionally mark
+ * it as invalid and remove all instances so that the instances can be
+ * re-attached at bt_hog_attach().
+ */
+ hog->attr = NULL;
+ g_slist_free_full(hog->instances, hog_free);
+ hog->instances = NULL;
+
for (l = hog->reports; l; l = l->next) {
struct report *r = l->data;
--
2.29.2
Hi Sonny,
On Wed, Jan 27, 2021 at 10:43 PM Sonny Sasaka <[email protected]> wrote:
>
> hog->attr does not own pointer, so it may be invalid when hog object
> gets re-attached at bt_hog_attach(). To solve this, this patch
> intentionally clears hog->attr at bt_hog_detach() to mark it as invalid
> so that it can be repopulated with the valid pointer at bt_hog_attach().
> The same applies to all sub-instances.
>
> Sample stack trace:
> * thread #1, stop reason = signal SIGSEGV
> * frame #0: 0x05ad49f2 bluetoothd`<name omitted> at gatt-db.c:1428
> frame #1: 0x05a91922 bluetoothd`bt_hog_attach at hog-lib.c:1694
> frame #2: 0x05a9160e bluetoothd`hog_accept at hog.c:212
> frame #3: 0x05ab4784 bluetoothd`service_accept at service.c:203
> frame #4: 0x05aba1e6 bluetoothd`device_attach_att at device.c:4542
> frame #5: 0x05a9c4a2 bluetoothd`connect_cb at gatt-database.c:656
> frame #6: 0x05a98e8c bluetoothd`server_cb at btio.c:264
> frame #7: 0xec8e6a1a libglib-2.0.so.0`g_main_context_dispatch at gmain.c:3325
> frame #8: 0xec8e6c58 libglib-2.0.so.0`g_main_context_iterate at gmain.c:4119
> frame #9: 0xec8e6e52 libglib-2.0.so.0`g_main_loop_run at gmain.c:4317
> frame #10: 0x05ad582e bluetoothd`mainloop_run at mainloop-glib.c:79
> frame #11: 0x05ad5a64 bluetoothd`mainloop_run_with_signal at mainloop-notify.c:201
> frame #12: 0x05ac35ac bluetoothd`main at main.c:1103
> frame #13: 0xec6ed0a2 libc.so.6`__libc_start_main at libc-start.c:308
>
> ---
> profiles/input/hog-lib.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> index 1f132aa4c..711bda73c 100644
> --- a/profiles/input/hog-lib.c
> +++ b/profiles/input/hog-lib.c
> @@ -1651,12 +1651,19 @@ static void primary_cb(uint8_t status, GSList *services, void *user_data)
> bool bt_hog_attach(struct bt_hog *hog, void *gatt)
> {
> GSList *l;
> + bt_uuid_t uuid;
>
> if (hog->attrib)
> return false;
>
> hog->attrib = g_attrib_ref(gatt);
>
> + if (!hog->attr && hog->gatt_db) {
> + bt_uuid16_create(&uuid, HOG_UUID16);
> + gatt_db_foreach_service(hog->gatt_db, &uuid,
> + foreach_hog_service, hog);
> + }
> +
> if (!hog->attr && !hog->primary) {
> discover_primary(hog, hog->attrib, NULL, primary_cb, hog);
> return true;
> @@ -1744,6 +1751,15 @@ void bt_hog_detach(struct bt_hog *hog)
> bt_hog_detach(instance);
> }
>
> + /* hog->attr doesn't own pointer, so it may be invalid when this hog
> + * object gets re-attached with bt_hog_attach(). So intentionally mark
> + * it as invalid and remove all instances so that the instances can be
> + * re-attached at bt_hog_attach().
> + */
> + hog->attr = NULL;
> + g_slist_free_full(hog->instances, hog_free);
> + hog->instances = NULL;
We can actually what watches instead of cleanup always like this, see:
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/shared/gatt-db.h#n275
> +
> for (l = hog->reports; l; l = l->next) {
> struct report *r = l->data;
>
> --
> 2.29.2
>
--
Luiz Augusto von Dentz
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=423455
---Test result---
##############################
Test: CheckPatch - FAIL
Output:
input/hog: Fix crashes of UAF of hog->attr
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#21:
frame #7: 0xec8e6a1a libglib-2.0.so.0`g_main_context_dispatch at gmain.c:3325
- total: 0 errors, 1 warnings, 34 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.
"[PATCH] input/hog: Fix crashes of UAF of hog->attr" has style problems, please review.
NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED 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: CheckGitLint - FAIL
Output:
input/hog: Fix crashes of UAF of hog->attr
18: B1 Line exceeds max length (81>80): " frame #7: 0xec8e6a1a libglib-2.0.so.0`g_main_context_dispatch at gmain.c:3325"
22: B1 Line exceeds max length (86>80): " frame #11: 0x05ad5a64 bluetoothd`mainloop_run_with_signal at mainloop-notify.c:201"
##############################
Test: CheckBuild - PASS
##############################
Test: MakeCheck - PASS
---
Regards,
Linux Bluetooth