[bionic]blutoothd segfault when you cancel the keyboard pairing during the dialog for pairing code

Bug #1887910 reported by Alex Tu on 2020-07-17
14
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OEM Priority Project
Critical
Alex Tu
bluez (Ubuntu)
High
Unassigned
Bionic
High
Alex Tu
Eoan
High
Unassigned
Focal
High
Unassigned
Groovy
High
Unassigned

Bug Description

[Impact]

This patch is for this issue:
steps:
1. pair bluetooth keyboard
2. see the dialog asking user input the code for pairing.
3. press "esc" to cancel it.
4. blutoothd segfault shows in dmesg after a while.
5. Bluetooth shows off on setting UI of right top corner. dmesg shows: [ 978.138593] bluetoothd[1569]: segfault at 0 ip 000055564abe0a06 sp 00007ffe4bec6410 error 4 in bluetoothd[55564ab77000+f3000]

[Test Case]

 1. pair bluetooth keyboard
 2. see the dialog asking user input the code for pairing.
 3. press "esc" to cancel it.
 4. the bluetooth should still work to pair another bluetooth device.

[Regression Potential]

 * This patch workaround the case that a queue node was created but not yet assigned function before user input pairing keycode. If the user cancel the paring before inputting pairing keycode then assign the function pointer a dummy 'direct_match'.

* Bluetoothd responses to Bluetooth functions and "queue" is a shared common data structure, so in case of regression happens then blutoothd systemd service would be crashed.

 * We can verify this by operating add/remove BT devices to trigger queue operations.

 * I verified on target machine BIOS ID:0983 on BT mouse, keyboard, headset on pairing, remove and functionality checking.

[Other Info]

 * NO.

Alex Tu (alextu) wrote :

after this patch, the issue can not be reproduced:
commit 24249e091ab3a935f6ea87b7f7355c689c045a80 (HEAD -> refs/heads/master)
Author: Luiz Augusto von Dentz <email address hidden>
Date: Mon Apr 9 14:48:41 2018 +0300

    shared/queue: Handle NULL as direct match on queue_remove_if

    As with queue_find when function is set to NULL use direct_match as
    callback.

diff --git a/src/shared/queue.c b/src/shared/queue.c
index 5ddb8326d..60df11143 100644
--- a/src/shared/queue.c
+++ b/src/shared/queue.c
@@ -280,9 +280,12 @@ void *queue_remove_if(struct queue *queue, queue_match_func_t function,
 {
        struct queue_entry *entry, *prev = NULL;

- if (!queue || !function)
+ if (!queue)
                return NULL;

+ if (!function)
+ function = direct_match;
+
        entry = queue->head;

Changed in oem-priority:
assignee: nobody → Alex Tu (alextu)
importance: Undecided → Critical
status: New → In Progress
tags: added: oem-priority originate-from-1886187 timbuktu
Daniel van Vugt (vanvugt) wrote :

It's good you know the fix already. Otherwise you might need this workaround to get a core dump:
https://bugs.launchpad.net/ubuntu/+source/apport/+bug/1870060/comments/3

Sebastien Bacher (seb128) wrote :

Is the issue fixed in the current version? Do you plan to work on a SRU and targetting what serie?

Changed in bluez (Ubuntu):
importance: Undecided → High
status: New → Incomplete
Alex Tu (alextu) wrote :

this is the debdiff which applied the patch , and I also host the modified code there : https://code.launchpad.net/~alextu/ubuntu/+source/bluez/+git/bluez/+ref/lp1887910-bluetoothd-segv-keyboard

Sebastien Bacher (seb128) wrote :

@Alex, thanks, a few comments:

* could you make the bug SRU compliant (adding a description of the impact and the regression potential)

* it would be nice to have an user friendly description of what the patch is solving in the changelog, also the email you used is not a valid one

* could you include details on what upstream version / ubuntu serie already include the fix?

Alex Tu (alextu) wrote :

@seb128, yes, I modified the description and the debdiff is there in #4.

description: updated
Sebastien Bacher (seb128) wrote :

@Alex, sorry I though you were familiar with the SRU process, see https://wiki.ubuntu.com/StableReleaseUpdates for details

especially

'A [Regression Potential] section with a discussion of how regressions are most likely to manifest, or may manifest even if it is unlikely, as a result of this change.'

'low, becuase' isn't really enough...

Also I saw the debdiff, did you read my points about the description and email used in that one?

Changed in bluez (Ubuntu):
status: Incomplete → In Progress
Daniel van Vugt (vanvugt) wrote :

I'm going to remove Ubuntu Sponsors from this briefly, until the following are fixed:

1. The issue mentioned in comment #7 - The Bug Description on this page needs improving.

2. The email address used in the patch <u@SoundWire> needs replacing with your real email.

3. Need to mention the bug ID and the commit ID where the patch came from, in the patch.

Daniel van Vugt (vanvugt) wrote :

Oh, I can't remove Ubuntu Sponsors. Still, please fix all of the above.

Changed in bluez (Ubuntu Groovy):
status: In Progress → Fix Released
Changed in bluez (Ubuntu Focal):
status: New → Fix Released
Changed in bluez (Ubuntu Eoan):
status: New → Fix Released
Changed in bluez (Ubuntu Bionic):
status: New → In Progress
Changed in bluez (Ubuntu Focal):
importance: Undecided → High
Changed in bluez (Ubuntu Eoan):
importance: Undecided → High
Changed in bluez (Ubuntu Bionic):
importance: Undecided → High
assignee: nobody → Alex Tu (alextu)
tags: added: bionic
Daniel van Vugt (vanvugt) wrote :

BTW the fix first appeared in 5.50 (commit f1b8cdef95d6b51cab122f2be0694521f47092d0).

Alex Tu (alextu) on 2020-07-20
description: updated
Alex Tu (alextu) wrote :

Sorry,
the debugging and debdiff was done on the target machine so that the email ID was missed.

This attached new debdiff also included the patch and lp number information in the changelog.

And also updated the truncated description.

Sebastien Bacher (seb128) wrote :

Alex, did you read comment 7? 'low' isn't responding to what the regression potential should provide, it's there to describe what impact the change could have to know on what to focus the testing

Alex Tu (alextu) on 2020-07-20
description: updated
Alex Tu (alextu) wrote :

sorry for bad habit that I miss-understanded before.
I revised it and put detail information there.

Alex Tu (alextu) on 2020-07-23
Changed in bluez (Ubuntu Bionic):
status: In Progress → Confirmed
Alex Tu (alextu) wrote :

It's seems not just one patch can fix this issue. I found another corner case that still can trigger a blutoothd segmentation fault.

I pair one Bluetooth keyboard but cancel before input pairing keycode. Before timeout of latest keyboard pairing, I pair another Bluetooth keyboard and still cancel it before input pairing keycode.

Please hold this SRU process and I'm debugging for that corner case.

Changed in bluez (Ubuntu Bionic):
status: Confirmed → In Progress
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers