Re: [PATCH] Bluetooth: btusb: fix excessive stack usage

From: Trent Piepho <tpiepho@gmail.com>
To: Arnd Bergmann <arnd@kernel.org>
Cc: Marcel Holtmann <marcel@holtmann.org>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
	Mark Chen <Mark-YW.Chen@mediatek.com>,
	Arnd Bergmann <arnd@arndb.de>, Kiran K <kiran.k@intel.com>,
	Alain Michaud <alainm@chromium.org>,
	Chethan T N <chethan.tumkur.narayan@intel.com>,
	Abhishek Pandit-Subedi <abhishekpandit@chromium.org>,
	Sathish Narasimman <nsathish41@gmail.com>,
	Rocky Liao <rjliao@codeaurora.org>,
	Ismael Ferreras Morezuelas <swyterzone@gmail.com>,
	Hilda Wu <hildawu@realtek.com>,
	linux-bluetooth <linux-bluetooth@vger.kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Bluetooth: btusb: fix excessive stack usage
Date: Thu, 4 Feb 2021 13:02:39 -0800	[thread overview]
Message-ID: <CA+7tXii4h+GPp-+qG3m+zhDORLtU_ZS=eer_wCkxrWs6sZqT5A@mail.gmail.com> (raw)
In-Reply-To: <20210204154716.1823454-1-arnd@kernel.org>

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];