[PATCH 0/2] Bluetooth: HIDP: validate short receive frames

[PATCH 0/2] Bluetooth: HIDP: validate short receive frames

[Sangho Lee]

The HIDP control and interrupt receive paths assume that every L2CAP SDU
contains a transaction header. The raw-report return path also assumes that
a numbered DATA response contains a report ID. Both assumptions allow a
connected peer to make HIDP consume data beyond the logical skb boundary.

The tests used two BlueZ 5.87 btvirt BR/EDR controllers, real L2CAP PSM
0x11/0x13 channels, HIDPCONNADD, and HIDIOCGFEATURE on bluetooth.git at
df541cd485ff. The series also applies without changes to bluetooth-next at
6f55ad8fb0ac.

On the unpatched KMSAN kernel, an empty control SDU produced two
uninitialized-value reports in hidp_session_run(), an empty interrupt SDU
produced one, and a DATA | FEATURE header without a report payload produced
one. Patch 1 removed only the first three reports; applying both patches
removed all four.

The tests also placed a controlled byte after the declared L2CAP PDU. A
trailing 0x15 after a zero-length control SDU was interpreted as virtual
cable unplug and terminated the unpatched HIDP session. The patched session
rejected the frame and completed a later feature report request. A trailing
report ID after a header-only DATA response made the unpatched raw-report
request complete with a zero-byte result; the patched kernel rejected it.

Finally, each path received 10,000 malformed responses on KASAN and UBSAN
kernels. No KASAN, UBSAN, Oops, or kernel BUG was observed. The KASAN runs
and all patched UBSAN runs accepted a subsequent valid feature report. The
unpatched UBSAN numbered-report run instead completed requests from queued
short responses, which further exposed the missing payload validation. No
information disclosure or code execution is claimed.

Sangho Lee (2):
  Bluetooth: HIDP: reject frames without a transaction header
  Bluetooth: HIDP: validate numbered report payloads

 net/bluetooth/hidp/core.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

-- 
2.43.0

[PATCH 1/2] Bluetooth: HIDP: reject frames without a transaction header

[Sangho Lee]

hidp_recv_ctrl_frame() and hidp_recv_intr_frame() read skb->data[0]
before checking that the L2CAP SDU contains a transaction header. A
connected HIDP peer can send an empty basic-mode SDU and make both paths
use an uninitialized byte from skb tailroom.

KMSAN reports the use in hidp_session_run(), with the uninitialized value
originating in __alloc_skb() through vhci_write(). The control path
produces two reports and the interrupt path produces one.

The byte can also be controlled by a malformed lower-layer packet. If an
HCI ACL packet contains an L2CAP PDU with a declared zero-length payload
followed by an extra 0x15 byte, l2cap_recv_acldata() reduces skb->len to
the declared PDU length before dispatch. The current HIDP path nevertheless
consumes the extra byte as HIDP_TRANS_HID_CONTROL |
HIDP_CTRL_VIRTUAL_CABLE_UNPLUG and terminates the HIDP session. With this
change, the same packet is discarded and a subsequent feature report
request succeeds.

Pull the transaction header with skb_pull_data() and discard frames that
do not contain it.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Sangho Lee <kudo3228@gmail.com>
---
 net/bluetooth/hidp/core.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 0e24c5e2955e..194208d03d18 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -560,16 +560,18 @@ static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
 static void hidp_recv_ctrl_frame(struct hidp_session *session,
 					struct sk_buff *skb)
 {
-	unsigned char hdr, type, param;
+	unsigned char type, param;
+	u8 *hdr;
 	int free_skb = 1;
 
 	BT_DBG("session %p skb %p len %u", session, skb, skb->len);
 
-	hdr = skb->data[0];
-	skb_pull(skb, 1);
+	hdr = skb_pull_data(skb, 1);
+	if (!hdr)
+		goto free;
 
-	type = hdr & HIDP_HEADER_TRANS_MASK;
-	param = hdr & HIDP_HEADER_PARAM_MASK;
+	type = *hdr & HIDP_HEADER_TRANS_MASK;
+	param = *hdr & HIDP_HEADER_PARAM_MASK;
 
 	switch (type) {
 	case HIDP_TRANS_HANDSHAKE:
@@ -590,6 +592,7 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
 		break;
 	}
 
+free:
 	if (free_skb)
 		kfree_skb(skb);
 }
