[PATCH] Bluetooth: SMP: add missing skb len check in smp_cmd_keypress_notify

[PATCH] Bluetooth: SMP: add missing skb len check in smp_cmd_keypress_notify

[Muhammad Bilal]

smp_cmd_keypress_notify() accesses the received payload as
struct smp_cmd_keypress_notify without verifying that skb->len
contains enough data.

smp_sig_channel() removes the opcode byte before dispatching to
command handlers, so a SMP_CMD_KEYPRESS_NOTIFY packet without a
payload leaves skb->len equal to zero on entry to the handler,
causing a 1-byte out-of-bounds read from the heap.

Add a length check before accessing the payload and return
SMP_INVALID_PARAMS when the packet is too short, matching the
pattern used by other SMP command handlers.

Fixes: 1408bb6efb04 ("Bluetooth: Add dummy handler for LE SC keypress notification")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 net/bluetooth/smp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 98f1da4f5..4c98e2a3a 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -2932,6 +2932,9 @@ static int smp_cmd_keypress_notify(struct l2cap_conn *conn,
 {
 	struct smp_cmd_keypress_notify *kp = (void *) skb->data;
 
+	if (skb->len < sizeof(*kp))
+		return SMP_INVALID_PARAMS;
+
 	bt_dev_dbg(conn->hcon->hdev, "value 0x%02x", kp->value);
 
 	return 0;
-- 
2.54.0

RE: Bluetooth: SMP: add missing skb len check in smp_cmd_keypress_notify

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 936 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=1096089

---Test result---

Test Summary:
CheckPatch                    PASS      0.74 seconds
GitLint                       PASS      0.35 seconds
SubjectPrefix                 PASS      0.13 seconds
BuildKernel                   PASS      28.81 seconds
CheckAllWarning               PASS      29.40 seconds
CheckSparse                   PASS      28.55 seconds
BuildKernel32                 PASS      26.07 seconds
TestRunnerSetup               PASS      576.08 seconds
TestRunner_smp-tester         PASS      18.39 seconds
IncrementalBuild              PASS      25.74 seconds



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

---
Regards,
Linux Bluetooth

Re: [PATCH] Bluetooth: SMP: add missing skb len check in smp_cmd_keypress_notify

[Paul Menzel]

Dear Muhammad,


Thank you for patch.

Am 17.05.26 um 16:54 schrieb Muhammad Bilal:
> smp_cmd_keypress_notify() accesses the received payload as
> struct smp_cmd_keypress_notify without verifying that skb->len
> contains enough data.
> 
> smp_sig_channel() removes the opcode byte before dispatching to
> command handlers, so a SMP_CMD_KEYPRESS_NOTIFY packet without a
> payload leaves skb->len equal to zero on entry to the handler,
> causing a 1-byte out-of-bounds read from the heap.
> 
> Add a length check before accessing the payload and return
> SMP_INVALID_PARAMS when the packet is too short, matching the
> pattern used by other SMP command handlers.
> 
> Fixes: 1408bb6efb04 ("Bluetooth: Add dummy handler for LE SC keypress notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
> ---
>   net/bluetooth/smp.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index 98f1da4f5..4c98e2a3a 100644
> --- a/net/bluetooth/smp.c
> +++ b/net/bluetooth/smp.c
> @@ -2932,6 +2932,9 @@ static int smp_cmd_keypress_notify(struct l2cap_conn *conn,
>   {
>   	struct smp_cmd_keypress_notify *kp = (void *) skb->data;
>   
> +	if (skb->len < sizeof(*kp))
> +		return SMP_INVALID_PARAMS;
> +
>   	bt_dev_dbg(conn->hcon->hdev, "value 0x%02x", kp->value);

Add the check after the debug log, so it can be inspected?

>   
>   	return 0;


Kind regards,

Paul

Re: [PATCH] Bluetooth: SMP: add missing skb len check in smp_cmd_keypress_notify

[Muhammad Bilal]

Hi Paul,

Thanks for the review.

Moving the check after bt_dev_dbg() would not be safe since the debug
statement reads kp->value, which is exactly what the length check is guarding.

On a truncated SMP_CMD_KEYPRESS_NOTIFY packet, skb->len may be smaller
than sizeof(*kp) when entering the handler, so evaluating kp->value in
the debug log would already access out-of-bounds memory before the
guard is reached.

Therefore the length check needs to remain before any access to
kp->value.

Regards,
Muhammad Bilal

Re: [PATCH] Bluetooth: SMP: add missing skb len check in smp_cmd_keypress_notify

[Paul Menzel]

Dear Muhammad,


Am 17.05.26 um 20:08 schrieb Muhammad Bilal:

> Thanks for the review.

Thank you for your instant reply.

> Moving the check after bt_dev_dbg() would not be safe since the debug
> statement reads kp->value, which is exactly what the length check is guarding.
> 
> On a truncated SMP_CMD_KEYPRESS_NOTIFY packet, skb->len may be smaller
> than sizeof(*kp) when entering the handler, so evaluating kp->value in
> the debug log would already access out-of-bounds memory before the
> guard is reached.
> 
> Therefore the length check needs to remain before any access to
> kp->value.

Thank you for the explanation. Is there another to log the faulty value?


Kind regards,

Paul

Re: [PATCH] Bluetooth: SMP: add missing skb len check in smp_cmd_keypress_notify

[Muhammad Bilal]

Hi Paul,

There is no safe way to access kp->value in the truncated case, since
the payload is not guaranteed to be present when skb->len < sizeof(*kp).

If diagnostic information is still useful, only metadata can be logged:

	if (skb->len < sizeof(*kp)) {
		bt_dev_dbg(conn->hcon->hdev,
			   "truncated keypress notify, len=%u",
			   skb->len);
		return SMP_INVALID_PARAMS;
	}

This keeps visibility into malformed packets without touching unvalidated
memory. Happy to send a v2 if that looks good to you.

Regards,
Muhammad Bilal