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@xxxxxxxxxx> 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@xxxxxxxxx>
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Xiang Mei <xmei5@xxxxxxx>
> > ---
> >  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@xxxxxxxxxx>
>
> Johan