| Message ID | 20211105071725.31539-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtw89: Fix crash by loading compressed firmware file | expand |
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 <tiwai@suse.de> > > --- > 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 <tiwai@suse.de> 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?
On Fri, 05 Nov 2021 09:25:13 +0100, Kalle Valo wrote: > > Takashi Iwai <tiwai@suse.de> 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
Takashi Iwai <tiwai@suse.de> writes: > On Fri, 05 Nov 2021 09:25:13 +0100, > Kalle Valo wrote: >> >> Takashi Iwai <tiwai@suse.de> 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.
On Fri, 2021-11-05 at 11:03 +0200, Kalle Valo wrote: > Takashi Iwai <tiwai@suse.de> writes: > > > On Fri, 05 Nov 2021 09:25:13 +0100, > > Kalle Valo wrote: > > > Takashi Iwai <tiwai@suse.de> 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 <tiwai@suse.de> writes: > > > > > On Fri, 05 Nov 2021 09:25:13 +0100, > > > Kalle Valo wrote: > > > > Takashi Iwai <tiwai@suse.de> 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
> -----Original Message----- > From: Takashi Iwai <tiwai@suse.de> > 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. > > > 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/20211111021457.13776-1-pkshih@realtek.com/T/#t -- Ping-Ke
On Thu, 11 Nov 2021 03:28:09 +0100, Pkshih wrote: > > > > -----Original Message----- > > From: Takashi Iwai <tiwai@suse.de> > > 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. 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/20211111021457.13776-1-pkshih@realtek.com/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/20211111021457.13776-1-pkshih@realtek.com/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 <tiwai@suse.de> > Sent: Thursday, November 11, 2021 9:34 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 > > 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/20211111021457.13776-1-pkshih@realtek.com/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
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
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 <tiwai@suse.de> --- drivers/net/wireless/realtek/rtw89/core.h | 3 ++- drivers/net/wireless/realtek/rtw89/fw.c | 15 ++++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-)