Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230630143125.1.I3b7c8905728f3124576361ca35ed28e37f12f5d1@changeid>
 <CABBYNZLtxMGQLFwZTztPo2epV2E9Z4uthFMeozxABW2KXQR1nQ@mail.gmail.com> <CAD=FV=XvSCzi1hjgcWa_AecsdG7wbe9o29n2M1cWLezqZk6Dig@mail.gmail.com>
In-Reply-To: <CAD=FV=XvSCzi1hjgcWa_AecsdG7wbe9o29n2M1cWLezqZk6Dig@mail.gmail.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Fri, 30 Jun 2023 15:30:55 -0700
Message-ID: <CAD=FV=VoCaDSS17pRJredtMYPTXiK8uoLOH5hEXaCdqCb9SONA@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: hci_sync: Avoid use-after-free in dbg for hci_remove_adv_monitor()
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Stephen Boyd <swboyd@chromium.org>,
        Manish Mandlik <mmandlik@google.com>,
        Miao-chen Chou <mcchou@google.com>,
        linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Fri, Jun 30, 2023 at 3:11=E2=80=AFPM Doug Anderson <dianders@chromium.or=
g> wrote:
>
> > > @@ -1980,9 +1981,10 @@ static int hci_remove_adv_monitor(struct hci_d=
ev *hdev,
> > >                 goto free_monitor;
> > >
> > >         case HCI_ADV_MONITOR_EXT_MSFT:
> > > +               handle =3D monitor->handle;
> > >                 status =3D 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