Thread (6 messages) 6 messages, 3 authors, 2025-10-31

Re: [PATCH net] Bluetooth: MGMT: Fix OOB access in parse_adv_monitor_pattern()

From: Ilia Gavrilov <hidden>
Date: 2025-10-24 08:56:53
Also in: linux-bluetooth, lkml, stable

On 10/23/25 18:30, Luiz Augusto von Dentz wrote:
Hi Ilia,

On Thu, Oct 23, 2025 at 11:08 AM Ilia Gavrilov
[off-list ref] wrote:
quoted
Hi, Luiz, thank you for the review.

On 10/23/25 16:18, Luiz Augusto von Dentz wrote:
quoted
Hi Ilia,

On Mon, Oct 20, 2025 at 11:12 AM Ilia Gavrilov
[off-list ref] wrote:
quoted
In the parse_adv_monitor_pattern() function, the value of
the 'length' variable is currently limited to HCI_MAX_EXT_AD_LENGTH(251).
The size of the 'value' array in the mgmt_adv_pattern structure is 31.
If the value of 'pattern[i].length' is set in the user space
and exceeds 31, the 'patterns[i].value' array can be accessed
out of bound when copied.

Increasing the size of the 'value' array in
the 'mgmt_adv_pattern' structure will break the userspace.
Considering this, and to avoid OOB access revert the limits for 'offset'
and 'length' back to the value of HCI_MAX_AD_LENGTH.

Found by InfoTeCS on behalf of Linux Verification Center
(linuxtesting.org) with SVACE.

Fixes: db08722fc7d4 ("Bluetooth: hci_core: Fix missing instances using HCI_MAX_AD_LENGTH")
Cc: stable@vger.kernel.org
Signed-off-by: Ilia Gavrilov <redacted>
---
 include/net/bluetooth/mgmt.h | 2 +-
 net/bluetooth/mgmt.c         | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 74edea06985b..4b07ce6dfd69 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -780,7 +780,7 @@ struct mgmt_adv_pattern {
        __u8 ad_type;
        __u8 offset;
        __u8 length;
-       __u8 value[31];
+       __u8 value[HCI_MAX_AD_LENGTH];
Why not use HCI_MAX_EXT_AD_LENGTH above? Or perhaps even make it
opaque since the actual size is defined by length - offset.
As I see it, user programs rely on this size of the structure, and if the size is changed, they will be broken.
Excerpt from bluez tools sources:
...
structure of mgmt_adv_pattern {
uint8_t ad type;
        uint8_t offset;
        length of uint8_t;
        uint8_t value[31];
} __packed;
...
Well it is broken for EA already, so the question is should we leave
it to just handle legacy advertisement or not? 
From a security point of view, it is better to fix the problem of OOB access of the 'value' array
in all kernels starting from version 6.6+.

There are two solutions: 
1) The simplest solution. Increase the size to the 'value' array in the 'mgmt_adv_pattern' structure,
but this type of command will not work:

$btmgmt
$menu monitor
$add-pattern 16:0:01020304050607080900

2) Do as suggested in this patch

 
At some point I was
actually just considering removing/deprecating the support of this
command altogether since there exists a standard way to do
advertisement monitoring called Monitoring Advertisers introduced in
6.0:

https://www.bluetooth.com/core-specification-6-feature-overview/?utm_source=internal&utm_medium=blog&utm_campaign=technical&utm_content=now-available-new-version-of-the-bluetooth-core-specification
It seems to me that it's better to remove/deprecate the support of the command
in the next version of the kernel.
The the standard monitoring list doesn't seem to be able to do
filtering on the data itself, which I think the where the decision
based filtering used, so it is not really compatible with the MS
vendor commands.
quoted
quoted
quoted
 } __packed;

 #define MGMT_OP_ADD_ADV_PATTERNS_MONITOR       0x0052
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index a3d16eece0d2..500033b70a96 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -5391,9 +5391,9 @@ static u8 parse_adv_monitor_pattern(struct adv_monitor *m, u8 pattern_count,
        for (i = 0; i < pattern_count; i++) {
                offset = patterns[i].offset;
                length = patterns[i].length;
-               if (offset >= HCI_MAX_EXT_AD_LENGTH ||
-                   length > HCI_MAX_EXT_AD_LENGTH ||
-                   (offset + length) > HCI_MAX_EXT_AD_LENGTH)
+               if (offset >= HCI_MAX_AD_LENGTH ||
+                   length > HCI_MAX_AD_LENGTH ||
+                   (offset + length) > HCI_MAX_AD_LENGTH)
                        return MGMT_STATUS_INVALID_PARAMS;

                p = kmalloc(sizeof(*p), GFP_KERNEL);
--
2.39.5
  
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help