linux-kernel - Re: [PATCH] Bluetooth: hci_sync: Avoid use-after-free in dbg for hci_remove_adv

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 30 Jun 2023 15:30:55 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Luiz Augusto von Dentz <luiz.dentz@...il.com>
Cc:     Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Stephen Boyd <swboyd@...omium.org>,
        Manish Mandlik <mmandlik@...gle.com>,
        Miao-chen Chou <mcchou@...gle.com>,
        linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Bluetooth: hci_sync: Avoid use-after-free in dbg for hci_remove_adv_monitor()

Hi,

On Fri, Jun 30, 2023 at 3:11 PM Doug Anderson <dianders@...omium.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