[PATCH v2] Bluetooth: hci_core: Fix poential Use-after-Free bug in hci_remove_adv_monitor

[PATCH v2] Bluetooth: hci_core: Fix poential Use-after-Free bug in hci_remove_adv_monitor

[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 removing the logging into msft_le_cancel_monitor_advertisement_cb
before calling hci_free_adv_monitor.

Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
---
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

RE: [v2] Bluetooth: hci_core: Fix poential Use-after-Free bug in hci_remove_adv_monitor

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2077 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=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

Re: [PATCH v2] Bluetooth: hci_core: Fix poential Use-after-Free bug in hci_remove_adv_monitor

[Zheng Hacker]

friendly ping

Zheng Wang <zyytlz.wz@163.com> 于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 <zyytlz.wz@163.com>
> ---
> 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
>

Re: [PATCH v2] Bluetooth: hci_core: Fix poential Use-after-Free bug in hci_remove_adv_monitor

[Simon Horman]

On Mon, Mar 13, 2023 at 05:55:35PM +0800, Zheng Hacker wrote:
> friendly ping
> 
> Zheng Wang <zyytlz.wz@163.com> 于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 <zyytlz.wz@163.com>
> > ---
> > 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
> >
> 

Re: [PATCH v2] Bluetooth: hci_core: Fix poential Use-after-Free bug in hci_remove_adv_monitor

[Simon Horman]

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 <zyytlz.wz@163.com> 于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 <zyytlz.wz@163.com>
> > > ---
> > > 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
> > >
> > 

Re: [PATCH v2] Bluetooth: hci_core: Fix poential Use-after-Free bug in hci_remove_adv_monitor

[Zheng Hacker]

Simon Horman <simon.horman@corigine.com> 于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 <zyytlz.wz@163.com> 于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 <zyytlz.wz@163.com>
> > > > ---
> > > > 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
> > > >
> > >