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 removing the logging into msft_le_cancel_monitor_advertisement_cb
before calling hci_free_adv_monitor.
Signed-off-by: Zheng Wang <[email protected]>
---
v2:
- move the logging inside msft_remove_monitor suggested by Luiz
---
net/bluetooth/hci_core.c | 2 --
net/bluetooth/msft.c | 2 ++
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index b65c3aabcd53..69b82c2907ff 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1981,8 +1981,6 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev,
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);
break;
}
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index bee6a4c656be..4b35f0ed1360 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -286,6 +286,8 @@ static int msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
* suspend. It will be re-monitored on resume.
*/
if (!msft->suspending) {
+ bt_dev_dbg(hdev, "%s remove monitor %d status %d", hdev->name,
+ monitor->handle, status);
hci_free_adv_monitor(hdev, monitor);
/* Clear any monitored devices by this Adv Monitor */
--
2.25.1
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=722888
---Test result---
Test Summary:
CheckPatch PASS 0.80 seconds
GitLint FAIL 0.66 seconds
SubjectPrefix PASS 0.12 seconds
BuildKernel PASS 30.50 seconds
CheckAllWarning PASS 33.46 seconds
CheckSparse PASS 37.77 seconds
CheckSmatch PASS 107.02 seconds
BuildKernel32 PASS 29.51 seconds
TestRunnerSetup PASS 427.93 seconds
TestRunner_l2cap-tester PASS 16.08 seconds
TestRunner_iso-tester PASS 16.20 seconds
TestRunner_bnep-tester PASS 5.32 seconds
TestRunner_mgmt-tester PASS 106.40 seconds
TestRunner_rfcomm-tester PASS 8.53 seconds
TestRunner_sco-tester PASS 7.86 seconds
TestRunner_ioctl-tester PASS 9.20 seconds
TestRunner_mesh-tester PASS 6.70 seconds
TestRunner_smp-tester PASS 7.74 seconds
TestRunner_userchan-tester PASS 5.63 seconds
IncrementalBuild PASS 27.62 seconds
Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v2] Bluetooth: hci_core: Fix poential Use-after-Free bug in 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 (83>80): "[v2] Bluetooth: hci_core: Fix poential Use-after-Free bug in hci_remove_adv_monitor"
---
Regards,
Linux Bluetooth
friendly ping
Zheng Wang <[email protected]> 于2023年2月17日周五 18:05写道：
>
> 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 removing the logging into msft_le_cancel_monitor_advertisement_cb
> before calling hci_free_adv_monitor.
>
> Signed-off-by: Zheng Wang <[email protected]>
> ---
> v2:
> - move the logging inside msft_remove_monitor suggested by Luiz
> ---
> net/bluetooth/hci_core.c | 2 --
> net/bluetooth/msft.c | 2 ++
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index b65c3aabcd53..69b82c2907ff 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1981,8 +1981,6 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev,
>
> 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);
> break;
> }
>
> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> index bee6a4c656be..4b35f0ed1360 100644
> --- a/net/bluetooth/msft.c
> +++ b/net/bluetooth/msft.c
> @@ -286,6 +286,8 @@ static int msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
> * suspend. It will be re-monitored on resume.
> */
> if (!msft->suspending) {
> + bt_dev_dbg(hdev, "%s remove monitor %d status %d", hdev->name,
> + monitor->handle, status);
> hci_free_adv_monitor(hdev, monitor);
>
> /* Clear any monitored devices by this Adv Monitor */
> --
> 2.25.1
>
On Mon, Mar 13, 2023 at 05:55:35PM +0800, Zheng Hacker wrote:
> friendly ping
>
> Zheng Wang <[email protected]> 于2023年2月17日周五 18:05写道：
> >
> > 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 removing the logging into msft_le_cancel_monitor_advertisement_cb
> > before calling hci_free_adv_monitor.
> >
> > Signed-off-by: Zheng Wang <[email protected]>
> > ---
> > v2:
> > - move the logging inside msft_remove_monitor suggested by Luiz
> > ---
> > net/bluetooth/hci_core.c | 2 --
> > net/bluetooth/msft.c | 2 ++
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index b65c3aabcd53..69b82c2907ff 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -1981,8 +1981,6 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev,
> >
> > 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);
> > break;
I'm probably missing something obvious.
But from my perspective a simpler fix would be to
move the msft_remove_monitor() call to below the bt_dev_dbg() call.
> > }
> >
> > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> > index bee6a4c656be..4b35f0ed1360 100644
> > --- a/net/bluetooth/msft.c
> > +++ b/net/bluetooth/msft.c
> > @@ -286,6 +286,8 @@ static int msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
> > * suspend. It will be re-monitored on resume.
> > */
> > if (!msft->suspending) {
> > + bt_dev_dbg(hdev, "%s remove monitor %d status %d", hdev->name,
> > + monitor->handle, status);
> > hci_free_adv_monitor(hdev, monitor);
> >
> > /* Clear any monitored devices by this Adv Monitor */
> > --
> > 2.25.1
> >
>
On Tue, Mar 14, 2023 at 04:06:11PM +0100, Simon Horman wrote:
> On Mon, Mar 13, 2023 at 05:55:35PM +0800, Zheng Hacker wrote:
> > friendly ping
> >
> > Zheng Wang <[email protected]> 于2023年2月17日周五 18:05写道：
> > >
> > > 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 removing the logging into msft_le_cancel_monitor_advertisement_cb
> > > before calling hci_free_adv_monitor.
> > >
> > > Signed-off-by: Zheng Wang <[email protected]>
> > > ---
> > > v2:
> > > - move the logging inside msft_remove_monitor suggested by Luiz
> > > ---
> > > net/bluetooth/hci_core.c | 2 --
> > > net/bluetooth/msft.c | 2 ++
> > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > index b65c3aabcd53..69b82c2907ff 100644
> > > --- a/net/bluetooth/hci_core.c
> > > +++ b/net/bluetooth/hci_core.c
> > > @@ -1981,8 +1981,6 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev,
> > >
> > > 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);
> > > break;
>
> I'm probably missing something obvious.
> But from my perspective a simpler fix would be to
> move the msft_remove_monitor() call to below the bt_dev_dbg() call.
The obvious thing I was missing is that was what was done in v1
but Luiz suggested moving the logging to
msft_le_cancel_monitor_advertisement_cb().
Sorry for the noise.
Link: https://lore.kernel.org/all/CABBYNZL_gZ+kr_OEqjYgMmt+=91=jC88g310F-ScMC=kLh0xdw@mail.gmail.com/
>
> > > }
> > >
> > > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> > > index bee6a4c656be..4b35f0ed1360 100644
> > > --- a/net/bluetooth/msft.c
> > > +++ b/net/bluetooth/msft.c
> > > @@ -286,6 +286,8 @@ static int msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
> > > * suspend. It will be re-monitored on resume.
> > > */
> > > if (!msft->suspending) {
> > > + bt_dev_dbg(hdev, "%s remove monitor %d status %d", hdev->name,
> > > + monitor->handle, status);
> > > hci_free_adv_monitor(hdev, monitor);
> > >
> > > /* Clear any monitored devices by this Adv Monitor */
> > > --
> > > 2.25.1
> > >
> >
Simon Horman <[email protected]> 于2023年3月14日周二 23:22写道：
>
> On Tue, Mar 14, 2023 at 04:06:11PM +0100, Simon Horman wrote:
> > On Mon, Mar 13, 2023 at 05:55:35PM +0800, Zheng Hacker wrote:
> > > friendly ping
> > >
> > > Zheng Wang <[email protected]> 于2023年2月17日周五 18:05写道：
> > > >
> > > > 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 removing the logging into msft_le_cancel_monitor_advertisement_cb
> > > > before calling hci_free_adv_monitor.
> > > >
> > > > Signed-off-by: Zheng Wang <[email protected]>
> > > > ---
> > > > v2:
> > > > - move the logging inside msft_remove_monitor suggested by Luiz
> > > > ---
> > > > net/bluetooth/hci_core.c | 2 --
> > > > net/bluetooth/msft.c | 2 ++
> > > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > > index b65c3aabcd53..69b82c2907ff 100644
> > > > --- a/net/bluetooth/hci_core.c
> > > > +++ b/net/bluetooth/hci_core.c
> > > > @@ -1981,8 +1981,6 @@ static int hci_remove_adv_monitor(struct hci_dev *hdev,
> > > >
> > > > 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);
> > > > break;
> >
> > I'm probably missing something obvious.
> > But from my perspective a simpler fix would be to
> > move the msft_remove_monitor() call to below the bt_dev_dbg() call.
>
> The obvious thing I was missing is that was what was done in v1
> but Luiz suggested moving the logging to
> msft_le_cancel_monitor_advertisement_cb().
> Sorry for the noise.
Hi Simon,
Thanks for your reply and detailed review :)
Best regards,
Zheng
>
> Link: https://lore.kernel.org/all/CABBYNZL_gZ+kr_OEqjYgMmt+=91=jC88g310F-ScMC=kLh0xdw@mail.gmail.com/
>
> >
> > > > }
> > > >
> > > > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> > > > index bee6a4c656be..4b35f0ed1360 100644
> > > > --- a/net/bluetooth/msft.c
> > > > +++ b/net/bluetooth/msft.c
> > > > @@ -286,6 +286,8 @@ static int msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
> > > > * suspend. It will be re-monitored on resume.
> > > > */
> > > > if (!msft->suspending) {
> > > > + bt_dev_dbg(hdev, "%s remove monitor %d status %d", hdev->name,
> > > > + monitor->handle, status);
> > > > hci_free_adv_monitor(hdev, monitor);
> > > >
> > > > /* Clear any monitored devices by this Adv Monitor */
> > > > --
> > > > 2.25.1
> > > >
> > >