| Message ID | 20230630150906.v2.1.I3b7c8905728f3124576361ca35ed28e37f12f5d1@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] Bluetooth: hci_sync: Avoid use-after-free in dbg for hci_remove_adv_monitor() | expand |
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
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
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; }
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(-)