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

On Fri, 2021-11-05 at 11:03 +0200, Kalle Valo wrote:

Takashi Iwai [off-list ref] writes:

quoted

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.

I can use a struct to access firmware header, becuase their fields
are multiple of 8 bits.

But, the "firmware section header" that is additional header followed
by firmware header, and it contains bit fields, likes:

#define GET_FWSECTION_HDR_SEC_SIZE(fwhdr)	\
	le32_get_bits(*((__le32 *)(fwhdr) + 1), GENMASK(23, 0))
#define GET_FWSECTION_HDR_CHECKSUM(fwhdr)	\
	le32_get_bits(*((__le32 *)(fwhdr) + 1), BIT(28))
#define GET_FWSECTION_HDR_REDL(fwhdr)	\
	le32_get_bits(*((__le32 *)(fwhdr) + 1), BIT(29))
#define GET_FWSECTION_HDR_DL_ADDR(fwhdr)	\
	le32_get_bits(*((__le32 *)(fwhdr)), GENMASK(31, 0))

If we use a struct, it needs big-/little- endians parts.

Then, we will access firmware header with two methods; is
it reasonable?


The macro SET_FW_HDR_PART_SIZE() is used to set the firmware
partition size we are going to download, and it is only used
by rtw89_fw_download_hdr(). So, I will set the partition size
after copying constant firmware header into skb->data.

--
Ping-Ke