Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220524202630.3569412-1-luiz.dentz@gmail.com>
In-Reply-To: <20220524202630.3569412-1-luiz.dentz@gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 24 May 2022 13:55:57 -0700
Message-ID: <CABBYNZKZajJyuenwyv+1T6Gt1SYyLz39yMJF9_LYdTubk+e_rw@mail.gmail.com>
Subject: Re: [PATCH-stable] Bluetooth: eir: Fix using strlen with hdev->{dev_name,short_name}
To:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Cc:     Stephen Hemminger <stephen@networkplumber.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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