| Message ID | 20210204154716.1823454-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: btusb: fix excessive stack usage | expand |
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];
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index eeafb8432c0f..838e6682301e 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -3161,7 +3161,7 @@ struct btmtk_wmt_hdr { struct btmtk_hci_wmt_cmd { struct btmtk_wmt_hdr hdr; - u8 data[1000]; + u8 data[]; } __packed; struct btmtk_hci_wmt_evt { @@ -3369,7 +3369,7 @@ static int btusb_mtk_hci_wmt_sync(struct hci_dev *hdev, struct btmtk_hci_wmt_evt_funcc *wmt_evt_funcc; u32 hlen, status = BTMTK_WMT_INVALID; struct btmtk_hci_wmt_evt *wmt_evt; - struct btmtk_hci_wmt_cmd wc; + struct btmtk_hci_wmt_cmd *wc; struct btmtk_wmt_hdr *hdr; int err; @@ -3383,20 +3383,24 @@ static int btusb_mtk_hci_wmt_sync(struct hci_dev *hdev, if (hlen > 255) return -EINVAL; - hdr = (struct btmtk_wmt_hdr *)&wc; + wc = kzalloc(hlen, GFP_KERNEL); + if (!wc) + return -ENOMEM; + + hdr = &wc->hdr; hdr->dir = 1; hdr->op = wmt_params->op; hdr->dlen = cpu_to_le16(wmt_params->dlen + 1); hdr->flag = wmt_params->flag; - memcpy(wc.data, wmt_params->data, wmt_params->dlen); + memcpy(wc->data, wmt_params->data, wmt_params->dlen); set_bit(BTUSB_TX_WAIT_VND_EVT, &data->flags); - err = __hci_cmd_send(hdev, 0xfc6f, hlen, &wc); + err = __hci_cmd_send(hdev, 0xfc6f, hlen, wc); if (err < 0) { clear_bit(BTUSB_TX_WAIT_VND_EVT, &data->flags); - return err; + goto err_free_wc; } /* The vendor specific WMT commands are all answered by a vendor @@ -3413,13 +3417,14 @@ static int btusb_mtk_hci_wmt_sync(struct hci_dev *hdev, if (err == -EINTR) { bt_dev_err(hdev, "Execution of wmt command interrupted"); clear_bit(BTUSB_TX_WAIT_VND_EVT, &data->flags); - return err; + goto err_free_wc; } if (err) { bt_dev_err(hdev, "Execution of wmt command timed out"); clear_bit(BTUSB_TX_WAIT_VND_EVT, &data->flags); - return -ETIMEDOUT; + err = -ETIMEDOUT; + goto err_free_wc; } /* Parse and handle the return WMT event */ @@ -3463,7 +3468,8 @@ static int btusb_mtk_hci_wmt_sync(struct hci_dev *hdev, err_free_skb: kfree_skb(data->evt_skb); data->evt_skb = NULL; - +err_free_wc: + kfree(wc); return err; }