Bluetooth: btusb: fix excessive stack usage

From: Arnd Bergmann <arnd@arndb.de>


Enlarging the size of 'struct btmtk_hci_wmt_cmd' makes it no longer
fit on the kernel stack, as seen from this compiler warning:

drivers/bluetooth/btusb.c:3365:12: error: stack frame size of 1036 bytes in function 'btusb_mtk_hci_wmt_sync' [-Werror,-Wframe-larger-than=]

Change the function to dynamically allocate the buffer instead.
As there are other sleeping functions called from the same location,
using GFP_KERNEL should be fine here, and the runtime overhead should
not matter as this is rarely called.

Unfortunately, I could not figure out why the message size is
increased in the previous patch. Using dynamic allocation means
any size is possible now, but there is still a range check that
limits the total size (including the five-byte header) to 255
bytes, so whatever was intended there is now undone.

Fixes: 48c13301e6ba ("Bluetooth: btusb: Fine-tune mt7663 mechanism.")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 drivers/bluetooth/btusb.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

-- 
2.29.2

Hi Arnd,

> Enlarging the size of 'struct btmtk_hci_wmt_cmd' makes it no longer

> fit on the kernel stack, as seen from this compiler warning:

> 

> drivers/bluetooth/btusb.c:3365:12: error: stack frame size of 1036 bytes in function 'btusb_mtk_hci_wmt_sync' [-Werror,-Wframe-larger-than=]

> 

> Change the function to dynamically allocate the buffer instead.

> As there are other sleeping functions called from the same location,

> using GFP_KERNEL should be fine here, and the runtime overhead should

> not matter as this is rarely called.

> 

> Unfortunately, I could not figure out why the message size is

> increased in the previous patch. Using dynamic allocation means

> any size is possible now, but there is still a range check that

> limits the total size (including the five-byte header) to 255

> bytes, so whatever was intended there is now undone.

> 

> Fixes: 48c13301e6ba ("Bluetooth: btusb: Fine-tune mt7663 mechanism.")

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

> drivers/bluetooth/btusb.c | 24 +++++++++++++++---------

> 1 file changed, 15 insertions(+), 9 deletions(-)


patch has been applied to bluetooth-next tree.

Regards

Marcel

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=427983

---Test result---

##############################
    Test: CheckPatch - FAIL
    Bluetooth: btusb: fix excessive stack usage
WARNING: Unknown commit id '48c13301e6ba', maybe rebased or not pulled?
#22: 
Fixes: 48c13301e6ba ("Bluetooth: btusb: Fine-tune mt7663 mechanism.")

total: 0 errors, 1 warnings, 69 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: btusb: fix excessive stack usage" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


    ##############################
    Test: CheckGitLint - FAIL
    Bluetooth: btusb: fix excessive stack usage
6: B1 Line exceeds max length (140>80): "drivers/bluetooth/btusb.c:3365:12: error: stack frame size of 1036 bytes in function 'btusb_mtk_hci_wmt_sync' [-Werror,-Wframe-larger-than=]"


    ##############################
    Test: CheckBuildK - PASS
    

    ##############################
    Test: CheckTestRunner: Setup - PASS
    

    ##############################
    Test: CheckTestRunner: l2cap-tester - PASS
    Total: 40, Passed: 34 (85.0%), Failed: 0, Not Run: 6

    ##############################
    Test: CheckTestRunner: bnep-tester - PASS
    Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: mgmt-tester - PASS
    Total: 416, Passed: 402 (96.6%), Failed: 0, Not Run: 14

    ##############################
    Test: CheckTestRunner: rfcomm-tester - PASS
    Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: sco-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: smp-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: userchan-tester - PASS
    Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0

    

---
Regards,
Linux Bluetooth

On Thu, Feb 4, 2021 at 7:47 AM Arnd Bergmann <arnd@kernel.org> wrote:
>

> Enlarging the size of 'struct btmtk_hci_wmt_cmd' makes it no longer

>

> Unfortunately, I could not figure out why the message size is

> increased in the previous patch. Using dynamic allocation means


That patch appears to be have been split out of fc342c4dc40
"Bluetooth: btusb: Add protocol support for MediaTek MT7921U USB
devices".  But there is no clear reason why those changes were split
out, which is not helped by vague patch description, and size increase
appears to be a totally random change to unrelated code.  This struct
is used by that latter commit to download firmware with a new format
for mt7921.

But new firmware download function uses code that is just copied from
existing fw download function (should be refactored to share code),
which has a max packet data size of "dlen = min_t(int, 250,
dl_size);", so there was no need to increase size at all.  I'd guess
someone experimented with larger chunks for firmware download, but
then did not use them, but left the larger max size in because it was
a separate commit.

It looks like the new firmware download function will crash if the
firmware file is not consistent:

sectionmap = (struct btmtk_section_map *)(fw_ptr +
MTK_FW_ROM_PATCH_HEADER_SIZE +
              MTK_FW_ROM_PATCH_GD_SIZE + MTK_FW_ROM_PATCH_SEC_MAP_SIZE * i);
section_offset = sectionmap->secoffset;
dl_size = sectionmap->bin_info_spec.dlsize;
...
fw_ptr += section_offset;
/* send fw_ptr[0] to fw_ptr[dl_size] via wmt_cmd(s) */

Both section_offset and dl_size are used unsanitized from the firmware
blob and could point outside the blob.

And the manually calculated struct sizes aren't necessary, if the
structs for the firmware were correct, it could just be:

struct btmtk_firmware {
       struct btmtk_patch_header header;
       struct btmtk_global_desc desc;
       struct btmtk_section_map sections[];
} __packed;

struct btmtk_firmware* fw_ptr = fw->data;

sectionmap = &fw_ptr->sections[i];