[v2,1/2] Bluetooth: hci_sync: Avoid use-after-free in dbg for hci_remove_adv_monitor()

Commit Message

Doug Anderson June 30, 2023, 10:09 p.m. UTC

KASAN reports that there's a use-after-free in
hci_remove_adv_monitor(). Trawling through the disassembly, you can
see that the complaint is from the access in bt_dev_dbg() under the
HCI_ADV_MONITOR_EXT_MSFT case. The problem case happens because
msft_remove_monitor() can end up freeing the monitor
structure. Specifically:
  hci_remove_adv_monitor() ->
  msft_remove_monitor() ->
  msft_remove_monitor_sync() ->
  msft_le_cancel_monitor_advertisement_cb() ->
  hci_free_adv_monitor()

Moving the printout to before the memory is freed.

Fixes: 7cf5c2978f23 ("Bluetooth: hci_sync: Refactor remove Adv Monitor")
Signed-off-by: Douglas Anderson <[e-mail]>
---

Changes in v2:
- Move the print, don't stash handle in a local.

 net/bluetooth/hci_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Doug Anderson June 30, 2023, 10:25 p.m. UTC | #1

Hi,

On Fri, Jun 30, 2023 at 3:20 PM Manish Mandlik <[e-mail]> wrote:
>
> Hi Douglas,
>
>
>
> On Fri, Jun 30, 2023 at 3:10 PM Douglas Anderson <[e-mail]> wrote:
>>
>> KASAN reports that there's a use-after-free in
>> hci_remove_adv_monitor(). Trawling through the disassembly, you can
>> see that the complaint is from the access in bt_dev_dbg() under the
>> HCI_ADV_MONITOR_EXT_MSFT case. The problem case happens because
>> msft_remove_monitor() can end up freeing the monitor
>> structure. Specifically:
>>   hci_remove_adv_monitor() ->
>>   msft_remove_monitor() ->
>>   msft_remove_monitor_sync() ->
>>   msft_le_cancel_monitor_advertisement_cb() ->
>>   hci_free_adv_monitor()
>>
>> Moving the printout to before the memory is freed.
>>
>> Fixes: 7cf5c2978f23 ("Bluetooth: hci_sync: Refactor remove Adv Monitor")
>> Signed-off-by: Douglas Anderson <[e-mail]>
>> ---
>>
>> Changes in v2:
>> - Move the print, don't stash handle in a local.
>>
>>  net/bluetooth/hci_core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 48917c68358d..7e2ecf8a55d8 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1980,9 +1980,9 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev,
>>                 goto free_monitor;
>>
>>         case HCI_ADV_MONITOR_EXT_MSFT:
>> -               status = msft_remove_monitor(hdev, monitor);
>>                 bt_dev_dbg(hdev, "%s remove monitor %d msft status %d",
>>                            hdev->name, monitor->handle, status);
>> +               status = msft_remove_monitor(hdev, monitor);
>
> Thanks for catching this. But that log also prints the value of "status". So, moving "status = msft_remove_monitor(hdev, monitor);" after the print will print an incorrect value of "status".

Ah crud, you're right. :(

OK, I'll send v3 which will be back to v1 for the first patch.

-Doug

[e-mail] June 30, 2023, 10:33 p.m. UTC | #2

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

---Test result---

Test Summary:
CheckPatch                    PASS      1.42 seconds
GitLint                       FAIL      0.95 seconds
SubjectPrefix                 PASS      0.25 seconds
BuildKernel                   PASS      33.76 seconds
CheckAllWarning               PASS      36.91 seconds
CheckSparse                   PASS      42.22 seconds
CheckSmatch                   PASS      113.64 seconds
BuildKernel32                 PASS      32.64 seconds
TestRunnerSetup               PASS      464.86 seconds
TestRunner_l2cap-tester       PASS      14.30 seconds
TestRunner_iso-tester         PASS      25.74 seconds
TestRunner_bnep-tester        PASS      6.16 seconds
TestRunner_mgmt-tester        PASS      140.60 seconds
TestRunner_rfcomm-tester      PASS      9.94 seconds
TestRunner_sco-tester         PASS      11.26 seconds
TestRunner_ioctl-tester       PASS      10.58 seconds
TestRunner_mesh-tester        PASS      7.70 seconds
TestRunner_smp-tester         PASS      8.97 seconds
TestRunner_userchan-tester    PASS      6.43 seconds
IncrementalBuild              PASS      36.49 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v2,1/2] Bluetooth: hci_sync: Avoid use-after-free in dbg for hci_remove_adv_monitor()

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
1: T1 Title exceeds max length (86>80): "[v2,1/2] Bluetooth: hci_sync: Avoid use-after-free in dbg for hci_remove_adv_monitor()"


---
Regards,
Linux Bluetooth

Patch

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 48917c68358d..7e2ecf8a55d8 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1980,9 +1980,9 @@  static int hci_remove_adv_monitor(struct hci_dev *hdev,
 		goto free_monitor;
 
 	case HCI_ADV_MONITOR_EXT_MSFT:
-		status = msft_remove_monitor(hdev, monitor);
 		bt_dev_dbg(hdev, "%s remove monitor %d msft status %d",
 			   hdev->name, monitor->handle, status);
+		status = msft_remove_monitor(hdev, monitor);
 		break;
 	}