[PATCH bluetooth] Bluetooth: qca: fix NVM tag length underflow in TLV parser

[PATCH bluetooth] Bluetooth: qca: fix NVM tag length underflow in TLV parser

[Xiang Mei]

In the TLV_TYPE_NVM branch of qca_tlv_check_data() the tag loop bound is
"while (idx < length - sizeof(struct tlv_type_nvm))". "length" is a signed
int from the firmware TLV header and sizeof(struct tlv_type_nvm) is a
size_t (12), so "length" is converted to size_t and any firmware-supplied
"length" < 12 makes the subtraction wrap to a huge value. The loop body
then reads a 12-byte struct tlv_type_nvm past the end of the short
vmalloc'd firmware buffer (and the EDL_TAG_ID_* handlers can write past it).

Rewrite the bound as "idx + sizeof(struct tlv_type_nvm) <= length"; both
operands are non-negative, so it no longer underflows and a "length" too
small for one record correctly skips the loop.

  BUG: KASAN: vmalloc-out-of-bounds in qca_download_firmware.isra.0 (drivers/bluetooth/btqca.c:421)
  Read of size 2 at addr ffffc900000e5004 by task kworker/u9:0/52
  Workqueue: hci0 hci_power_on
  Call Trace:
   ...
   kasan_report (mm/kasan/report.c:595)
   qca_download_firmware.isra.0 (drivers/bluetooth/btqca.c:421 drivers/bluetooth/btqca.c:617)
   qca_uart_setup (drivers/bluetooth/btqca.c:948)
   qca_setup (drivers/bluetooth/hci_qca.c:2029)
   hci_uart_setup (drivers/bluetooth/hci_ldisc.c:438)
   hci_dev_open_sync (net/bluetooth/hci_sync.c:5227)
   hci_power_on (net/bluetooth/hci_core.c:920)
   process_one_work (kernel/workqueue.c:3322)
   worker_thread (kernel/workqueue.c:3486)
   kthread (kernel/kthread.c:436)
   ret_from_fork (arch/x86/kernel/process.c:158)
   ret_from_fork_asm (arch/x86/entry/entry_64.S:245)

