[PATCH v1 1/1] Bluetooth: L2CAP: fix heap over-read in l2cap_get_conf_opt

[PATCH v1 1/1] Bluetooth: L2CAP: fix heap over-read in l2cap_get_conf_opt

[Muhammad Bilal]

l2cap_get_conf_opt() reads opt->val via a switch on opt->len (1, 2,
or 4 bytes).  opt->len is a remote-controlled u8.  All three callers
loop on (len >= L2CAP_CONF_OPT_SIZE), so the loop body executes with
as few as 2 bytes remaining.  A packet ending with opt->len=4 and
only 2 bytes left causes get_unaligned_le32(opt->val) to read 4 bytes
past the buffer before the caller can act on the return value.

Commit 7c9cbd0b5e38 ("Bluetooth: Verify that l2cap_get_conf_opt
provides large enough buffer") added a post-call len < 0 guard in
each caller, but the over-read fires inside l2cap_get_conf_opt()
before that guard is reached.

Add a buflen parameter and validate L2CAP_CONF_OPT_SIZE + opt->len
<= buflen before any access to opt->val.  Return -EINVAL on
violation.  Update all three callers to capture the return value and
break on negative.  With the bounds check ensuring the option fits
within the remaining buffer, the post-call len < 0 check is no
longer needed and is removed.

Fixes: 7c9cbd0b5e38 ("Bluetooth: Verify that l2cap_get_conf_opt provides large enough buffer")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 net/bluetooth/l2cap_core.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index fdccd62ccca8..6052ffb280ac 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3051,12 +3051,23 @@ static struct sk_buff *l2cap_build_cmd(struct l2cap_conn *conn, u8 code,
 }
 
 static inline int l2cap_get_conf_opt(void **ptr, int *type, int *olen,
-				     unsigned long *val)
+				     unsigned long *val, size_t buflen)
 {
 	struct l2cap_conf_opt *opt = *ptr;
 	int len;
 
+	/* Guard opt->len dereference: reject if the 2-byte option header
+	 * itself does not fit in the remaining buffer.
+	 */
+	if (buflen < L2CAP_CONF_OPT_SIZE)
+		return -EINVAL;
+
 	len = L2CAP_CONF_OPT_SIZE + opt->len;
+
+	/* Reject options whose payload extends past the remaining buffer. */
+	if ((size_t)len > buflen)
+		return -EINVAL;
+
 	*ptr += len;
 
 	*type = opt->type;
@@ -3437,9 +3448,11 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data
 	BT_DBG("chan %p", chan);
 
 	while (len >= L2CAP_CONF_OPT_SIZE) {
-		len -= l2cap_get_conf_opt(&req, &type, &olen, &val);
-		if (len < 0)
+		int optlen = l2cap_get_conf_opt(&req, &type, &olen, &val, len);
+
+		if (optlen < 0)
 			break;
+		len -= optlen;
 
 		hint  = type & L2CAP_CONF_HINT;
 		type &= L2CAP_CONF_MASK;
@@ -3675,9 +3688,11 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len,
 	BT_DBG("chan %p, rsp %p, len %d, req %p", chan, rsp, len, data);
 
 	while (len >= L2CAP_CONF_OPT_SIZE) {
-		len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val);
-		if (len < 0)
+		int optlen = l2cap_get_conf_opt(&rsp, &type, &olen, &val, len);
+
+		if (optlen < 0)
 			break;
+		len -= optlen;
 
 		switch (type) {
 		case L2CAP_CONF_MTU:
@@ -3946,9 +3961,11 @@ static void l2cap_conf_rfc_get(struct l2cap_chan *chan, void *rsp, int len)
 		return;
 
 	while (len >= L2CAP_CONF_OPT_SIZE) {
-		len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val);
-		if (len < 0)
+		int optlen = l2cap_get_conf_opt(&rsp, &type, &olen, &val, len);
+
+		if (optlen < 0)
 			break;
+		len -= optlen;
 
 		switch (type) {
 		case L2CAP_CONF_RFC:
-- 
2.53.0

RE: [v1,1/1] Bluetooth: L2CAP: fix heap over-read in l2cap_get_conf_opt

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PASS      0.70 seconds
VerifyFixes                   PASS      0.11 seconds
VerifySignedoff               PASS      0.12 seconds
GitLint                       PASS      0.29 seconds
SubjectPrefix                 PASS      0.11 seconds
BuildKernel                   PASS      26.93 seconds
CheckAllWarning               PASS      29.76 seconds
CheckSparse                   PASS      28.95 seconds
BuildKernel32                 PASS      26.00 seconds
TestRunnerSetup               PASS      572.85 seconds
TestRunner_l2cap-tester       PASS      59.10 seconds
IncrementalBuild              PASS      25.01 seconds



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

---
Regards,
Linux Bluetooth

Re: [PATCH v1 1/1] Bluetooth: L2CAP: fix heap over-read in l2cap_get_conf_opt

[Paul Menzel]

Dear Muhammad,


Thank you for your patch.

Am 26.05.26 um 04:17 schrieb Muhammad Bilal:
> l2cap_get_conf_opt() reads opt->val via a switch on opt->len (1, 2,
> or 4 bytes).  opt->len is a remote-controlled u8.  All three callers
> loop on (len >= L2CAP_CONF_OPT_SIZE), so the loop body executes with
> as few as 2 bytes remaining.  A packet ending with opt->len=4 and
> only 2 bytes left causes get_unaligned_le32(opt->val) to read 4 bytes
> past the buffer before the caller can act on the return value.
> 
> Commit 7c9cbd0b5e38 ("Bluetooth: Verify that l2cap_get_conf_opt
> provides large enough buffer") added a post-call len < 0 guard in
> each caller, but the over-read fires inside l2cap_get_conf_opt()
> before that guard is reached.
> 
> Add a buflen parameter and validate L2CAP_CONF_OPT_SIZE + opt->len
> <= buflen before any access to opt->val.  Return -EINVAL on
> violation.  Update all three callers to capture the return value and
> break on negative.  With the bounds check ensuring the option fits
> within the remaining buffer, the post-call len < 0 check is no
> longer needed and is removed.

By any chance, do you have a reproducer?

> Fixes: 7c9cbd0b5e38 ("Bluetooth: Verify that l2cap_get_conf_opt provides large enough buffer")
> Cc: stable@vger.kernel.org
> Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
> ---
>   net/bluetooth/l2cap_core.c | 31 ++++++++++++++++++++++++-------
>   1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index fdccd62ccca8..6052ffb280ac 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -3051,12 +3051,23 @@ static struct sk_buff *l2cap_build_cmd(struct l2cap_conn *conn, u8 code,
>   }
>   
>   static inline int l2cap_get_conf_opt(void **ptr, int *type, int *olen,
> -				     unsigned long *val)
> +				     unsigned long *val, size_t buflen)
>   {
>   	struct l2cap_conf_opt *opt = *ptr;
>   	int len;
>   
> +	/* Guard opt->len dereference: reject if the 2-byte option header
> +	 * itself does not fit in the remaining buffer.
> +	 */
> +	if (buflen < L2CAP_CONF_OPT_SIZE)
> +		return -EINVAL;

I always wonder, if Linux should log a debug message or even warning.

> +
>   	len = L2CAP_CONF_OPT_SIZE + opt->len;
> +
> +	/* Reject options whose payload extends past the remaining buffer. */
> +	if ((size_t)len > buflen)
> +		return -EINVAL;

Ditto.

> +
>   	*ptr += len;
>   
>   	*type = opt->type;
> @@ -3437,9 +3448,11 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data
>   	BT_DBG("chan %p", chan);
>   
>   	while (len >= L2CAP_CONF_OPT_SIZE) {
> -		len -= l2cap_get_conf_opt(&req, &type, &olen, &val);
> -		if (len < 0)
> +		int optlen = l2cap_get_conf_opt(&req, &type, &olen, &val, len);
> +
> +		if (optlen < 0)
>   			break;
> +		len -= optlen;
>   
>   		hint  = type & L2CAP_CONF_HINT;
>   		type &= L2CAP_CONF_MASK;
> @@ -3675,9 +3688,11 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len,
>   	BT_DBG("chan %p, rsp %p, len %d, req %p", chan, rsp, len, data);
>   
>   	while (len >= L2CAP_CONF_OPT_SIZE) {
> -		len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val);
> -		if (len < 0)
> +		int optlen = l2cap_get_conf_opt(&rsp, &type, &olen, &val, len);
> +
> +		if (optlen < 0)
>   			break;
> +		len -= optlen;
>   
>   		switch (type) {
>   		case L2CAP_CONF_MTU:
> @@ -3946,9 +3961,11 @@ static void l2cap_conf_rfc_get(struct l2cap_chan *chan, void *rsp, int len)
>   		return;
>   
>   	while (len >= L2CAP_CONF_OPT_SIZE) {
> -		len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val);
> -		if (len < 0)
> +		int optlen = l2cap_get_conf_opt(&rsp, &type, &olen, &val, len);
> +
> +		if (optlen < 0)
>   			break;
> +		len -= optlen;
>   
>   		switch (type) {
>   		case L2CAP_CONF_RFC:

The diff looks good.

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [PATCH v1 1/1] Bluetooth: L2CAP: fix heap over-read in l2cap_get_conf_opt

[Muhammad Bilal]

Thanks for the review.

> By any chance, do you have a reproducer?

No standalone reproducer is available. The issue can be triggered by
a malformed L2CAP configuration request where opt->len exceeds the
remaining buffer, i.e. a crafted packet from a remote peer.

> I always wonder, if Linux should log a debug message or even warning.

Existing callers generally handle malformed configuration options by
silently aborting parsing, so I followed the same pattern. Adding a
BT_ERR() on -EINVAL could be reasonable; I can include that in a v2
if preferred.

Regards,
Muhammad Bilal

Re: [PATCH v1 1/1] Bluetooth: L2CAP: fix heap over-read in l2cap_get_conf_opt

[Paul Menzel]

Dear Muhammad,


Am 27.05.26 um 07:18 schrieb Muhammad Bilal:

>> By any chance, do you have a reproducer?
> 
> No standalone reproducer is available. The issue can be triggered by
> a malformed L2CAP configuration request where opt->len exceeds the
> remaining buffer, i.e. a crafted packet from a remote peer.

Understood.

>> I always wonder, if Linux should log a debug message or even warning.
> 
> Existing callers generally handle malformed configuration options by
> silently aborting parsing, so I followed the same pattern. Adding a
> BT_ERR() on -EINVAL could be reasonable; I can include that in a v2
> if preferred.

Thank you for sharing the reasoning. It makes sense, and no need to add 
it then.


Kind regards,

Paul