From: Luiz Augusto von Dentz <[email protected]>
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 <[email protected]>
---
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
Hi Stephen,
On Tue, May 24, 2022 at 1:26 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> From: Luiz Augusto von Dentz <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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
Hi Tom,
On Thu, May 26, 2022 at 1:50 PM Tom Unbehau <[email protected]> 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
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