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] [thread-next>] [day] [month] [year] [list]
Message-ID: <tdj2vssbhlth7gxggbnltssux6r7kzhn3guloefmieboszejo3@7yrut5ygk3yq>
Date: Thu, 17 Jul 2025 00:11:16 -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 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/

> 
> 
> -- 
> Luiz Augusto von Dentz

Thank you for your feedback.

	Ivan Pravdin