linux-kernel - Re: [PATCH v2] Bluetooth: coredump: Use tmp buffer with dev

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]
Message-ID: <v7jemzeqc6cgvpb67iyzoxg5wbuhxfjprhpdamszkvs3dvglu4@2dzbx4d76ipj>
Date: Thu, 17 Jul 2025 01:06:48 -0400
From: Ivan Pravdin <ipravdin.official@...il.com>
To: Luiz Augusto von Dentz <luiz.dentz@...il.com>
Cc: marcel@...tmann.org, johan.hedberg@...il.com, 
	linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org, 
	syzbot+ac3c79181f6aecc5120c@...kaller.appspotmail.com
Subject: Re: [PATCH v2] Bluetooth: coredump: Use tmp buffer with dev_coredumpv

On Thu, Jul 17, 2025 at 12:11:16AM GMT, Ivan Pravdin wrote:
> On Wed, Jul 16, 2025 at 10:11:01AM GMT, Luiz Augusto von Dentz wrote:
> > Hi Ivan,
> > 
> > On Tue, Jul 15, 2025 at 8:38 PM Ivan Pravdin
> > <ipravdin.official@...il.com> wrote:
> > >
> > > Create and use new vmalloc'ed buffer with dev_coredumpv. From
> > > dev_coredumpv documentation:
> > >
> > > `This function takes ownership of the vmalloc'ed data and will free
> > > it when it is no longer used.`
> > >
> > > As hdev->dump is used after dev_coredumpv, create temporary buffer to
> > > hold hdev->dump data.
> > >
> > > Reported-by: syzbot+ac3c79181f6aecc5120c@...kaller.appspotmail.com
> > > Closes: https://lore.kernel.org/all/67eaa688.050a0220.1547ec.014a.GAE@google.com
> > 
> > It should probably close the syzkaller issue, which I assume is this one:
> > 
> > https://syzkaller.appspot.com/bug?extid=ac3c79181f6aecc5120c
> 
> I will change it in v3.
> 
> > 
> > > Fixes: b257e02ecc46 ("HCI: coredump: Log devcd dumps into the monitor")
> > > Signed-off-by: Ivan Pravdin <ipravdin.official@...il.com>
> > > ---
> > > v1 -> v2: Changed subject prefix to Bluetooth:
> > >
> > >  net/bluetooth/coredump.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/bluetooth/coredump.c b/net/bluetooth/coredump.c
> > > index 819eacb38762..1232c9a94f95 100644
> > > --- a/net/bluetooth/coredump.c
> > > +++ b/net/bluetooth/coredump.c
> > > @@ -243,6 +243,7 @@ static void hci_devcd_handle_pkt_pattern(struct hci_dev *hdev,
> > >  static void hci_devcd_dump(struct hci_dev *hdev)
> > >  {
> > >         struct sk_buff *skb;
> > > +       char *coredump;
> > >         u32 size;
> > >
> > >         bt_dev_dbg(hdev, "state %d", hdev->dump.state);
> > > @@ -250,7 +251,11 @@ static void hci_devcd_dump(struct hci_dev *hdev)
> > >         size = hdev->dump.tail - hdev->dump.head;
> > >
> > >         /* Emit a devcoredump with the available data */
> > > -       dev_coredumpv(&hdev->dev, hdev->dump.head, size, GFP_KERNEL);
> > > +       coredump = vmalloc(size);
> > > +       if (coredump) {
> > > +               memcpy(coredump, hdev->dump.head, size);
> > > +               dev_coredumpv(&hdev->dev, coredump, size, GFP_KERNEL);
> > > +       }
> > >
> > >
> > >         /* Send a copy to monitor as a diagnostic packet */
> > >         skb = bt_skb_alloc(size, GFP_ATOMIC);
> > > --
> > > 2.45.2
> > >
> > 
> > What is wrong here the is code that attempt to send a copy to the
> > monitor uses dump.head _after_ dev_coredumpv has freed it, so just
> > changing the order shall make it work properly:
> > 
> > diff --git a/net/bluetooth/coredump.c b/net/bluetooth/coredump.c
> > index 819eacb38762..720cb79adf96 100644
> > --- a/net/bluetooth/coredump.c
> > +++ b/net/bluetooth/coredump.c
> > @@ -249,15 +249,15 @@ static void hci_devcd_dump(struct hci_dev *hdev)
> > 
> >         size = hdev->dump.tail - hdev->dump.head;
> > 
> > -       /* Emit a devcoredump with the available data */
> > -       dev_coredumpv(&hdev->dev, hdev->dump.head, size, GFP_KERNEL);
> > -
> >         /* Send a copy to monitor as a diagnostic packet */
> >         skb = bt_skb_alloc(size, GFP_ATOMIC);
> >         if (skb) {
> >                 skb_put_data(skb, hdev->dump.head, size);
> >                 hci_recv_diag(hdev, skb);
> >         }
> > +
> > +       /* Emit a devcoredump with the available data */
> > +       dev_coredumpv(&hdev->dev, hdev->dump.head, size, GFP_KERNEL);
> >  }
> > 
> >  static void hci_devcd_handle_pkt_complete(struct hci_dev *hdev
> 
> Unfortunately this does not work. This was my initial attempt but it
> still reproduces the error [2]. The reason for that is that
> dev_coredumpv takes the ownership of the buffer. From the comment
> above dev_coredumpm_timeout (which is used by dev_coredumpv):
> 
> > Creates a new device coredump for the given device. If a previous one hasn't
> > been read yet, the new coredump is discarded. The data lifetime is determined
> > by the device coredump framework and when it is no longer needed the @free
> > function will be called to free the data.
> 
> If hci_devcd_dump is called multiple times, first time hdev->dump.head
> will be kept. However, calling hci_devcd_dump second time before data is
> read by userspace will free it, leading to uaf error like the one
> observed in the report. 
> 
> As hci_devcd_dump need not be called only one time before hdev is freed, 
> calling it multiple times will lead to uaf, no matter in what order
> skb_put_data and dev_coredumpv occur in the function.
> 
> To solve this problem, a temporary buffer should be used. Such buffer
> should copy data from hdev->dump.head and be surrendered to
> dev_coredumpv without any possibility to be altered or freed.
> 
> For reference, here are some examples of using temporary buffer with
> dev_coredumpv in Intel AVS driver:
> 
> [bluetoot-next 6f40d15c70bc] sound/soc/intel/avs/apl.c:183
> [bluetoot-next 6f40d15c70bc] sound/soc/intel/avs/skl.c:147
> 
> [2] https://lore.kernel.org/all/6845dc6b.050a0220.daf97.0af4.GAE@google.com/
> 

Nevermind... I was under impression that hdev->dump is allocated only
once and is reused until hdev is freed. The link I have copied actually
shows another error that is solved by another patch [1]. I have tested
your suggestion on top of unintegrated patch and it did not reproduce
any issues [2].

I will include your suggestion in v3.

Thanks.

[1] https://lore.kernel.org/all/20250716004252.125466-2-ipravdin.official@gmail.com/
[2] https://lore.kernel.org/all/gkct6i53onzjuzugsor6pnrhjfef7trwhgbjrgfie25xirebhp@dpqxvkguqdnf/

> > 
> > 
> > -- 
> > Luiz Augusto von Dentz
> 
> Thank you for your feedback.
> 
> 	Ivan Pravdin

	Ivan Pravdin