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;
Date:   Mon, 23 May 2022 15:28:32 -0700
From:   Stephen Hemminger <stephen@networkplumber.org>
To:     Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     linux-bluetooth@vger.kernel.org
Subject: Fw: [Bug 216018] New: Bluetooth: strlen in append_eir_data_to_buf
 causes calling process to freeze when short/longname does not have double
 NUL termination.
Message-ID: <20220523152832.199b93bf@hermes.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk


Begin forwarded message:

Date: Mon, 23 May 2022 15:32:38 +0000
From: bugzilla-daemon@kernel.org
To: stephen@networkplumber.org
Subject: [Bug 216018] New: Bluetooth: strlen in append_eir_data_to_buf causes calling process to freeze when short/longname does not have double NUL termination.


https://bugzilla.kernel.org/show_bug.cgi?id=216018

            Bug ID: 216018
           Summary: Bluetooth: strlen in append_eir_data_to_buf causes
                    calling process to freeze when short/longname does not
                    have double NUL termination.
           Product: Networking
           Version: 2.5
    Kernel Version: 5.17.6-1-MANJARO #1
          Hardware: All
                OS: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: Other
          Assignee: stephen@networkplumber.org
          Reporter: tomu1@verifone.com
        Regression: No

Created attachment 301019
  --> https://bugzilla.kernel.org/attachment.cgi?id=301019&action=edit  
Program to cause strlen Kernel Bug

The process I use to set the bluetooth short/longname via HCI freezes with a
kernel bug.
I have investigated this problem and have possibly found the issue.

When writing the bluetooth localname to the kernel via HCI, the userspace is
required to
write the command to the HCI socket in the following way
(https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/mgmt-api.txt):


"
Command Code:           0x000F
        Controller Index:       <controller id>
        Command Parameters:     Name (249 Octets)
                                Short_Name (11 Octets)
        Return Parameters:      Name (249 Octets)
                                Short_Name (11 Octets))

[...]

The values of name and short name will be remembered when
        switching the controller off and back on again. So the name
        and short name only have to be set once when a new controller
        is found and will stay until removed.                           
"

It is requested that both Name and Short_Name are zero terminated.

When this command is sent via the HCI socket eventually set_local_name is
called in the kernel:

static int set_local_name(struct sock *sk, struct hci_dev *hdev, void *data,
                          u16 len)
{
        struct mgmt_cp_set_local_name *cp = data;

The structures which are important here are hci_dev and mgmt_cp_set_local_name:

struct hci_dev (excerpt):
The relevant struct is defined as such:
        __u8            dev_name[HCI_MAX_NAME_LENGTH];
        __u8            short_name[HCI_MAX_SHORT_NAME_LENGTH];

The length macros are defined as such:
#define HCI_MAX_NAME_LENGTH             248
#define HCI_MAX_SHORT_NAME_LENGTH       10

struct mgmt_cp_set_local_name {
        __u8    name[MGMT_MAX_NAME_LENGTH];
        __u8    short_name[MGMT_MAX_SHORT_NAME_LENGTH];
} __packed;

Its length macros are the following:

/* Reserve one extra byte for names in management messages so that they
 * are always guaranteed to be nul-terminated */
#define MGMT_MAX_NAME_LENGTH            (HCI_MAX_NAME_LENGTH + 1)
#define MGMT_MAX_SHORT_NAME_LENGTH      (HCI_MAX_SHORT_NAME_LENGTH + 1)

When set_local_name (net/bluetooth/mgmt.c) is called an memcpy is used to
transfer
the names:

memcpy(hdev->short_name, cp->short_name, sizeof(hdev->short_name));
if (!hdev_is_powered(hdev)) {
                memcpy(hdev->dev_name, cp->name, sizeof(hdev->dev_name));

This would not be a problem in of itself,
however when ext_info_changed is called in the same function (and possibly
elsewhere), it internally uses append_eir_data_to_buf, which uses 
strlen to determine the length of the device/shortname:

static u16 append_eir_data_to_buf(struct hci_dev *hdev, u8 *eir)
...
name_len = strlen(hdev->dev_name);
....
name_len = strlen(hdev->short_name);

As such, when the userspace gives a name which does not have 2 NUL bytes
at the end of long(dev)name and shortname, then the strlen will search
over the size of the buffer, as internal structure does not have a NUL
byte, which, sadly, causes my started hci process
to completely freeze (unrecoverable) due to kernel hardening
kicking in.

An easy wasy to reproduce this issue is by executing the attached program.
Please note blueZ and all of these Bluetooth grabbing services need to be
disabled and the controller itself must be powered off.

A temporary workaround is to terminate the sent names with two NUL
bytes.

It would be very kind of you to take a look at this issue.

Thank you for your time,
Tom Unbehau

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are the assignee for the bug.