Fixes: 2e4edfa1e2bd ("Bluetooth: qca: add missing firmware sanity checks")
Reported-by: Weiming Shi <bestswngs@gmail.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Xiang Mei <xmei5@asu.edu>
---
 drivers/bluetooth/btqca.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index 04ebe290bc78..10c496eaea2c 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -415,7 +415,7 @@ static int qca_tlv_check_data(struct hci_dev *hdev,
 
 		idx = 0;
 		data = tlv->data;
-		while (idx < length - sizeof(struct tlv_type_nvm)) {
+		while (idx + sizeof(struct tlv_type_nvm) <= length) {
 			tlv_nvm = (struct tlv_type_nvm *)(data + idx);
 
 			tag_id = le16_to_cpu(tlv_nvm->tag_id);
-- 
2.43.0

Re: [PATCH bluetooth] Bluetooth: qca: fix NVM tag length underflow in TLV parser

[Johan Hovold]

On Sat, Jul 04, 2026 at 04:10:30PM -0700, Xiang Mei wrote:
> In the TLV_TYPE_NVM branch of qca_tlv_check_data() the tag loop bound is
> "while (idx < length - sizeof(struct tlv_type_nvm))". "length" is a signed
> int from the firmware TLV header and sizeof(struct tlv_type_nvm) is a
> size_t (12), so "length" is converted to size_t and any firmware-supplied
> "length" < 12 makes the subtraction wrap to a huge value.

Yeah, there should have been a lower bound check on length before the
loop.

> The loop body
> then reads a 12-byte struct tlv_type_nvm past the end of the short
> vmalloc'd firmware buffer (and the EDL_TAG_ID_* handlers can write past it).

No, only four bytes are read before the loop exits without any further
consequences due to the following check:

	 if (length < idx + sizeof(struct tlv_type_nvm) + tag_len)
	 	return -EINVAL;

which is false if length < 12.

> Rewrite the bound as "idx + sizeof(struct tlv_type_nvm) <= length"; both
> operands are non-negative, so it no longer underflows and a "length" too
> small for one record correctly skips the loop.

This works too.

>   BUG: KASAN: vmalloc-out-of-bounds in qca_download_firmware.isra.0 (drivers/bluetooth/btqca.c:421)
>   Read of size 2 at addr ffffc900000e5004 by task kworker/u9:0/52
>   Workqueue: hci0 hci_power_on
>   Call Trace:
>    ...
>    kasan_report (mm/kasan/report.c:595)
>    qca_download_firmware.isra.0 (drivers/bluetooth/btqca.c:421 drivers/bluetooth/btqca.c:617)
>    qca_uart_setup (drivers/bluetooth/btqca.c:948)
>    qca_setup (drivers/bluetooth/hci_qca.c:2029)
>    hci_uart_setup (drivers/bluetooth/hci_ldisc.c:438)
>    hci_dev_open_sync (net/bluetooth/hci_sync.c:5227)
>    hci_power_on (net/bluetooth/hci_core.c:920)
>    process_one_work (kernel/workqueue.c:3322)
>    worker_thread (kernel/workqueue.c:3486)
>    kthread (kernel/kthread.c:436)
>    ret_from_fork (arch/x86/kernel/process.c:158)
>    ret_from_fork_asm (arch/x86/entry/entry_64.S:245)
> 
> Fixes: 2e4edfa1e2bd ("Bluetooth: qca: add missing firmware sanity checks")
> Reported-by: Weiming Shi <bestswngs@gmail.com>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Xiang Mei <xmei5@asu.edu>
> ---
>  drivers/bluetooth/btqca.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index 04ebe290bc78..10c496eaea2c 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -415,7 +415,7 @@ static int qca_tlv_check_data(struct hci_dev *hdev,
>  
>  		idx = 0;
>  		data = tlv->data;
> -		while (idx < length - sizeof(struct tlv_type_nvm)) {
> +		while (idx + sizeof(struct tlv_type_nvm) <= length) {
>  			tlv_nvm = (struct tlv_type_nvm *)(data + idx);
>  
>  			tag_id = le16_to_cpu(tlv_nvm->tag_id);

With the commit message fixed you can add my:

Reviewed-by: Johan Hovold <johan@kernel.org>

Johan

Re: [PATCH bluetooth] Bluetooth: qca: fix NVM tag length underflow in TLV parser

[Bartosz Golaszewski]

On Sun, 5 Jul 2026 01:10:30 +0200, Xiang Mei <xmei5@asu.edu> said:
> In the TLV_TYPE_NVM branch of qca_tlv_check_data() the tag loop bound is
> "while (idx < length - sizeof(struct tlv_type_nvm))". "length" is a signed
> int from the firmware TLV header and sizeof(struct tlv_type_nvm) is a
> size_t (12), so "length" is converted to size_t and any firmware-supplied
> "length" < 12 makes the subtraction wrap to a huge value. The loop body
> then reads a 12-byte struct tlv_type_nvm past the end of the short
> vmalloc'd firmware buffer (and the EDL_TAG_ID_* handlers can write past it).
>
> Rewrite the bound as "idx + sizeof(struct tlv_type_nvm) <= length"; both
> operands are non-negative, so it no longer underflows and a "length" too
> small for one record correctly skips the loop.
>
>   BUG: KASAN: vmalloc-out-of-bounds in qca_download_firmware.isra.0 (drivers/bluetooth/btqca.c:421)
>   Read of size 2 at addr ffffc900000e5004 by task kworker/u9:0/52
>   Workqueue: hci0 hci_power_on
>   Call Trace:
>    ...
>    kasan_report (mm/kasan/report.c:595)
>    qca_download_firmware.isra.0 (drivers/bluetooth/btqca.c:421 drivers/bluetooth/btqca.c:617)
>    qca_uart_setup (drivers/bluetooth/btqca.c:948)
>    qca_setup (drivers/bluetooth/hci_qca.c:2029)
>    hci_uart_setup (drivers/bluetooth/hci_ldisc.c:438)
>    hci_dev_open_sync (net/bluetooth/hci_sync.c:5227)
>    hci_power_on (net/bluetooth/hci_core.c:920)
>    process_one_work (kernel/workqueue.c:3322)
>    worker_thread (kernel/workqueue.c:3486)
>    kthread (kernel/kthread.c:436)
>    ret_from_fork (arch/x86/kernel/process.c:158)
>    ret_from_fork_asm (arch/x86/entry/entry_64.S:245)
>
> Fixes: 2e4edfa1e2bd ("Bluetooth: qca: add missing firmware sanity checks")
> Reported-by: Weiming Shi <bestswngs@gmail.com>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Xiang Mei <xmei5@asu.edu>
> ---
>  drivers/bluetooth/btqca.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index 04ebe290bc78..10c496eaea2c 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -415,7 +415,7 @@ static int qca_tlv_check_data(struct hci_dev *hdev,
>
>  		idx = 0;
>  		data = tlv->data;
> -		while (idx < length - sizeof(struct tlv_type_nvm)) {
> +		while (idx + sizeof(struct tlv_type_nvm) <= length) {
>  			tlv_nvm = (struct tlv_type_nvm *)(data + idx);
>
>  			tag_id = le16_to_cpu(tlv_nvm->tag_id);
> --
> 2.43.0
>
>

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Re: [PATCH bluetooth] Bluetooth: qca: fix NVM tag length underflow in TLV parser

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Sat,  4 Jul 2026 16:10:30 -0700 you wrote:
> In the TLV_TYPE_NVM branch of qca_tlv_check_data() the tag loop bound is
> "while (idx < length - sizeof(struct tlv_type_nvm))". "length" is a signed
> int from the firmware TLV header and sizeof(struct tlv_type_nvm) is a
> size_t (12), so "length" is converted to size_t and any firmware-supplied
> "length" < 12 makes the subtraction wrap to a huge value. The loop body
> then reads a 12-byte struct tlv_type_nvm past the end of the short
> vmalloc'd firmware buffer (and the EDL_TAG_ID_* handlers can write past it).
> 
> [...]

Here is the summary with links:
  - Bluetooth: qca: fix NVM tag length underflow in TLV parser
    https://git.kernel.org/bluetooth/bluetooth-next/c/198bd312df38

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bluetooth] Bluetooth: qca: fix NVM tag length underflow in TLV parser

[Xiang Mei]

Xiang

On Sun, Jul 5, 2026 at 2:41 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Sat, Jul 04, 2026 at 04:10:30PM -0700, Xiang Mei wrote:
> > In the TLV_TYPE_NVM branch of qca_tlv_check_data() the tag loop bound is
> > "while (idx < length - sizeof(struct tlv_type_nvm))". "length" is a signed
> > int from the firmware TLV header and sizeof(struct tlv_type_nvm) is a
> > size_t (12), so "length" is converted to size_t and any firmware-supplied
> > "length" < 12 makes the subtraction wrap to a huge value.
>
> Yeah, there should have been a lower bound check on length before the
> loop.
>
> > The loop body
> > then reads a 12-byte struct tlv_type_nvm past the end of the short
> > vmalloc'd firmware buffer (and the EDL_TAG_ID_* handlers can write past it).
>
> No, only four bytes are read before the loop exits without any further
> consequences due to the following check:
>
>          if (length < idx + sizeof(struct tlv_type_nvm) + tag_len)
>                 return -EINVAL;
>
> which is false if length < 12.
>
Thanks Johan, and you're right. Only the 2-byte tag_id (and at most
the 4 bytes of tag_id/tag_len) is read out of bounds. This commit
message overstated the impact, and I should have caught that.

Unfortunately, the bot merged this patch when I was preparing the v2
this morning. If you think the inaccurate commit message is worth
fixing on record, I can send a follow-up to the maintainers to correct
the message

Xiang
> > Rewrite the bound as "idx + sizeof(struct tlv_type_nvm) <= length"; both
> > operands are non-negative, so it no longer underflows and a "length" too
> > small for one record correctly skips the loop.
>
> This works too.
>
> >   BUG: KASAN: vmalloc-out-of-bounds in qca_download_firmware.isra.0 (drivers/bluetooth/btqca.c:421)
> >   Read of size 2 at addr ffffc900000e5004 by task kworker/u9:0/52
> >   Workqueue: hci0 hci_power_on
> >   Call Trace:
> >    ...
> >    kasan_report (mm/kasan/report.c:595)
> >    qca_download_firmware.isra.0 (drivers/bluetooth/btqca.c:421 drivers/bluetooth/btqca.c:617)
> >    qca_uart_setup (drivers/bluetooth/btqca.c:948)
> >    qca_setup (drivers/bluetooth/hci_qca.c:2029)
> >    hci_uart_setup (drivers/bluetooth/hci_ldisc.c:438)
> >    hci_dev_open_sync (net/bluetooth/hci_sync.c:5227)
> >    hci_power_on (net/bluetooth/hci_core.c:920)
> >    process_one_work (kernel/workqueue.c:3322)
> >    worker_thread (kernel/workqueue.c:3486)
> >    kthread (kernel/kthread.c:436)
> >    ret_from_fork (arch/x86/kernel/process.c:158)
> >    ret_from_fork_asm (arch/x86/entry/entry_64.S:245)
> >
> > Fixes: 2e4edfa1e2bd ("Bluetooth: qca: add missing firmware sanity checks")
> > Reported-by: Weiming Shi <bestswngs@gmail.com>
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Xiang Mei <xmei5@asu.edu>
> > ---
> >  drivers/bluetooth/btqca.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> > index 04ebe290bc78..10c496eaea2c 100644
> > --- a/drivers/bluetooth/btqca.c
> > +++ b/drivers/bluetooth/btqca.c
> > @@ -415,7 +415,7 @@ static int qca_tlv_check_data(struct hci_dev *hdev,
> >
> >               idx = 0;
> >               data = tlv->data;
> > -             while (idx < length - sizeof(struct tlv_type_nvm)) {
> > +             while (idx + sizeof(struct tlv_type_nvm) <= length) {
> >                       tlv_nvm = (struct tlv_type_nvm *)(data + idx);
> >
> >                       tag_id = le16_to_cpu(tlv_nvm->tag_id);
>
> With the commit message fixed you can add my:
>
> Reviewed-by: Johan Hovold <johan@kernel.org>
>
> Johan

Re: [PATCH bluetooth] Bluetooth: qca: fix NVM tag length underflow in TLV parser

[Johan Hovold]

On Tue, Jul 07, 2026 at 10:55:23AM -0700, Xiang Mei wrote:
> On Sun, Jul 5, 2026 at 2:41 AM Johan Hovold <johan@kernel.org> wrote:
> > On Sat, Jul 04, 2026 at 04:10:30PM -0700, Xiang Mei wrote:

> > > The loop body
> > > then reads a 12-byte struct tlv_type_nvm past the end of the short
> > > vmalloc'd firmware buffer (and the EDL_TAG_ID_* handlers can write past it).
> >
> > No, only four bytes are read before the loop exits without any further
> > consequences due to the following check:

> Thanks Johan, and you're right. Only the 2-byte tag_id (and at most
> the 4 bytes of tag_id/tag_len) is read out of bounds. This commit
> message overstated the impact, and I should have caught that.
> 
> Unfortunately, the bot merged this patch when I was preparing the v2
> this morning. If you think the inaccurate commit message is worth
> fixing on record, I can send a follow-up to the maintainers to correct
> the message

It was Luiz who applied the patch -- perhaps he did not notice my
conditional Reviewed-by tag.

I assume the Bluetooth tree is not rebased so I don't think there's
anything that can be done here and there is no need to send a v2 (unless
Luiz says otherwise).

> > With the commit message fixed you can add my:
> >
> > Reviewed-by: Johan Hovold <johan@kernel.org>

Johan

Re: [PATCH bluetooth] Bluetooth: qca: fix NVM tag length underflow in TLV parser

[Luiz Augusto von Dentz]

Hi Johan,

On Wed, Jul 8, 2026 at 10:21 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, Jul 07, 2026 at 10:55:23AM -0700, Xiang Mei wrote:
> > On Sun, Jul 5, 2026 at 2:41 AM Johan Hovold <johan@kernel.org> wrote:
> > > On Sat, Jul 04, 2026 at 04:10:30PM -0700, Xiang Mei wrote:
>
> > > > The loop body
> > > > then reads a 12-byte struct tlv_type_nvm past the end of the short
> > > > vmalloc'd firmware buffer (and the EDL_TAG_ID_* handlers can write past it).
> > >
> > > No, only four bytes are read before the loop exits without any further
> > > consequences due to the following check:
>
> > Thanks Johan, and you're right. Only the 2-byte tag_id (and at most
> > the 4 bytes of tag_id/tag_len) is read out of bounds. This commit
> > message overstated the impact, and I should have caught that.
> >
> > Unfortunately, the bot merged this patch when I was preparing the v2
> > this morning. If you think the inaccurate commit message is worth
> > fixing on record, I can send a follow-up to the maintainers to correct
> > the message
>
> It was Luiz who applied the patch -- perhaps he did not notice my
> conditional Reviewed-by tag.
>
> I assume the Bluetooth tree is not rebased so I don't think there's
> anything that can be done here and there is no need to send a v2 (unless
> Luiz says otherwise).

If only the commit message that needs updating then I guess we don't
need to worry about if stating something slightly inaccurate.

> > > With the commit message fixed you can add my:
> > >
> > > Reviewed-by: Johan Hovold <johan@kernel.org>
>
> Johan



-- 
Luiz Augusto von Dentz

Re: [PATCH bluetooth] Bluetooth: qca: fix NVM tag length underflow in TLV parser

[Johan Hovold]

On Wed, Jul 08, 2026 at 11:15:00AM -0400, Luiz Augusto von Dentz wrote:
> On Wed, Jul 8, 2026 at 10:21 AM Johan Hovold <johan@kernel.org> wrote:
> > On Tue, Jul 07, 2026 at 10:55:23AM -0700, Xiang Mei wrote:
> > > On Sun, Jul 5, 2026 at 2:41 AM Johan Hovold <johan@kernel.org> wrote:
> > > > On Sat, Jul 04, 2026 at 04:10:30PM -0700, Xiang Mei wrote:

> > > > > The loop body
> > > > > then reads a 12-byte struct tlv_type_nvm past the end of the short
> > > > > vmalloc'd firmware buffer (and the EDL_TAG_ID_* handlers can write past it).
> > > >
> > > > No, only four bytes are read before the loop exits without any further
> > > > consequences due to the following check:
> >
> > > Thanks Johan, and you're right. Only the 2-byte tag_id (and at most
> > > the 4 bytes of tag_id/tag_len) is read out of bounds. This commit
> > > message overstated the impact, and I should have caught that.
> > >
> > > Unfortunately, the bot merged this patch when I was preparing the v2
> > > this morning. If you think the inaccurate commit message is worth
> > > fixing on record, I can send a follow-up to the maintainers to correct
> > > the message
> >
> > It was Luiz who applied the patch -- perhaps he did not notice my
> > conditional Reviewed-by tag.
> >
> > I assume the Bluetooth tree is not rebased so I don't think there's
> > anything that can be done here and there is no need to send a v2 (unless
> > Luiz says otherwise).
> 
> If only the commit message that needs updating then I guess we don't
> need to worry about if stating something slightly inaccurate.

It's mostly an issue for backporting and determining the impact of the
fix. The commit message as it stands suggests that the bug can result in
memory corruption when it really is just a 4-byte oob read.

And then the inclusion of my tag makes it look like I failed to spot
this during review.

But let's keep it this way this time.

Johan