[PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name}

Commit Message

Luiz Augusto von Dentz May 24, 2022, 8:26 p.m. UTC

From: Luiz Augusto von Dentz <[e-mail]>

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 <[e-mail]>
---
 net/bluetooth/eir.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

Comments

Luiz Augusto von Dentz May 24, 2022, 8:55 p.m. UTC | #1

Hi Stephen,

On Tue, May 24, 2022 at 1:26 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> From: Luiz Augusto von Dentz <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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

[e-mail] May 24, 2022, 9:06 p.m. UTC | #2

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

Stephen Hemminger May 24, 2022, 9:21 p.m. UTC | #3

On Tue, 24 May 2022 13:55:57 -0700
Luiz Augusto von Dentz <[e-mail]> 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.

Luiz Augusto von Dentz May 24, 2022, 11:42 p.m. UTC | #4

Hi Stephen,

On Tue, May 24, 2022 at 2:21 PM Stephen Hemminger
<[e-mail]> wrote:
>
> On Tue, 24 May 2022 13:55:57 -0700
> Luiz Augusto von Dentz <[e-mail]> 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 May 25, 2022, 8:09 p.m. UTC | #5

Hi Tom,

On Tue, May 24, 2022 at 4:42 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Stephen,
>
> On Tue, May 24, 2022 at 2:21 PM Stephen Hemminger
> <[e-mail]> wrote:
> >
> > On Tue, 24 May 2022 13:55:57 -0700
> > Luiz Augusto von Dentz <[e-mail]> 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/[e-mail]/

Tom Unbehau May 26, 2022, 8:50 p.m. UTC | #6

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

Luiz Augusto von Dentz May 26, 2022, 11:46 p.m. UTC | #7

Hi Tom,

On Thu, May 26, 2022 at 1:50 PM Tom Unbehau <[e-mail]> 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

Tom Unbehau May 27, 2022, 8:43 p.m. UTC | #8

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

Patch

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 */