mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH 0/3] Bluetooth: btmtk: firmware debug event routing and WMT FUNC_CTRL status fixes
@ 2026-09-11 10:42 Chris Lu
  2026-09-11 10:42 ` [PATCH 1/3] Bluetooth: btmtk: Route firmware debug event to the diag channel Chris Lu
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Chris Lu @ 2026-09-11 10:42 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Luiz Von Dentz
  Cc: Sean Wang, Will Lee, SS Wu, linux-bluetooth, linux-kernel,
	linux-mediatek, Chris Lu

This series bundles three independent MediaTek Bluetooth driver fixes:

Patch 1 is a resend of a fix submitted on 25 Aug 2026
("Bluetooth: btmtk: Route firmware debug event to the diag channel")
that received no review feedback. There are no code changes since
that submission; resending it alongside the two related fixes below.

Patches 2-3 fix how btmtk_usb_hci_wmt_sync() (and its btmtksdio.c /
btmtkuart.c counterparts) interpret a WMT FUNC_CTRL event that carries
only the WMT header and no trailing 2-byte status word. Such an event
is a normal firmware ack for a plain enable/disable request, with the
result carried in the header's own flag byte, not a failure as the
current code assumes:

  - Patch 2 fixes this for btmtk.c, where a bounds check already
    existed (added by e3ac0d9f1a20) but defaulted to the wrong
    result.
  - Patch 3 applies the same fix to btmtksdio.c and btmtkuart.c, which
    never had a bounds check for this event at all and read 2 bytes
    past the end of the received SKB whenever firmware sent the short
    form. While there, it also adds the missing base WMT header length
    check that btmtk.c already has (skb_pull_data() before touching
    wmt_evt->whdr.op), since these two files were unconditionally
    dereferencing that field with no length validation at all.

Chris Lu (3):
  Bluetooth: btmtk: Route firmware debug event to the diag channel
  Bluetooth: btmtk: fix wrong status for short WMT FUNC_CTRL events
  Bluetooth: btmtksdio, btmtkuart: validate WMT event length before
    struct access

 drivers/bluetooth/btmtk.c     |  8 +++++++-
 drivers/bluetooth/btmtksdio.c | 20 +++++++++++++++++++-
 drivers/bluetooth/btmtkuart.c | 19 ++++++++++++++++++-
 3 files changed, 44 insertions(+), 3 deletions(-)

--
2.45.2

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/3] Bluetooth: btmtk: Route firmware debug event to the diag channel
  2026-09-11 10:42 [PATCH 0/3] Bluetooth: btmtk: firmware debug event routing and WMT FUNC_CTRL status fixes Chris Lu
@ 2026-09-11 10:42 ` Chris Lu
  2026-09-11 10:42 ` [PATCH 2/3] Bluetooth: btmtk: fix wrong status for short WMT FUNC_CTRL events Chris Lu
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Chris Lu @ 2026-09-11 10:42 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Luiz Von Dentz
  Cc: Sean Wang, Will Lee, SS Wu, linux-bluetooth, linux-kernel,
	linux-mediatek, Chris Lu

MediaTek controllers may emit a firmware debug event on the ACL channel
using the reserved handle 0x0efd, which shows up in the ACL header as
0x2efd once the start fragment flag is included.

Neither btmtk_usb_recv_acl() nor btmtksdio_recv_acl() recognizes it, so
the packet is passed to the HCI core, which has no connection with that
handle and complains:

  Bluetooth: hci0: ACL packet for unknown connection handle 3837

Handle it the same way as the existing firmware debug logging packets and
forward it to the diagnostic channel instead.

Verified on MT7922: under the condition that triggers this firmware
debug event, it is now routed to the diag channel instead of reaching
the host as an unknown ACL packet.

Signed-off-by: Chris Lu <chris.lu@mediatek.com>
---
 drivers/bluetooth/btmtk.c     | 1 +
 drivers/bluetooth/btmtksdio.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/bluetooth/btmtk.c b/drivers/bluetooth/btmtk.c
index 660ed5b02841..5b0a131bc9d2 100644
--- a/drivers/bluetooth/btmtk.c
+++ b/drivers/bluetooth/btmtk.c
@@ -1076,6 +1076,7 @@ int btmtk_usb_recv_acl(struct hci_dev *hdev, struct sk_buff *skb)
 		fallthrough;
 	case 0x05ff:		/* Firmware debug logging 1 */
 	case 0x05fe:		/* Firmware debug logging 2 */
+	case 0x2efd:		/* Firmware debug event */
 		return hci_recv_diag(hdev, skb);
 	}

diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index fe4ca9395aa3..9a1b6f9b0872 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -460,6 +460,7 @@ static int btmtksdio_recv_acl(struct hci_dev *hdev, struct sk_buff *skb)
 		fallthrough;
 	case 0x05ff:
 	case 0x05fe:
+	case 0x2efd:		/* Firmware debug event */
 		/* Firmware debug logging */
 		return hci_recv_diag(hdev, skb);
 	}
--
2.45.2

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/3] Bluetooth: btmtk: fix wrong status for short WMT FUNC_CTRL events
  2026-09-11 10:42 [PATCH 0/3] Bluetooth: btmtk: firmware debug event routing and WMT FUNC_CTRL status fixes Chris Lu
  2026-09-11 10:42 ` [PATCH 1/3] Bluetooth: btmtk: Route firmware debug event to the diag channel Chris Lu
