| Message ID | 20230630143125.1.I3b7c8905728f3124576361ca35ed28e37f12f5d1@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bluetooth: hci_sync: Avoid use-after-free in dbg for hci_remove_adv_monitor() | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | success | TestRunner PASS |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
Hi Douglas, On Fri, Jun 30, 2023 at 2:40 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() > > Let's fix the problem by just stashing the relevant data when it's > still valid. > > Fixes: 7cf5c2978f23 ("Bluetooth: hci_sync: Refactor remove Adv Monitor") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > net/bluetooth/hci_core.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index 48917c68358d..dbb2043a9112 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -1972,6 +1972,7 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev, > struct adv_monitor *monitor) > { > int status = 0; > + int handle; > > switch (hci_get_adv_monitor_offload_ext(hdev)) { > case HCI_ADV_MONITOR_EXT_NONE: /* also goes here when powered off */ > @@ -1980,9 +1981,10 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev, > goto free_monitor; > > case HCI_ADV_MONITOR_EXT_MSFT: > + handle = monitor->handle; > status = msft_remove_monitor(hdev, monitor); > bt_dev_dbg(hdev, "%s remove monitor %d msft status %d", > - hdev->name, monitor->handle, status); > + hdev->name, handle, status); Just move the call to bt_dev_dbg under msft_remove_monitor, also there is no reason to print hdev->name since bt_dev_dbg already does that so while at it we can probably fix this as well. > break; > } > > -- > 2.41.0.255.g8b1d071c50-goog >
Hi, On Fri, Jun 30, 2023 at 2:55 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Douglas, > > On Fri, Jun 30, 2023 at 2:40 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() > > > > Let's fix the problem by just stashing the relevant data when it's > > still valid. > > > > Fixes: 7cf5c2978f23 ("Bluetooth: hci_sync: Refactor remove Adv Monitor") > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > net/bluetooth/hci_core.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > > index 48917c68358d..dbb2043a9112 100644 > > --- a/net/bluetooth/hci_core.c > > +++ b/net/bluetooth/hci_core.c > > @@ -1972,6 +1972,7 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev, > > struct adv_monitor *monitor) > > { > > int status = 0; > > + int handle; > > > > switch (hci_get_adv_monitor_offload_ext(hdev)) { > > case HCI_ADV_MONITOR_EXT_NONE: /* also goes here when powered off */ > > @@ -1980,9 +1981,10 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev, > > goto free_monitor; > > > > case HCI_ADV_MONITOR_EXT_MSFT: > > + handle = monitor->handle; > > status = msft_remove_monitor(hdev, monitor); > > bt_dev_dbg(hdev, "%s remove monitor %d msft status %d", > > - hdev->name, monitor->handle, status); > > + hdev->name, handle, status); > > Just move the call to bt_dev_dbg under msft_remove_monitor, Sure. I wasn't sure how much the order of the printout matters, but if it doesn't then just putting the print first makes sense. Done in v2. > also there > is no reason to print hdev->name since bt_dev_dbg already does that so > while at it we can probably fix this as well. I made that a separate patch just to keep it cleaner. I also fixed the "add" function which has the same issue. -Doug
Hi, On Fri, Jun 30, 2023 at 3:11 PM Doug Anderson <dianders@chromium.org> wrote: > > > > @@ -1980,9 +1981,10 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev, > > > goto free_monitor; > > > > > > case HCI_ADV_MONITOR_EXT_MSFT: > > > + handle = monitor->handle; > > > status = msft_remove_monitor(hdev, monitor); > > > bt_dev_dbg(hdev, "%s remove monitor %d msft status %d", > > > - hdev->name, monitor->handle, status); > > > + hdev->name, handle, status); > > > > Just move the call to bt_dev_dbg under msft_remove_monitor, > > Sure. I wasn't sure how much the order of the printout matters, but if > it doesn't then just putting the print first makes sense. Done in v2. So I assumed that this meant you just wanted me to switch the order, which I did for v2. ...but then Manish pointed out that meant I wasn't printing the right status. Looking again, maybe you meant that I should move the debug statement into the msft_remove_monitor(). I'm not convinced that's any cleaner. That would mean adding an "exit" label to that function just for the printout. It also makes the printout asymmetric with other similar printouts. I'm going back to v1 here. If I've misunderstood then I guess I can always spin again. :-/ -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=761687 ---Test result--- Test Summary: CheckPatch PASS 0.82 seconds GitLint PASS 0.39 seconds SubjectPrefix PASS 0.14 seconds BuildKernel PASS 40.34 seconds CheckAllWarning PASS 43.82 seconds CheckSparse PASS 49.54 seconds CheckSmatch PASS 133.50 seconds BuildKernel32 PASS 38.92 seconds TestRunnerSetup PASS 548.39 seconds TestRunner_l2cap-tester PASS 16.87 seconds TestRunner_iso-tester PASS 30.80 seconds TestRunner_bnep-tester PASS 7.45 seconds TestRunner_mgmt-tester PASS 158.27 seconds TestRunner_rfcomm-tester PASS 11.48 seconds TestRunner_sco-tester PASS 12.65 seconds TestRunner_ioctl-tester PASS 12.88 seconds TestRunner_mesh-tester PASS 9.09 seconds TestRunner_smp-tester PASS 10.63 seconds TestRunner_userchan-tester PASS 7.63 seconds IncrementalBuild PASS 36.82 seconds --- Regards, Linux Bluetooth
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 48917c68358d..dbb2043a9112 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -1972,6 +1972,7 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor) { int status = 0; + int handle; switch (hci_get_adv_monitor_offload_ext(hdev)) { case HCI_ADV_MONITOR_EXT_NONE: /* also goes here when powered off */ @@ -1980,9 +1981,10 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev, goto free_monitor; case HCI_ADV_MONITOR_EXT_MSFT: + handle = monitor->handle; status = msft_remove_monitor(hdev, monitor); bt_dev_dbg(hdev, "%s remove monitor %d msft status %d", - hdev->name, monitor->handle, status); + hdev->name, handle, status); 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() Let's fix the problem by just stashing the relevant data when it's still valid. Fixes: 7cf5c2978f23 ("Bluetooth: hci_sync: Refactor remove Adv Monitor") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- net/bluetooth/hci_core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)