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

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

[Douglas Anderson]

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 <dianders@chromium.org>
---

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);
 		break;
 	}
 
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH v2 2/2] Bluetooth: hci_sync: Don't double print name in add/remove adv_monitor

[Douglas Anderson]

The hci_add_adv_monitor() hci_remove_adv_monitor() functions call
bt_dev_dbg() to print some debug statements. The bt_dev_dbg() macro
automatically adds in the device's name. That means that we shouldn't
include the name in the bt_dev_dbg() calls.

Suggested-by: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- ("Don't double print name...") new for v2.

 net/bluetooth/hci_core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7e2ecf8a55d8..a8e54383e551 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1949,14 +1949,14 @@ int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
 
 	switch (hci_get_adv_monitor_offload_ext(hdev)) {
 	case HCI_ADV_MONITOR_EXT_NONE:
-		bt_dev_dbg(hdev, "%s add monitor %d status %d", hdev->name,
+		bt_dev_dbg(hdev, "add monitor %d status %d",
 			   monitor->handle, status);
 		/* Message was not forwarded to controller - not an error */
 		break;
 
 	case HCI_ADV_MONITOR_EXT_MSFT:
 		status = msft_add_monitor_pattern(hdev, monitor);
-		bt_dev_dbg(hdev, "%s add monitor %d msft status %d", hdev->name,
+		bt_dev_dbg(hdev, "add monitor %d msft status %d",
 			   monitor->handle, status);
 		break;
 	}
@@ -1975,13 +1975,13 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev,
 
 	switch (hci_get_adv_monitor_offload_ext(hdev)) {
 	case HCI_ADV_MONITOR_EXT_NONE: /* also goes here when powered off */
-		bt_dev_dbg(hdev, "%s remove monitor %d status %d", hdev->name,
+		bt_dev_dbg(hdev, "remove monitor %d status %d",
 			   monitor->handle, status);
 		goto free_monitor;
 
 	case HCI_ADV_MONITOR_EXT_MSFT:
-		bt_dev_dbg(hdev, "%s remove monitor %d msft status %d",
-			   hdev->name, monitor->handle, status);
+		bt_dev_dbg(hdev, "remove monitor %d msft status %d",
+			   monitor->handle, status);
 		status = msft_remove_monitor(hdev, monitor);
 		break;
 	}
-- 
2.41.0.255.g8b1d071c50-goog

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

[Doug Anderson]

Hi,

On Fri, Jun 30, 2023 at 3:20 PM Manish Mandlik <mmandlik@google.com> wrote:
>
> Hi Douglas,
>
>
>
> On Fri, Jun 30, 2023 at 3:10 PM Douglas Anderson <dianders@chromium.org> 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 <dianders@chromium.org>
>> ---
>>
>> 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

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

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]

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