@ 2026-09-11 10:42 ` Chris Lu
  2026-09-11 10:42 ` [PATCH 3/3] Bluetooth: btmtksdio, btmtkuart: validate WMT event length before struct access Chris Lu
  2026-09-11 14:33 ` [PATCH 0/3] Bluetooth: btmtk: firmware debug event routing and WMT FUNC_CTRL status fixes Luiz Augusto von Dentz
  3 siblings, 0 replies; 6+ messages in thread
From: Chris Lu @ 2026-09-11 10:42 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Luiz Von Dentz
  Cc: Sean Wang, Will Lee, SS Wu, linux-bluetooth, linux-kernel,
	linux-mediatek, Chris Lu

A too-short BTMTK_WMT_FUNC_CTRL event (WMT header only, no trailing
2-byte status word) is always treated as BTMTK_WMT_ON_UNDONE. This
short form is how firmware acks a plain enable/disable request, and
the actual result is carried in the header's own flag byte (0 =
success), not a separate status word. Decode it from there instead of
assuming failure.

Verified setup on MT7920, MT7921, MT7922 and MT7925: no regression.

Fixes: e3ac0d9f1a20 ("Bluetooth: btmtk: accept too short WMT FUNC_CTRL events")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Chris Lu <chris.lu@mediatek.com>
---
 drivers/bluetooth/btmtk.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btmtk.c b/drivers/bluetooth/btmtk.c
index 660ed5b02841..03b99826b52c 100644
--- a/drivers/bluetooth/btmtk.c
+++ b/drivers/bluetooth/btmtk.c
@@ -791,7 +791,12 @@ static int btmtk_usb_hci_wmt_sync(struct hci_dev *hdev,
 	case BTMTK_WMT_FUNC_CTRL:
 		if (!skb_pull_data(data->evt_skb,
 				   sizeof(wmt_evt_funcc->status))) {
-			status = BTMTK_WMT_ON_UNDONE;
+			/* A plain enable/disable request is acked with just
+			 * the WMT header and no trailing status word; the
+			 * result is carried in the header's own flag byte.
+			 */
+			status = wmt_evt->whdr.flag ? BTMTK_WMT_ON_UNDONE :
+						       BTMTK_WMT_ON_DONE;
 			break;
 		}

--
2.45.2

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 3/3] Bluetooth: btmtksdio, btmtkuart: validate WMT event length before struct access
  2026-09-11 10:42 [PATCH 0/3] Bluetooth: btmtk: firmware debug event routing and WMT FUNC_CTRL status fixes Chris Lu
  2026-09-11 10:42 ` [PATCH 1/3] Bluetooth: btmtk: Route firmware debug event to the diag channel Chris Lu
  2026-09-11 10:42 ` [PATCH 2/3] Bluetooth: btmtk: fix wrong status for short WMT FUNC_CTRL events Chris Lu
@ 2026-09-11 10:42 ` Chris Lu
  2026-09-11 14:33 ` [PATCH 0/3] Bluetooth: btmtk: firmware debug event routing and WMT FUNC_CTRL status fixes Luiz Augusto von Dentz
  3 siblings, 0 replies; 6+ messages in thread
From: Chris Lu @ 2026-09-11 10:42 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Luiz Von Dentz
  Cc: Sean Wang, Will Lee, SS Wu, linux-bluetooth, linux-kernel,
	linux-mediatek, Chris Lu

btmtksdio.c and btmtkuart.c cast a received WMT event straight to
struct btmtk_hci_wmt_evt and read its op/flag fields without checking
the event is long enough to contain them, unlike btmtk.c. The
FUNC_CTRL case then further casts to struct btmtk_hci_wmt_evt_funcc
and reads its 2-byte status field, again without a length check.
Firmware that sends a short or malformed WMT event makes both drivers
read past the end of the received SKB.

Mirror btmtk.c: validate the base WMT header with skb_pull_data()
before touching any of its fields, and when a FUNC_CTRL event turns
out to be the short, header-only form (a plain enable/disable ack
with no status word), decode the result from the header's own flag
byte instead (0 = success, otherwise failure).

Verified setup on MT7920, MT7921, MT7922 and MT7925: no regression.

Fixes: 9aebfd4a2200 ("Bluetooth: mediatek: add support for MediaTek MT7663S and MT7668S SDIO devices")
Fixes: e0b67035a90b ("Bluetooth: mediatek: update the common setup between MT7622 and other devices")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Chris Lu <chris.lu@mediatek.com>
---
 drivers/bluetooth/btmtksdio.c | 20 +++++++++++++++++++-
 drivers/bluetooth/btmtkuart.c | 20 +++++++++++++++++++-
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index fe4ca9395aa3..a8ebcd0c005c 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -217,7 +217,14 @@ static int mtk_hci_wmt_sync(struct hci_dev *hdev,
 	}

 	/* Parse and handle the return WMT event */
-	wmt_evt = (struct btmtk_hci_wmt_evt *)bdev->evt_skb->data;
+	wmt_evt = skb_pull_data(bdev->evt_skb, sizeof(*wmt_evt));
+	if (!wmt_evt) {
+		bt_dev_err(hdev, "WMT event too short (%u bytes)",
+			   bdev->evt_skb->len);
+		err = -EINVAL;
+		goto err_free_skb;
+	}
+
 	if (wmt_evt->whdr.op != hdr->op) {
 		bt_dev_err(hdev, "Wrong op received %d expected %d",
 			   wmt_evt->whdr.op, hdr->op);
@@ -233,6 +240,17 @@ static int mtk_hci_wmt_sync(struct hci_dev *hdev,
 			status = BTMTK_WMT_PATCH_DONE;
 		break;
 	case BTMTK_WMT_FUNC_CTRL:
+		if (!skb_pull_data(bdev->evt_skb,
+				   sizeof(wmt_evt_funcc->status))) {
+			/* A plain enable/disable request is acked with just
+			 * the WMT header and no trailing status word; the
+			 * result is carried in the header's own flag byte.
+			 */
+			status = wmt_evt->whdr.flag ? BTMTK_WMT_ON_UNDONE :
+						       BTMTK_WMT_ON_DONE;
+			break;
+		}
+
 		wmt_evt_funcc = (struct btmtk_hci_wmt_evt_funcc *)wmt_evt;
 		if (be16_to_cpu(wmt_evt_funcc->status) == 0x404)
 			status = BTMTK_WMT_ON_DONE;
diff --git a/drivers/bluetooth/btmtkuart.c b/drivers/bluetooth/btmtkuart.c
index 27aa48ff3ac2..4af6fbbbd302 100644
--- a/drivers/bluetooth/btmtkuart.c
+++ b/drivers/bluetooth/btmtkuart.c
@@ -151,7 +151,14 @@ static int mtk_hci_wmt_sync(struct hci_dev *hdev,
 	}

 	/* Parse and handle the return WMT event */
-	wmt_evt = (struct btmtk_hci_wmt_evt *)bdev->evt_skb->data;
+	wmt_evt = skb_pull_data(bdev->evt_skb, sizeof(*wmt_evt));
+	if (!wmt_evt) {
+		bt_dev_err(hdev, "WMT event too short (%u bytes)",
+			   bdev->evt_skb->len);
+		err = -EINVAL;
+		goto err_free_wc;
+	}
+
 	if (wmt_evt->whdr.op != hdr->op) {
 		bt_dev_err(hdev, "Wrong op received %d expected %d",
 			   wmt_evt->whdr.op, hdr->op);
@@ -167,6 +174,17 @@ static int mtk_hci_wmt_sync(struct hci_dev *hdev,
 			status = BTMTK_WMT_PATCH_DONE;
 		break;
 	case BTMTK_WMT_FUNC_CTRL:
+		if (!skb_pull_data(bdev->evt_skb,
+				   sizeof(wmt_evt_funcc->status))) {
+			/* A plain enable/disable request is acked with just
+			 * the WMT header and no trailing status word; the
+			 * result is carried in the header's own flag byte.
+			 */
+			status = wmt_evt->whdr.flag ? BTMTK_WMT_ON_UNDONE :
+						       BTMTK_WMT_ON_DONE;
+			break;
+		}
+
 		wmt_evt_funcc = (struct btmtk_hci_wmt_evt_funcc *)wmt_evt;
 		if (be16_to_cpu(wmt_evt_funcc->status) == 0x404)
 			status = BTMTK_WMT_ON_DONE;
--
2.45.2

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/3] Bluetooth: btmtk: firmware debug event routing and WMT FUNC_CTRL status fixes
  2026-09-11 10:42 [PATCH 0/3] Bluetooth: btmtk: firmware debug event routing and WMT FUNC_CTRL status fixes Chris Lu
                   ` (2 preceding siblings ...)
  2026-09-11 10:42 ` [PATCH 3/3] Bluetooth: btmtksdio, btmtkuart: validate WMT event length before struct access Chris Lu
@ 2026-09-11 14:33 ` Luiz Augusto von Dentz
  2026-09-14  6:53   ` Chris Lu (陸稚泓)
  3 siblings, 1 reply; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2026-09-11 14:33 UTC (permalink / raw)
  To: Chris Lu
  Cc: Marcel Holtmann, Johan Hedberg, Sean Wang, Will Lee, SS Wu,
	linux-bluetooth, linux-kernel, linux-mediatek

Hi Chris,

On Fri, Sep 11, 2026 at 6:42 AM Chris Lu <chris.lu@mediatek.com> wrote:
>
> This series bundles three independent MediaTek Bluetooth driver fixes:
>
> Patch 1 is a resend of a fix submitted on 25 Aug 2026
> ("Bluetooth: btmtk: Route firmware debug event to the diag channel")
> that received no review feedback. There are no code changes since
> that submission; resending it alongside the two related fixes below.
>
> Patches 2-3 fix how btmtk_usb_hci_wmt_sync() (and its btmtksdio.c /
> btmtkuart.c counterparts) interpret a WMT FUNC_CTRL event that carries
> only the WMT header and no trailing 2-byte status word. Such an event
> is a normal firmware ack for a plain enable/disable request, with the
> result carried in the header's own flag byte, not a failure as the
> current code assumes:
>
>   - Patch 2 fixes this for btmtk.c, where a bounds check already
>     existed (added by e3ac0d9f1a20) but defaulted to the wrong
>     result.
>   - Patch 3 applies the same fix to btmtksdio.c and btmtkuart.c, which
>     never had a bounds check for this event at all and read 2 bytes
>     past the end of the received SKB whenever firmware sent the short
>     form. While there, it also adds the missing base WMT header length
>     check that btmtk.c already has (skb_pull_data() before touching
>     wmt_evt->whdr.op), since these two files were unconditionally
>     dereferencing that field with no length validation at all.
>
> Chris Lu (3):
>   Bluetooth: btmtk: Route firmware debug event to the diag channel
>   Bluetooth: btmtk: fix wrong status for short WMT FUNC_CTRL events
>   Bluetooth: btmtksdio, btmtkuart: validate WMT event length before
>     struct access
>
>  drivers/bluetooth/btmtk.c     |  8 +++++++-
>  drivers/bluetooth/btmtksdio.c | 20 +++++++++++++++++++-
>  drivers/bluetooth/btmtkuart.c | 19 ++++++++++++++++++-
>  3 files changed, 44 insertions(+), 3 deletions(-)
>
> --
> 2.45.2

Sashiko flagged a problem regarding the usage of ACL connection handle
without masking the PB field:

https://sashiko.dev/#/patchset/20260911104234.2276126-1-chris.lu%40mediatek.com

If the HCI fragmentation doesn't apply to these handles, please add a
comment regarding it; otherwise, users like Sashiko will keep flagging
it going forward.


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/3] Bluetooth: btmtk: firmware debug event routing and WMT FUNC_CTRL status fixes
  2026-09-11 14:33 ` [PATCH 0/3] Bluetooth: btmtk: firmware debug event routing and WMT FUNC_CTRL status fixes Luiz Augusto von Dentz
