[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: [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

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

[Paul Menzel]

Dear Muhammad,


Am 17.05.26 um 21:03 schrieb Muhammad Bilal:

> 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).

Indeed, you are right.

> 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.

It looks good to me. Hopefully the maintainers agree.


Kind regards,

Paul

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

[Luiz Augusto von Dentz]

Hi Muhammad,

On Sun, May 17, 2026 at 10:55 AM Muhammad Bilal <meatuni001@gmail.com> wrote:
>
> 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;

Perhaps we should stop assigning it directly and instead just use
`skb_pull_data`, which performs bounds checks on its own.

> +       if (skb->len < sizeof(*kp))
> +               return SMP_INVALID_PARAMS;

I suggested we add a bt_dev_warn_ratelimit with something like "Too
small packet: skb->len %u < %u" to make debugging easier.

> +
>         bt_dev_dbg(conn->hcon->hdev, "value 0x%02x", kp->value);
>
>         return 0;
> --
> 2.54.0
>


-- 
Luiz Augusto von Dentz

[PATCH v2] 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.

Use skb_pull_data() to safely consume the payload; it performs
a bounds check internally and returns NULL when the packet is too
short.  Add a ratelimited warning in that path to aid debugging
of malformed packets, matching the pattern used by hci_event.c.

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 | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 98f1da4f5..1b237e623 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -2930,7 +2930,15 @@ static int smp_cmd_dhkey_check(struct l2cap_conn *conn, struct sk_buff *skb)
 static int smp_cmd_keypress_notify(struct l2cap_conn *conn,
 				   struct sk_buff *skb)
 {
-	struct smp_cmd_keypress_notify *kp = (void *) skb->data;
+	struct smp_cmd_keypress_notify *kp;
+
+	kp = skb_pull_data(skb, sizeof(struct smp_cmd_keypress_notify));
+	if (!kp) {
+		bt_dev_warn_ratelimited(conn->hcon->hdev,
+					"Too small packet: skb->len %u < %zu",
+					skb->len, sizeof(struct smp_cmd_keypress_notify));
+		return SMP_INVALID_PARAMS;
+	}
 
 	bt_dev_dbg(conn->hcon->hdev, "value 0x%02x", kp->value);
 
-- 
2.54.0