| Message ID | 20230210041030.865478-1-zyytlz.wz@163.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bluetooth: hci_core: Fix poential Use-after-Free bug in 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 |
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=720534 ---Test result--- Test Summary: CheckPatch PASS 0.67 seconds GitLint PASS 0.33 seconds SubjectPrefix PASS 0.12 seconds BuildKernel PASS 31.39 seconds CheckAllWarning PASS 34.34 seconds CheckSparse PASS 39.17 seconds CheckSmatch PASS 106.54 seconds BuildKernel32 PASS 30.43 seconds TestRunnerSetup PASS 435.55 seconds TestRunner_l2cap-tester PASS 16.51 seconds TestRunner_iso-tester PASS 17.04 seconds TestRunner_bnep-tester PASS 5.70 seconds TestRunner_mgmt-tester PASS 111.82 seconds TestRunner_rfcomm-tester PASS 9.06 seconds TestRunner_sco-tester PASS 8.34 seconds TestRunner_ioctl-tester PASS 9.73 seconds TestRunner_mesh-tester PASS 7.32 seconds TestRunner_smp-tester PASS 8.30 seconds TestRunner_userchan-tester PASS 6.02 seconds IncrementalBuild PASS 28.96 seconds --- Regards, Linux Bluetooth
Hi Zheng, On Thu, Feb 9, 2023 at 8:11 PM Zheng Wang <zyytlz.wz@163.com> wrote: > > In hci_remove_adv_monitor, if it gets into HCI_ADV_MONITOR_EXT_MSFT case, > the function will free the monitor and print its handle after that. > > Fix it by switch the order. > > Fixes: 7cf5c2978f23 ("Bluetooth: hci_sync: Refactor remove Adv Monitor") > Signed-off-by: Zheng Wang <zyytlz.wz@163.com> > --- > 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 b65c3aabcd53..db3352c60de6 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); I wonder if it is not a good idea to move the logging inside msft_remove_monitor? > break; > } > > -- > 2.25.1 >
Luiz Augusto von Dentz <luiz.dentz@gmail.com> 于2023年2月11日周六 03:53写道： > > Hi Zheng, > > On Thu, Feb 9, 2023 at 8:11 PM Zheng Wang <zyytlz.wz@163.com> wrote: > > > > In hci_remove_adv_monitor, if it gets into HCI_ADV_MONITOR_EXT_MSFT case, > > the function will free the monitor and print its handle after that. > > > > Fix it by switch the order. > > > > Fixes: 7cf5c2978f23 ("Bluetooth: hci_sync: Refactor remove Adv Monitor") > > Signed-off-by: Zheng Wang <zyytlz.wz@163.com> > > --- > > 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 b65c3aabcd53..db3352c60de6 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); > > I wonder if it is not a good idea to move the logging inside > msft_remove_monitor? > > > break; > > } > > Hello Luiz, Thanks for your quick reply. I think moving it inside msft_remove_monitor is a good idea. Because the variable status is returned by msft_remove_monitor. The call chain is msft_remove_monitor ->msft_remove_monitor_sync ->msft_le_cancel_monitor_advertisement_cb ->hci_free_adv_monitor ->kfree(monitor); We could mov the bt_dev_dbg to the location before hci_free_adv_monitor calling and delete the releated code in hci_remove_adv_monitor. If you agree with that, I'll make another patch. Best regards, Zheng Wang
Dear Zheng, Thank you for your patch. Am 10.02.23 um 05:10 schrieb Zheng Wang: > In hci_remove_adv_monitor, if it gets into HCI_ADV_MONITOR_EXT_MSFT case, > the function will free the monitor and print its handle after that. > > Fix it by switch the order. … by switch*ing* … There is a small typo in the commit message summary/title: po*t*ential > Fixes: 7cf5c2978f23 ("Bluetooth: hci_sync: Refactor remove Adv Monitor") > Signed-off-by: Zheng Wang <zyytlz.wz@163.com> > --- > net/bluetooth/hci_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) […] Kind regards, Paul
Paul Menzel <pmenzel@molgen.mpg.de> 于2023年2月13日周一 16:19写道： > > Dear Zheng, > > > Thank you for your patch. > > Am 10.02.23 um 05:10 schrieb Zheng Wang: > > In hci_remove_adv_monitor, if it gets into HCI_ADV_MONITOR_EXT_MSFT case, > > the function will free the monitor and print its handle after that. > > > > Fix it by switch the order. > > … by switch*ing* … > > There is a small typo in the commit message summary/title: po*t*ential > Hi Paul, Thanks for pointing that out ^^. Will correct it in the next version of patch. Best regards, Zheng Wang
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index b65c3aabcd53..db3352c60de6 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; }
In hci_remove_adv_monitor, if it gets into HCI_ADV_MONITOR_EXT_MSFT case, the function will free the monitor and print its handle after that. Fix it by switch the order. Fixes: 7cf5c2978f23 ("Bluetooth: hci_sync: Refactor remove Adv Monitor") Signed-off-by: Zheng Wang <zyytlz.wz@163.com> --- net/bluetooth/hci_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)