@ 2026-09-14  6:53   ` Chris Lu (陸稚泓)
  0 siblings, 0 replies; 6+ messages in thread
From: Chris Lu (陸稚泓) @ 2026-09-14  6:53 UTC (permalink / raw)
  To: luiz.dentz
  Cc: Will-CY Lee (李政穎),
	marcel, SS Wu (巫憲欣),
	linux-kernel, johan.hedberg, Sean Wang, linux-bluetooth,
	linux-mediatek

Hi Luiz,

On Fri, 2026-09-11 at 10:33 -0400, Luiz Augusto von Dentz wrote:
> Hi Chris,
> 
> On Fri, Sep 11, 2026 at 6:42 AM Chris Lu <chris.lu@mediatek.com>
> wrote:
> > 
> > This series bundles three independent MediaTek Bluetooth driver
> > fixes:
> > 
> > Patch 1 is a resend of a fix submitted on 25 Aug 2026
> > ("Bluetooth: btmtk: Route firmware debug event to the diag
> > channel")
> > that received no review feedback. There are no code changes since
> > that submission; resending it alongside the two related fixes
> > below.
> > 
> > Patches 2-3 fix how btmtk_usb_hci_wmt_sync() (and its btmtksdio.c /
> > btmtkuart.c counterparts) interpret a WMT FUNC_CTRL event that
> > carries
> > only the WMT header and no trailing 2-byte status word. Such an
> > event
> > is a normal firmware ack for a plain enable/disable request, with
> > the
> > result carried in the header's own flag byte, not a failure as the
> > current code assumes:
> > 
> >   - Patch 2 fixes this for btmtk.c, where a bounds check already
> >     existed (added by e3ac0d9f1a20) but defaulted to the wrong
> >     result.
> >   - Patch 3 applies the same fix to btmtksdio.c and btmtkuart.c,
> > which
> >     never had a bounds check for this event at all and read 2 bytes
> >     past the end of the received SKB whenever firmware sent the
> > short
> >     form. While there, it also adds the missing base WMT header
> > length
> >     check that btmtk.c already has (skb_pull_data() before touching
> >     wmt_evt->whdr.op), since these two files were unconditionally
> >     dereferencing that field with no length validation at all.
> > 
> > Chris Lu (3):
> >   Bluetooth: btmtk: Route firmware debug event to the diag channel
> >   Bluetooth: btmtk: fix wrong status for short WMT FUNC_CTRL events
> >   Bluetooth: btmtksdio, btmtkuart: validate WMT event length before
> >     struct access
> > 
> >  drivers/bluetooth/btmtk.c     |  8 +++++++-
> >  drivers/bluetooth/btmtksdio.c | 20 +++++++++++++++++++-
> >  drivers/bluetooth/btmtkuart.c | 19 ++++++++++++++++++-
> >  3 files changed, 44 insertions(+), 3 deletions(-)
> > 
> > --
> > 2.45.2
> 
> Sashiko flagged a problem regarding the usage of ACL connection
> handle
> without masking the PB field:
> 
> https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260911104234.2276126-1-chris.lu*40mediatek.com__;IyU!!CTRNKA9wMg0ARbw!jwNe1Tfud02zOuV-S3UGvy1vdJQtqffgxH1tvNsv8D4Qk5oGmd-B00PQYhbifDx9HwTx7Kr__RP2ZAjjnro$
>  
> 
> If the HCI fragmentation doesn't apply to these handles, please add a
> comment regarding it; otherwise, users like Sashiko will keep
> flagging
> it going forward.
> 
> 

Confirmed with MediaTek internally: 0x0efd (and the other three
vendor-reserved handles already handled in this switch: 0xfc6f, 0x05ff,
0x05fe) is not a real connection handle -- it's used to multiplex
firmware debug/dump data onto the ACL channel. This event is always
sent as a single, complete ACL_START packet, so HCI fragmentation
(and thus a matching ACL_CONT case, 0x1efd) never applies to it.

Added a comment above the switch statement in both btmtk.c and
btmtksdio.c explaining this for all four handles, so it's clear going
forward. Will include it in the next version.

Thanks for catching this.

BRs,
Chris Lu


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-09-14  6:53 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-11 10:42 [PATCH 0/3] Bluetooth: btmtk: firmware debug event routing and WMT FUNC_CTRL status fixes Chris Lu
2026-09-11 10:42 ` [PATCH 1/3] Bluetooth: btmtk: Route firmware debug event to the diag channel Chris Lu
2026-09-11 10:42 ` [PATCH 2/3] Bluetooth: btmtk: fix wrong status for short WMT FUNC_CTRL events Chris Lu
2026-09-11 10:42 ` [PATCH 3/3] Bluetooth: btmtksdio, btmtkuart: validate WMT event length before struct access Chris Lu
2026-09-11 14:33 ` [PATCH 0/3] Bluetooth: btmtk: firmware debug event routing and WMT FUNC_CTRL status fixes Luiz Augusto von Dentz
2026-09-14  6:53   ` Chris Lu (陸稚泓)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®