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
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
> -----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