Re: [PATCH] rtw89: Fix crash by loading compressed firmware file

Takashi Iwai [off-list ref] writes:

On Fri, 05 Nov 2021 09:25:13 +0100,
Kalle Valo wrote:

quoted

Takashi Iwai [off-list ref] writes:

quoted

On Fri, 05 Nov 2021 08:17:25 +0100,
Takashi Iwai wrote:

quoted

When a firmware is loaded in the compressed format or via user-mode
helper, it's mapped in read-only, and the rtw89 driver crashes at
rtw89_fw_download() when it tries to modify some data.

This patch is an attemp to avoid the crash by re-allocating the data
via vmalloc() for the data modification.

Alternatively, we may drop the code that modifies the loaded firmware
data?  At least SET_FW_HDR_PART_SIZE() in rtw89_fw_hdr_parser() looks
writing it, and I have no idea why this overwrite is needed.

Strange, isn't the firmware data marked as const just to avoid this kind
of problem? Does rtw89 have wrong casts somewhere which removes the
const?

Yes.  SET_FW_HDR_PART_SIZE() does the cast, dropping the const.

Oh man, all of GET and SET macros in fw.h have those casts:

#define GET_FW_HDR_MAJOR_VERSION(fwhdr)	\
	le32_get_bits(*((__le32 *)(fwhdr) + 1), GENMASK(7, 0))
#define GET_FW_HDR_MINOR_VERSION(fwhdr)	\
	le32_get_bits(*((__le32 *)(fwhdr) + 1), GENMASK(15, 8))
#define GET_FW_HDR_SUBVERSION(fwhdr)	\
	le32_get_bits(*((__le32 *)(fwhdr) + 1), GENMASK(23, 16))

I don't know how I missed those during my review :( But this is exactly
why I prefer having a proper struct for commands and events, instead of
u8 buf used with these macros.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches