Bluetooth: MGMT: Fix OOB access in parse_adv_monitor_pattern()

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

Posted by Ilia Gavrilov 3 months, 3 weeks ago

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 <Ilia.Gavrilov@infotecs.ru>
---
 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];
 } __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

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

Posted by Luiz Augusto von Dentz 3 months, 2 weeks ago

Hi Ilia,

On Mon, Oct 20, 2025 at 11:12 AM Ilia Gavrilov
<Ilia.Gavrilov@infotecs.ru> wrote:
>
> 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 <Ilia.Gavrilov@infotecs.ru>
> ---
>  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.

>  } __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



-- 
Luiz Augusto von Dentz

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

Posted by Ilia Gavrilov 3 months, 2 weeks ago

Hi, Luiz, thank you for the review.

On 10/23/25 16:18, Luiz Augusto von Dentz wrote:
> Hi Ilia,
> 
> On Mon, Oct 20, 2025 at 11:12 AM Ilia Gavrilov
> <Ilia.Gavrilov@infotecs.ru> wrote:
>>
>> 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 <Ilia.Gavrilov@infotecs.ru>
>> ---
>>  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;
...


>>  } __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
> 
> 
>

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

Posted by Luiz Augusto von Dentz 3 months, 2 weeks ago

Hi Ilia,

On Thu, Oct 23, 2025 at 11:08 AM Ilia Gavrilov
<Ilia.Gavrilov@infotecs.ru> wrote:
>
> Hi, Luiz, thank you for the review.
>
> On 10/23/25 16:18, Luiz Augusto von Dentz wrote:
> > Hi Ilia,
> >
> > On Mon, Oct 20, 2025 at 11:12 AM Ilia Gavrilov
> > <Ilia.Gavrilov@infotecs.ru> wrote:
> >>
> >> 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 <Ilia.Gavrilov@infotecs.ru>
> >> ---
> >>  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? 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

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.

>
> >>  } __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
> >
> >
> >
>


-- 
Luiz Augusto von Dentz

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

Posted by Ilia Gavrilov 3 months, 2 weeks ago

On 10/23/25 18:30, Luiz Augusto von Dentz wrote:
> Hi Ilia,
> 
> On Thu, Oct 23, 2025 at 11:08 AM Ilia Gavrilov
> <Ilia.Gavrilov@infotecs.ru> wrote:
>>
>> Hi, Luiz, thank you for the review.
>>
>> On 10/23/25 16:18, Luiz Augusto von Dentz wrote:
>>> Hi Ilia,
>>>
>>> On Mon, Oct 20, 2025 at 11:12 AM Ilia Gavrilov
>>> <Ilia.Gavrilov@infotecs.ru> wrote:
>>>>
>>>> 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 <Ilia.Gavrilov@infotecs.ru>
>>>> ---
>>>>  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.
> 
>>
>>>>  } __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
>>>
>>>
>>>
>>
> 
>