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.
Buglink: https://bugzilla.opensuse.org/show_bug.cgi?id=1188303
Signed-off-by: Takashi Iwai <[email protected]>
---
drivers/net/wireless/realtek/rtw89/core.h | 3 ++-
drivers/net/wireless/realtek/rtw89/fw.c | 15 ++++++++++-----
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
index c2885e4dd882..048855e05697 100644
--- a/drivers/net/wireless/realtek/rtw89/core.h
+++ b/drivers/net/wireless/realtek/rtw89/core.h
@@ -2309,7 +2309,8 @@ struct rtw89_fw_suit {
RTW89_FW_VER_CODE((s)->major_ver, (s)->minor_ver, (s)->sub_ver, (s)->sub_idex)
struct rtw89_fw_info {
- const struct firmware *firmware;
+ const void *firmware;
+ size_t firmware_size;
struct rtw89_dev *rtwdev;
struct completion completion;
u8 h2c_seq;
diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c
index 212aaf577d3c..b59fecaeea25 100644
--- a/drivers/net/wireless/realtek/rtw89/fw.c
+++ b/drivers/net/wireless/realtek/rtw89/fw.c
@@ -124,8 +124,8 @@ int rtw89_mfw_recognize(struct rtw89_dev *rtwdev, enum rtw89_fw_type type,
struct rtw89_fw_suit *fw_suit)
{
struct rtw89_fw_info *fw_info = &rtwdev->fw;
- const u8 *mfw = fw_info->firmware->data;
- u32 mfw_len = fw_info->firmware->size;
+ const u8 *mfw = fw_info->firmware;
+ u32 mfw_len = fw_info->firmware_size;
const struct rtw89_mfw_hdr *mfw_hdr = (const struct rtw89_mfw_hdr *)mfw;
const struct rtw89_mfw_info *mfw_info;
int i;
@@ -489,7 +489,10 @@ static void rtw89_load_firmware_cb(const struct firmware *firmware, void *contex
return;
}
- fw->firmware = firmware;
+ fw->firmware = vmalloc(firmware->size);
+ if (fw->firmware)
+ memcpy((void *)fw->firmware, firmware->data, firmware->size);
+ release_firmware(firmware);
complete_all(&fw->completion);
}
@@ -518,8 +521,10 @@ void rtw89_unload_firmware(struct rtw89_dev *rtwdev)
rtw89_wait_firmware_completion(rtwdev);
- if (fw->firmware)
- release_firmware(fw->firmware);
+ if (fw->firmware) {
+ vfree(fw->firmware);
+ fw->firmware = NULL;
+ }
}
#define H2C_CAM_LEN 60
--
2.26.2
On Fri, 05 Nov 2021 08:17:25 +0100,
Takashi Iwai wrote:
>
> 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.
thanks,
Takashi
>
> Buglink: https://bugzilla.opensuse.org/show_bug.cgi?id=1188303
> Signed-off-by: Takashi Iwai <[email protected]>
>
> ---
> drivers/net/wireless/realtek/rtw89/core.h | 3 ++-
> drivers/net/wireless/realtek/rtw89/fw.c | 15 ++++++++++-----
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw89/core.h b/drivers/net/wireless/realtek/rtw89/core.h
> index c2885e4dd882..048855e05697 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ -2309,7 +2309,8 @@ struct rtw89_fw_suit {
> RTW89_FW_VER_CODE((s)->major_ver, (s)->minor_ver, (s)->sub_ver, (s)->sub_idex)
>
> struct rtw89_fw_info {
> - const struct firmware *firmware;
> + const void *firmware;
> + size_t firmware_size;
> struct rtw89_dev *rtwdev;
> struct completion completion;
> u8 h2c_seq;
> diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c
> index 212aaf577d3c..b59fecaeea25 100644
> --- a/drivers/net/wireless/realtek/rtw89/fw.c
> +++ b/drivers/net/wireless/realtek/rtw89/fw.c
> @@ -124,8 +124,8 @@ int rtw89_mfw_recognize(struct rtw89_dev *rtwdev, enum rtw89_fw_type type,
> struct rtw89_fw_suit *fw_suit)
> {
> struct rtw89_fw_info *fw_info = &rtwdev->fw;
> - const u8 *mfw = fw_info->firmware->data;
> - u32 mfw_len = fw_info->firmware->size;
> + const u8 *mfw = fw_info->firmware;
> + u32 mfw_len = fw_info->firmware_size;
> const struct rtw89_mfw_hdr *mfw_hdr = (const struct rtw89_mfw_hdr *)mfw;
> const struct rtw89_mfw_info *mfw_info;
> int i;
> @@ -489,7 +489,10 @@ static void rtw89_load_firmware_cb(const struct firmware *firmware, void *contex
> return;
> }
>
> - fw->firmware = firmware;
> + fw->firmware = vmalloc(firmware->size);
> + if (fw->firmware)
> + memcpy((void *)fw->firmware, firmware->data, firmware->size);
> + release_firmware(firmware);
> complete_all(&fw->completion);
> }
>
> @@ -518,8 +521,10 @@ void rtw89_unload_firmware(struct rtw89_dev *rtwdev)
>
> rtw89_wait_firmware_completion(rtwdev);
>
> - if (fw->firmware)
> - release_firmware(fw->firmware);
> + if (fw->firmware) {
> + vfree(fw->firmware);
> + fw->firmware = NULL;
> + }
> }
>
> #define H2C_CAM_LEN 60
> --
> 2.26.2
>
Takashi Iwai <[email protected]> writes:
> On Fri, 05 Nov 2021 08:17:25 +0100,
> Takashi Iwai wrote:
>>
>> 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?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Takashi Iwai <[email protected]> writes:
> On Fri, 05 Nov 2021 09:25:13 +0100,
> Kalle Valo wrote:
>>
>> Takashi Iwai <[email protected]> writes:
>>
>> > On Fri, 05 Nov 2021 08:17:25 +0100,
>> > Takashi Iwai wrote:
>> >>
>> >> 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 <[email protected]> writes:
>
> > On Fri, 05 Nov 2021 09:25:13 +0100,
> > Kalle Valo wrote:
> > > Takashi Iwai <[email protected]> writes:
> > >
> > > > On Fri, 05 Nov 2021 08:17:25 +0100,
> > > > Takashi Iwai wrote:
> > > > > 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, 05 Nov 2021 15:28:39 +0100,
Pkshih wrote:
>
> On Fri, 2021-11-05 at 11:03 +0200, Kalle Valo wrote:
> > Takashi Iwai <[email protected]> writes:
> >
> > > On Fri, 05 Nov 2021 09:25:13 +0100,
> > > Kalle Valo wrote:
> > > > Takashi Iwai <[email protected]> writes:
> > > >
> > > > > On Fri, 05 Nov 2021 08:17:25 +0100,
> > > > > Takashi Iwai wrote:
> > > > > > 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.
Sounds good.
thanks,
Takashi
On Fri, 05 Nov 2021 09:25:13 +0100,
Kalle Valo wrote:
>
> Takashi Iwai <[email protected]> writes:
>
> > On Fri, 05 Nov 2021 08:17:25 +0100,
> > Takashi Iwai wrote:
> >>
> >> 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.
thanks,
Takashi
> -----Original Message-----
> From: Takashi Iwai <[email protected]>
> Sent: Friday, November 5, 2021 11:07 PM
> To: Pkshih <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> 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.
>
> > 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.
>
> Sounds good.
>
Please check if my patch works on your platform.
Thanks you.
[1] https://lore.kernel.org/linux-wireless/[email protected]/T/#t
--
Ping-Ke
On Thu, 11 Nov 2021 03:28:09 +0100,
Pkshih wrote:
>
>
> > -----Original Message-----
> > From: Takashi Iwai <[email protected]>
> > Sent: Friday, November 5, 2021 11:07 PM
> > To: Pkshih <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]
> > 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.
I actually misread the code. The register offset is applied to __le32
pointer, so this should be fine.
> > > 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.
> >
> > Sounds good.
> >
>
> Please check if my patch works on your platform.
> Thanks you.
>
> [1] https://lore.kernel.org/linux-wireless/[email protected]/T/#t
Thanks. I'll ask people testing those patches.
Takashi
On Thu, 11 Nov 2021 07:31:06 +0100,
Takashi Iwai wrote:
>
> On Thu, 11 Nov 2021 03:28:09 +0100,
> Pkshih wrote:
> > Please check if my patch works on your platform.
> > Thanks you.
> >
> > [1] https://lore.kernel.org/linux-wireless/[email protected]/T/#t
>
> Thanks. I'll ask people testing those patches.
The patches have been confirmed to work.
Feel free to put the tag
BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1188303
thanks,
Takashi
> -----Original Message-----
> From: Takashi Iwai <[email protected]>
> Sent: Thursday, November 11, 2021 9:34 PM
> To: Pkshih <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH] rtw89: Fix crash by loading compressed firmware file
>
> On Thu, 11 Nov 2021 07:31:06 +0100,
> Takashi Iwai wrote:
> >
> > On Thu, 11 Nov 2021 03:28:09 +0100,
> > Pkshih wrote:
> > > Please check if my patch works on your platform.
> > > Thanks you.
> > >
> > > [1] https://lore.kernel.org/linux-wireless/[email protected]/T/#t
> >
> > Thanks. I'll ask people testing those patches.
>
> The patches have been confirmed to work.
> Feel free to put the tag
>
> BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1188303
>
I have sent v2 with BugLink and Tested-by tags.
If anything is improper, please let me know.
--
Ping-Ke