@@ -597,14 +600,15 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
 static void hidp_recv_intr_frame(struct hidp_session *session,
 				struct sk_buff *skb)
 {
-	unsigned char hdr;
+	u8 *hdr;
 
 	BT_DBG("session %p skb %p len %u", session, skb, skb->len);
 
-	hdr = skb->data[0];
-	skb_pull(skb, 1);
+	hdr = skb_pull_data(skb, 1);
+	if (!hdr)
+		goto free;
 
-	if (hdr == (HIDP_TRANS_DATA | HIDP_DATA_RTYPE_INPUT)) {
+	if (*hdr == (HIDP_TRANS_DATA | HIDP_DATA_RTYPE_INPUT)) {
 		hidp_set_timer(session);
 
 		if (session->input)
@@ -616,9 +620,10 @@ static void hidp_recv_intr_frame(struct hidp_session *session,
 			BT_DBG("report len %d", skb->len);
 		}
 	} else {
-		BT_DBG("Unsupported protocol header 0x%02x", hdr);
+		BT_DBG("Unsupported protocol header 0x%02x", *hdr);
 	}
 
+free:
 	kfree_skb(skb);
 }
 
-- 
2.43.0

[PATCH 2/2] Bluetooth: HIDP: validate numbered report payloads

[Sangho Lee]

When hidp_get_raw_report() waits for a numbered report,
hidp_process_data() compares the expected report number with skb->data[0].
A connected HIDP peer can reply with only a DATA transaction header,
leaving the skb empty after the header is removed.

KMSAN reports an uninitialized-value use in hidp_session_run(), with the
value originating in __alloc_skb() through vhci_write(). The transaction
header checks remove the empty-frame reports, but this report remains until
the payload check is added.

The comparison can also consume a peer-controlled byte beyond the declared
L2CAP PDU. A DATA | FEATURE response followed by an extra 0x01 byte made
the current code accept that byte as report ID 1 and complete
HIDIOCGFEATURE with a zero-byte result. With this change the malformed
response is rejected with -EIO, while a subsequent valid response still
succeeds.

Require a payload byte before comparing a numbered report ID. Unnumbered
reports continue to accept an empty payload.

Fixes: 0ff1731a1ae5 ("HID: bt: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE")
Cc: stable@vger.kernel.org
Signed-off-by: Sangho Lee <kudo3228@gmail.com>
---
 net/bluetooth/hidp/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 194208d03d18..f5bdf9f1ca63 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -543,9 +543,10 @@ static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
 	}
 
 	if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags) &&
-				param == session->waiting_report_type) {
+	    param == session->waiting_report_type) {
 		if (session->waiting_report_number < 0 ||
-		    session->waiting_report_number == skb->data[0]) {
+		    (skb->len &&
+		     session->waiting_report_number == skb->data[0])) {
 			/* hidp_get_raw_report() is waiting on this report. */
 			session->report_return = skb;
 			done_with_skb = 0;
-- 
2.43.0

RE: Bluetooth: HIDP: validate short receive frames

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2389 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1132870

---Test result---

Test Summary:
CheckPatch                    PASS      1.15 seconds
VerifyFixes                   PASS      0.10 seconds
VerifySignedoff               PASS      0.10 seconds
GitLint                       PASS      0.52 seconds
SubjectPrefix                 PASS      0.33 seconds
BuildKernel                   PASS      21.22 seconds
CheckAllWarning               PASS      23.42 seconds
CheckSparse                   PASS      22.62 seconds
BuildKernel32                 PASS      20.69 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      378.79 seconds
TestRunner_l2cap-tester       PASS      54.95 seconds
TestRunner_iso-tester         PASS      78.97 seconds
TestRunner_bnep-tester        PASS      15.98 seconds
TestRunner_mgmt-tester        FAIL      205.83 seconds
TestRunner_rfcomm-tester      PASS      22.50 seconds
TestRunner_sco-tester         PASS      28.89 seconds
TestRunner_ioctl-tester       PASS      23.71 seconds
TestRunner_mesh-tester        FAIL      23.67 seconds
TestRunner_smp-tester         PASS      20.84 seconds
TestRunner_userchan-tester    PASS      17.63 seconds
TestRunner_6lowpan-tester     PASS      20.98 seconds
IncrementalBuild              PASS      23.07 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 494, Passed: 489 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.253 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.049 seconds
Mesh - Send cancel - 2                               Timed out    1.986 seconds


https://github.com/bluez/bluetooth-next/pull/479

---
Regards,
Linux Bluetooth