* [PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name}
@ 2022-05-24 20:26 Luiz Augusto von Dentz
2022-05-24 20:55 ` Luiz Augusto von Dentz
2022-05-24 21:06 ` bluez.test.bot
0 siblings, 2 replies; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2022-05-24 20:26 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Both dev_name and short_name are not guaranteed to be NULL terminated so
this instead use strnlen and then attempt to determine if the resulting
string needs to be truncated or not.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216018
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
net/bluetooth/eir.c | 41 ++++++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/net/bluetooth/eir.c b/net/bluetooth/eir.c
index 7e930f77ecab..4171edee88e4 100644
--- a/net/bluetooth/eir.c
+++ b/net/bluetooth/eir.c
@@ -13,6 +13,20 @@
#define PNP_INFO_SVCLASS_ID 0x1200
+static u8 eir_append_name(u8 *eir, u16 eir_len, u8 type, u8 *data, u8 data_len)
+{
+ u8 name[HCI_MAX_SHORT_NAME_LENGTH + 1];
+
+ /* If data is already NULL terminated just pass it directly */
+ if (data[data_len - 1] == '\0')
+ return eir_append_data(eir, eir_len, type, data, data_len);
+
+ memcpy(name, data, HCI_MAX_SHORT_NAME_LENGTH);
+ name[HCI_MAX_SHORT_NAME_LENGTH] = '\0';
+
+ return eir_append_data(eir, eir_len, type, name, sizeof(name));
+}
+
u8 eir_append_local_name(struct hci_dev *hdev, u8 *ptr, u8 ad_len)
{
size_t short_len;
@@ -23,29 +37,26 @@ u8 eir_append_local_name(struct hci_dev *hdev, u8 *ptr, u8 ad_len)
return ad_len;
/* use complete name if present and fits */
- complete_len = strlen(hdev->dev_name);
+ complete_len = strnlen(hdev->dev_name, sizeof(hdev->dev_name));
if (complete_len && complete_len <= HCI_MAX_SHORT_NAME_LENGTH)
- return eir_append_data(ptr, ad_len, EIR_NAME_COMPLETE,
+ return eir_append_name(ptr, ad_len, EIR_NAME_COMPLETE,
hdev->dev_name, complete_len + 1);
/* use short name if present */
- short_len = strlen(hdev->short_name);
+ short_len = strnlen(hdev->short_name, sizeof(hdev->short_name));
if (short_len)
- return eir_append_data(ptr, ad_len, EIR_NAME_SHORT,
- hdev->short_name, short_len + 1);
+ return eir_append_name(ptr, ad_len, EIR_NAME_SHORT,
+ hdev->short_name,
+ short_len == HCI_MAX_SHORT_NAME_LENGTH ?
+ short_len : short_len + 1);
/* use shortened full name if present, we already know that name
* is longer then HCI_MAX_SHORT_NAME_LENGTH
*/
- if (complete_len) {
- u8 name[HCI_MAX_SHORT_NAME_LENGTH + 1];
-
- memcpy(name, hdev->dev_name, HCI_MAX_SHORT_NAME_LENGTH);
- name[HCI_MAX_SHORT_NAME_LENGTH] = '\0';
-
- return eir_append_data(ptr, ad_len, EIR_NAME_SHORT, name,
- sizeof(name));
- }
+ if (complete_len)
+ return eir_append_name(ptr, ad_len, EIR_NAME_SHORT,
+ hdev->dev_name,
+ HCI_MAX_SHORT_NAME_LENGTH);
return ad_len;
}
@@ -168,7 +179,7 @@ void eir_create(struct hci_dev *hdev, u8 *data)
u8 *ptr = data;
size_t name_len;
- name_len = strlen(hdev->dev_name);
+ name_len = strnlen(hdev->dev_name, sizeof(hdev->dev_name));
if (name_len > 0) {
/* EIR Data type */
--
2.35.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name}
2022-05-24 20:26 [PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name} Luiz Augusto von Dentz
@ 2022-05-24 20:55 ` Luiz Augusto von Dentz
2022-05-24 21:21 ` Stephen Hemminger
2022-05-24 21:06 ` bluez.test.bot
1 sibling, 1 reply; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2022-05-24 20:55 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Stephen Hemminger
Hi Stephen,
On Tue, May 24, 2022 at 1:26 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Both dev_name and short_name are not guaranteed to be NULL terminated so
> this instead use strnlen and then attempt to determine if the resulting
> string needs to be truncated or not.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216018
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> net/bluetooth/eir.c | 41 ++++++++++++++++++++++++++---------------
> 1 file changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/net/bluetooth/eir.c b/net/bluetooth/eir.c
> index 7e930f77ecab..4171edee88e4 100644
> --- a/net/bluetooth/eir.c
> +++ b/net/bluetooth/eir.c
> @@ -13,6 +13,20 @@
>
> #define PNP_INFO_SVCLASS_ID 0x1200
>
> +static u8 eir_append_name(u8 *eir, u16 eir_len, u8 type, u8 *data, u8 data_len)
> +{
> + u8 name[HCI_MAX_SHORT_NAME_LENGTH + 1];
> +
> + /* If data is already NULL terminated just pass it directly */
> + if (data[data_len - 1] == '\0')
> + return eir_append_data(eir, eir_len, type, data, data_len);
> +
> + memcpy(name, data, HCI_MAX_SHORT_NAME_LENGTH);
> + name[HCI_MAX_SHORT_NAME_LENGTH] = '\0';
> +
> + return eir_append_data(eir, eir_len, type, name, sizeof(name));
> +}
> +
> u8 eir_append_local_name(struct hci_dev *hdev, u8 *ptr, u8 ad_len)
> {
> size_t short_len;
> @@ -23,29 +37,26 @@ u8 eir_append_local_name(struct hci_dev *hdev, u8 *ptr, u8 ad_len)
> return ad_len;
>
> /* use complete name if present and fits */
> - complete_len = strlen(hdev->dev_name);
> + complete_len = strnlen(hdev->dev_name, sizeof(hdev->dev_name));
> if (complete_len && complete_len <= HCI_MAX_SHORT_NAME_LENGTH)
> - return eir_append_data(ptr, ad_len, EIR_NAME_COMPLETE,
> + return eir_append_name(ptr, ad_len, EIR_NAME_COMPLETE,
> hdev->dev_name, complete_len + 1);
>
> /* use short name if present */
> - short_len = strlen(hdev->short_name);
> + short_len = strnlen(hdev->short_name, sizeof(hdev->short_name));
> if (short_len)
> - return eir_append_data(ptr, ad_len, EIR_NAME_SHORT,
> - hdev->short_name, short_len + 1);
> + return eir_append_name(ptr, ad_len, EIR_NAME_SHORT,
> + hdev->short_name,
> + short_len == HCI_MAX_SHORT_NAME_LENGTH ?
> + short_len : short_len + 1);
>
> /* use shortened full name if present, we already know that name
> * is longer then HCI_MAX_SHORT_NAME_LENGTH
> */
> - if (complete_len) {
> - u8 name[HCI_MAX_SHORT_NAME_LENGTH + 1];
> -
> - memcpy(name, hdev->dev_name, HCI_MAX_SHORT_NAME_LENGTH);
> - name[HCI_MAX_SHORT_NAME_LENGTH] = '\0';
> -
> - return eir_append_data(ptr, ad_len, EIR_NAME_SHORT, name,
> - sizeof(name));
> - }
> + if (complete_len)
> + return eir_append_name(ptr, ad_len, EIR_NAME_SHORT,
> + hdev->dev_name,
> + HCI_MAX_SHORT_NAME_LENGTH);
>
> return ad_len;
> }
> @@ -168,7 +179,7 @@ void eir_create(struct hci_dev *hdev, u8 *data)
> u8 *ptr = data;
> size_t name_len;
>
> - name_len = strlen(hdev->dev_name);
> + name_len = strnlen(hdev->dev_name, sizeof(hdev->dev_name));
>
> if (name_len > 0) {
> /* EIR Data type */
> --
> 2.35.1
Here is a tentative fix, I do wonder though why you were trying to set
the name directly and not using the likes of bluetoothctl/btmgmt?
bluetoothd don't seem to bother setting a shortname so it is probably
not reproducible with it but btmgmt does:
[mgmt]# name "This is a long name" "Short Name"
[mgmt]# @ MGMT Command: Set Local... (0x000f) plen 260 {0x0001}
[hci0] 13:37:27.052763
Name: This is a long name
Short name: Short Name
@ MGMT Event: Command Comp.. (0x0001) plen 263 {0x0001} [hci0] 13:37:27.053224
Set Local Name (0x000f) plen 260
Status: Success (0x00)
Name: This is a long name
Short name: Short Name
Anyway it looks like one needs to be advertising in order to trigger
the problem but with the above changes it doesn't crash anymore:
@ MGMT Command: Add Extende.. (0x0055) plen 14 {0x0001} [hci0] 13:53:28.130215
Instance: 1
Advertising data length: 3
Flags: 0x06
LE General Discoverable Mode
BR/EDR Not Supported
Scan response length: 0
< HCI Command: LE Set Exten.. (0x08|0x0037) plen 7 #119 [hci0] 13:53:28.130215
Handle: 0x01
Operation: Complete extended advertising data (0x03)
Fragment preference: Minimize fragmentation (0x01)
Data length: 0x03
Flags: 0x06
LE General Discoverable Mode
BR/EDR Not Supported
> HCI Event: Command Complete (0x0e) plen 4 #120 [hci0] 13:53:28.130215
LE Set Extended Advertising Data (0x08|0x0037) ncmd 1
Status: Success (0x00)
< HCI Command: LE Set Exte.. (0x08|0x0038) plen 17 #121 [hci0] 13:53:28.130215
Handle: 0x01
Operation: Complete scan response data (0x03)
Fragment preference: Minimize fragmentation (0x01)
Data length: 0x0d
Name (short): Short Name
> HCI Event: Command Complete (0x0e) plen 4 #122 [hci0] 13:53:28.130215
LE Set Extended Scan Response Data (0x08|0x0038) ncmd 1
Status: Success (0x00)
< HCI Command: LE Set Exten.. (0x08|0x0039) plen 6 #123 [hci0] 13:53:28.130215
Extended advertising: Enabled (0x01)
Number of sets: 1 (0x01)
Entry 0
Handle: 0x01
Duration: 0 ms (0x00)
Max ext adv events: 0
> HCI Event: Command Complete (0x0e) plen 4 #124 [hci0] 13:53:28.134215
LE Set Extended Advertising Enable (0x08|0x0039) ncmd 1
Status: Success (0x00)
@ MGMT Event: Command Complete (0x0001) plen 4 {0x0001} [hci0] 13:53:28.134215
Add Extended Advertising Data (0x0055) plen 1
Status: Success (0x00)
Instance: 1
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name}
2022-05-24 20:26 [PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name} Luiz Augusto von Dentz
2022-05-24 20:55 ` Luiz Augusto von Dentz
@ 2022-05-24 21:06 ` bluez.test.bot
1 sibling, 0 replies; 9+ messages in thread
From: bluez.test.bot @ 2022-05-24 21:06 UTC (permalink / raw)
To: linux-bluetooth, luiz.dentz
[-- Attachment #1: Type: text/plain, Size: 1098 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=644763
---Test result---
Test Summary:
CheckPatch PASS 1.55 seconds
GitLint PASS 1.01 seconds
SubjectPrefix PASS 1.02 seconds
BuildKernel PASS 42.01 seconds
BuildKernel32 PASS 28.58 seconds
Incremental Build with patchesPASS 39.46 seconds
TestRunner: Setup PASS 471.79 seconds
TestRunner: l2cap-tester PASS 17.46 seconds
TestRunner: bnep-tester PASS 6.04 seconds
TestRunner: mgmt-tester PASS 101.14 seconds
TestRunner: rfcomm-tester PASS 9.66 seconds
TestRunner: sco-tester PASS 9.32 seconds
TestRunner: smp-tester PASS 11.81 seconds
TestRunner: userchan-tester PASS 6.36 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name}
2022-05-24 20:55 ` Luiz Augusto von Dentz
@ 2022-05-24 21:21 ` Stephen Hemminger
2022-05-24 23:42 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Hemminger @ 2022-05-24 21:21 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
On Tue, 24 May 2022 13:55:57 -0700
Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> Here is a tentative fix, I do wonder though why you were trying to set
> the name directly and not using the likes of bluetoothctl/btmgmt?
> bluetoothd don't seem to bother setting a shortname so it is probably
> not reproducible with it but btmgmt does:
Not me. I just forward reports from bugzilla.
Any networking related bug reports end up going to me, and I filter/forward them.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name}
2022-05-24 21:21 ` Stephen Hemminger
@ 2022-05-24 23:42 ` Luiz Augusto von Dentz
2022-05-25 20:09 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2022-05-24 23:42 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: linux-bluetooth
Hi Stephen,
On Tue, May 24, 2022 at 2:21 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue, 24 May 2022 13:55:57 -0700
> Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>
> > Here is a tentative fix, I do wonder though why you were trying to set
> > the name directly and not using the likes of bluetoothctl/btmgmt?
> > bluetoothd don't seem to bother setting a shortname so it is probably
> > not reproducible with it but btmgmt does:
>
> Not me. I just forward reports from bugzilla.
> Any networking related bug reports end up going to me, and I filter/forward them.
Sorry, I guess we should forward this to Tom then.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name}
2022-05-24 23:42 ` Luiz Augusto von Dentz
@ 2022-05-25 20:09 ` Luiz Augusto von Dentz
2022-05-26 20:50 ` Tom Unbehau
0 siblings, 1 reply; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2022-05-25 20:09 UTC (permalink / raw)
To: tomu1; +Cc: linux-bluetooth
Hi Tom,
On Tue, May 24, 2022 at 4:42 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Stephen,
>
> On Tue, May 24, 2022 at 2:21 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Tue, 24 May 2022 13:55:57 -0700
> > Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> >
> > > Here is a tentative fix, I do wonder though why you were trying to set
> > > the name directly and not using the likes of bluetoothctl/btmgmt?
> > > bluetoothd don't seem to bother setting a shortname so it is probably
> > > not reproducible with it but btmgmt does:
> >
> > Not me. I just forward reports from bugzilla.
> > Any networking related bug reports end up going to me, and I filter/forward them.
>
> Sorry, I guess we should forward this to Tom then.
Could you please check if the following change does fix the problem:
https://patchwork.kernel.org/project/bluetooth/patch/20220524202630.3569412-1-luiz.dentz@gmail.com/
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name}
2022-05-25 20:09 ` Luiz Augusto von Dentz
@ 2022-05-26 20:50 ` Tom Unbehau
2022-05-26 23:46 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 9+ messages in thread
From: Tom Unbehau @ 2022-05-26 20:50 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
[-- Attachment #1: Type: text/plain, Size: 802 bytes --]
Sending this mail again due to HTML mails not being allowed.
Hi Luiz,
The patch did not fix my issue. The issue described in the bugzilla ticket was an error
in the mgmt.c module. I do not see any direct correlation between your patch and the error i am encountering.
I have tried your patch on mainline (5.18) and got the same strlen bug when executing the example
program I have attached to the bugzilla ticket.
I think strlen in the mgmt.c module needs to be replaced by strnlen.
I have attached a patch with these changes to this mail. After applying this patch the
error could not be reproduced for me.
However, I am not sure, if the changes you have made in the eir.c module are also prudent and could fix
similar issues (I am not familiar with this).
Regards,
Tom Unbehau
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: btfix2.patch --]
[-- Type: text/x-patch; name="btfix2.patch", Size: 725 bytes --]
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index d2d390534e54..8e528fad2264 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1082,11 +1082,11 @@ static u16 append_eir_data_to_buf(struct hci_dev *hdev, u8 *eir)
eir_len = eir_append_le16(eir, eir_len, EIR_APPEARANCE,
hdev->appearance);
- name_len = strlen(hdev->dev_name);
+ name_len = strnlen(hdev->dev_name, sizeof(hdev->dev_name));
eir_len = eir_append_data(eir, eir_len, EIR_NAME_COMPLETE,
hdev->dev_name, name_len);
- name_len = strlen(hdev->short_name);
+ name_len = strnlen(hdev->short_name, sizeof(hdev->short_name));
eir_len = eir_append_data(eir, eir_len, EIR_NAME_SHORT,
hdev->short_name, name_len);
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name}
2022-05-26 20:50 ` Tom Unbehau
@ 2022-05-26 23:46 ` Luiz Augusto von Dentz
2022-05-27 20:43 ` Tom Unbehau
0 siblings, 1 reply; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2022-05-26 23:46 UTC (permalink / raw)
To: Tom Unbehau; +Cc: linux-bluetooth
Hi Tom,
On Thu, May 26, 2022 at 1:50 PM Tom Unbehau <Tom.Unbehau@verifone.com> wrote:
>
> Sending this mail again due to HTML mails not being allowed.
>
> Hi Luiz,
>
> The patch did not fix my issue. The issue described in the bugzilla ticket was an error
> in the mgmt.c module. I do not see any direct correlation between your patch and the error i am encountering.
> I have tried your patch on mainline (5.18) and got the same strlen bug when executing the example
> program I have attached to the bugzilla ticket.
>
> I think strlen in the mgmt.c module needs to be replaced by strnlen.
> I have attached a patch with these changes to this mail. After applying this patch the
> error could not be reproduced for me.
Well looks like there are multiple places actually using strlen rather
than strnlen with the likes of dev_name and short_name.
> However, I am not sure, if the changes you have made in the eir.c module are also prudent and could fix
> similar issues (I am not familiar with this).
>
> Regards,
> Tom Unbehau
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name}
2022-05-26 23:46 ` Luiz Augusto von Dentz
@ 2022-05-27 20:43 ` Tom Unbehau
0 siblings, 0 replies; 9+ messages in thread
From: Tom Unbehau @ 2022-05-27 20:43 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
I agree with you 100%. I think a look needs to be taken at any place where the dev_name/short_name fields of the
hci_dev structure are used.
Perhaps there are even other places which assume these names are NUL terminated, but which are not as
obvious as using strlen on them.
Regards,
Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-05-27 20:43 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-24 20:26 [PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name} Luiz Augusto von Dentz
2022-05-24 20:55 ` Luiz Augusto von Dentz
2022-05-24 21:21 ` Stephen Hemminger
2022-05-24 23:42 ` Luiz Augusto von Dentz
2022-05-25 20:09 ` Luiz Augusto von Dentz
2022-05-26 20:50 ` Tom Unbehau
2022-05-26 23:46 ` Luiz Augusto von Dentz
2022-05-27 20:43 ` Tom Unbehau
2022-05-24 21:06 ` bluez.test.bot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.