From: Kalle Valo <hidden> Date: 2021-11-05 09:04:09
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
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
On Fri, 2021-11-05 at 11:03 +0200, Kalle Valo wrote:
quoted
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?
You should put const in the cast in le32_get_bits() invocations, at
least.
For the le32_replace_bits(), ideally it should be rewritten in some
better way the compiler can catch. e.g. use an inline function to
take a void * argument without const,
static inline void RTW89_SET_FWCMD_CXRFK_TYPE(void *cmd, unsigned int val)
{
le32p_replace_bits((__le32 *)((u8 *)(cmd) + 2), val, GENMASK(17, 10));
}
Then the compiler will warn when you pass a const pointer there.
BTW, while reading your reply, I noticed that it's an unaligned access
to a 32bit value, which is another potential breakage on some
architectures. So the whole stuff has to be rewritten in anyway...
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.
-----Original Message-----
From: Takashi Iwai <redacted>
Sent: Friday, November 5, 2021 11:07 PM
To: Pkshih <pkshih@realtek.com>
Cc: kvalo@codeaurora.org; linux-wireless@vger.kernel.org; Larry.Finger@gmail.com;
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rtw89: Fix crash by loading compressed firmware file
You should put const in the cast in le32_get_bits() invocations, at
least.
For the le32_replace_bits(), ideally it should be rewritten in some
better way the compiler can catch. e.g. use an inline function to
take a void * argument without const,
static inline void RTW89_SET_FWCMD_CXRFK_TYPE(void *cmd, unsigned int val)
{
le32p_replace_bits((__le32 *)((u8 *)(cmd) + 2), val, GENMASK(17, 10));
}
Then the compiler will warn when you pass a const pointer there.
I have sent a patchset [1] to do these two things by patch 2 and 3.
BTW, while reading your reply, I noticed that it's an unaligned access
to a 32bit value, which is another potential breakage on some
architectures. So the whole stuff has to be rewritten in anyway...
We use these macros/inline function on skb->data mostly, and I
suppose skb->data is a 32bit aligned address. Since I don't have
this kind of machine on hand, I would like to defer this work until
I have one.
quoted
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.
-----Original Message-----
From: Takashi Iwai <redacted>
Sent: Friday, November 5, 2021 11:07 PM
To: Pkshih <pkshih@realtek.com>
Cc: kvalo@codeaurora.org; linux-wireless@vger.kernel.org; Larry.Finger@gmail.com;
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rtw89: Fix crash by loading compressed firmware file
You should put const in the cast in le32_get_bits() invocations, at
least.
For the le32_replace_bits(), ideally it should be rewritten in some
better way the compiler can catch. e.g. use an inline function to
take a void * argument without const,
static inline void RTW89_SET_FWCMD_CXRFK_TYPE(void *cmd, unsigned int val)
{
le32p_replace_bits((__le32 *)((u8 *)(cmd) + 2), val, GENMASK(17, 10));
}
Then the compiler will warn when you pass a const pointer there.
I have sent a patchset [1] to do these two things by patch 2 and 3.
quoted
BTW, while reading your reply, I noticed that it's an unaligned access
to a 32bit value, which is another potential breakage on some
architectures. So the whole stuff has to be rewritten in anyway...
We use these macros/inline function on skb->data mostly, and I
suppose skb->data is a 32bit aligned address. Since I don't have
this kind of machine on hand, I would like to defer this work until
I have one.
I actually misread the code. The register offset is applied to __le32
pointer, so this should be fine.
quoted
quoted
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.