Hi all,
I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
struct rt_firmware_hdr {
/* 8-byte alinment required */
/* LONG WORD 0 ---- */
__le16 Signature; /* 92C0: test chip; 92C,
* 88C0: test chip; 88C1: MP A-cut;
* 92C1: MP A-cut */
u8 Category; /* AP/NIC and USB/PCI */
u8 Function; /* Reserved for different FW function
* indcation, for further use when
* driver needs to download different
* FW for different conditions */
__le16 Version; /* FW Version */
u8 Subversion; /* FW Subversion, default 0x00 */
u16 Rsvd1;
/* LONG WORD 1 ---- */
u8 Month; /* Release time Month field */
u8 Date; /* Release time Date field */
u8 Hour; /* Release time Hour field */
u8 Minute; /* Release time Minute field */
__le16 RamCodeSize; /* The size of RAM code */
u8 Foundry;
u8 Rsvd2;
/* LONG WORD 2 ---- */
__le32 SvnIdx; /* The SVN entry index */
u32 Rsvd3;
/* LONG WORD 3 ---- */
u32 Rsvd4;
u32 Rsvd5;
};
Then we have in rtl8188e_firmware_download():
fwhdr = (struct rt_firmware_hdr *)dvobj->firmware.data;
<snip>
if (IS_FW_HEADER_EXIST(fwhdr)) {
/* Shift 32 bytes for FW header */
fw_data = fw_data + 32;
fw_size = fw_size - 32;
}
We add/sub 32 bytes but the size of struct rt_firmware_hdr is actually
33 bytes. I noticed this when I wanted to replace:
fw_data = fw_data + 32;
fw_size = fw_size - 32;
with:
fw_data = fw_data + sizeof(struct rt_firmware_hdr);
fw_size = fw_size - sizeof(struct rt_firmware_hdr);;
To me it looks add/sub 32 is correct here but the struct is
wrong. I don't know if the firmware for this driver is so much different
from firmware for the drivers in drivers/net/wireless/realtek/rtlwifi.
They use a struct of size 32.
Also, souldn't the u16 and u32 variables in the struct be __le16 and
__le32 ?
I wonder if we can just use the rtlwifi_firmware_header structure from
drivers/net/wireless/realtek/rtlwifi/wifi.h ?
Comments from people with better knowledge appreciated. :)
regards,
Michael
On 4/14/22 10:41, David Laight wrote:
> From: Michael Straube
>> Sent: 13 April 2022 20:42
>>
>> On 4/13/22 18:27, Michael Straube wrote:
>>> Hi all,
>>>
>>> I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
>>>
>>>
>>> struct rt_firmware_hdr {
>>> /* 8-byte alinment required */
>
> Probably need an __aligned(8) at the bottom then?
I don't see any reason why this is needed. Do I miss something?
>>> /* LONG WORD 0 ---- */
>>> __le16 Signature; /* 92C0: test chip; 92C,
>>> * 88C0: test chip; 88C1: MP A-cut;
>>> * 92C1: MP A-cut */
>>> u8 Category; /* AP/NIC and USB/PCI */
>>> u8 Function; /* Reserved for different FW function
>>> * indcation, for further use when
>>> * driver needs to download different
>>> * FW for different conditions */
>>> __le16 Version; /* FW Version */
>>> u8 Subversion; /* FW Subversion, default 0x00 */
>>> u16 Rsvd1;
>>>
>>> /* LONG WORD 1 ---- */
>>> u8 Month; /* Release time Month field */
>>> u8 Date; /* Release time Date field */
>>> u8 Hour; /* Release time Hour field */
>>> u8 Minute; /* Release time Minute field */
>>> __le16 RamCodeSize; /* The size of RAM code */
>>> u8 Foundry;
>>> u8 Rsvd2;
>>>
>>> /* LONG WORD 2 ---- */
>>> __le32 SvnIdx; /* The SVN entry index */
>>> u32 Rsvd3;
>>>
>>> /* LONG WORD 3 ---- */
>>> u32 Rsvd4;
>>> u32 Rsvd5;
>>> };
> ...
>>
>> Ok, I figured it out by looking at the hexdumps of firmware files. The
>> field Rsvd1 should be u8 instead of u16. I'll prepare a patch for this.
>
> I'd also add a compile-time assert on the size.
I'll add a patch to the series I recently sent.
thanks,
Michael
From: Michael Straube
> Sent: 14 April 2022 11:08
>
> On 4/14/22 10:41, David Laight wrote:
> > From: Michael Straube
> >> Sent: 13 April 2022 20:42
> >>
> >> On 4/13/22 18:27, Michael Straube wrote:
> >>> Hi all,
> >>>
> >>> I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
> >>>
> >>>
> >>> struct rt_firmware_hdr {
> >>> /* 8-byte alinment required */
> >
> > Probably need an __aligned(8) at the bottom then?
>
> I don't see any reason why this is needed. Do I miss something?
Dunno, the comment might be wrong.
david
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 4/14/22 05:14, David Laight wrote:
> From: Michael Straube
>> Sent: 14 April 2022 11:08
>>
>> On 4/14/22 10:41, David Laight wrote:
>>> From: Michael Straube
>>>> Sent: 13 April 2022 20:42
>>>>
>>>> On 4/13/22 18:27, Michael Straube wrote:
>>>>> Hi all,
>>>>>
>>>>> I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
>>>>>
>>>>>
>>>>> struct rt_firmware_hdr {
>>>>> /* 8-byte alinment required */
>>>
>>> Probably need an __aligned(8) at the bottom then?
>>
>> I don't see any reason why this is needed. Do I miss something?
>
> Dunno, the comment might be wrong.
Actually, the le16_to_cpu() references require alignment 4 (I think I got that
right).
Larry
On 4/13/22 22:58, Pavel Skripkin wrote:
> Hi Michael,
>
> On 4/13/22 19:27, Michael Straube wrote:
>> Hi all,
>>
>> I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
>>
>>
>> struct rt_firmware_hdr {
>> /* 8-byte alinment required */
>> /* LONG WORD 0 ---- */
>> __le16 Signature; /* 92C0: test chip; 92C,
>> * 88C0: test chip; 88C1: MP A-cut;
>> * 92C1: MP A-cut */
>> u8 Category; /* AP/NIC and USB/PCI */
>> u8 Function; /* Reserved for different FW function
>> * indcation, for further use when
>> * driver needs to download different
>> * FW for different conditions */
>> __le16 Version; /* FW Version */
>> u8 Subversion; /* FW Subversion, default 0x00 */
>> u16 Rsvd1;
>>
>> /* LONG WORD 1 ---- */
>> u8 Month; /* Release time Month field */
>> u8 Date; /* Release time Date field */
>> u8 Hour; /* Release time Hour field */
>> u8 Minute; /* Release time Minute field */
>> __le16 RamCodeSize; /* The size of RAM code */
>> u8 Foundry;
>> u8 Rsvd2;
>>
>> /* LONG WORD 2 ---- */
>> __le32 SvnIdx; /* The SVN entry index */
>> u32 Rsvd3;
>>
>> /* LONG WORD 3 ---- */
>> u32 Rsvd4;
>> u32 Rsvd5;
>> };
>>
>>
>> Then we have in rtl8188e_firmware_download():
>>
>>
>> fwhdr = (struct rt_firmware_hdr *)dvobj->firmware.data;
>>
>> <snip>
>>
>> if (IS_FW_HEADER_EXIST(fwhdr)) {
>> /* Shift 32 bytes for FW header */
>> fw_data = fw_data + 32;
>> fw_size = fw_size - 32;
>> }
>>
>> We add/sub 32 bytes but the size of struct rt_firmware_hdr is actually
>> 33 bytes. I noticed this when I wanted to replace:
>>
>
> Looks like size of that structure is 36
>
> └──$ pahole -C rt_firmware_hdr r8188eu.ko
> struct rt_firmware_hdr {
> __le16 Signature; /* 0 2 */
> u8 Category; /* 2 1 */
> u8 Function; /* 3 1 */
> __le16 Version; /* 4 2 */
> u8 Subversion; /* 6 1 */
>
> /* XXX 1 byte hole, try to pack */
>
> u16 Rsvd1; /* 8 2 */
> u8 Month; /* 10 1 */
> u8 Date; /* 11 1 */
> u8 Hour; /* 12 1 */
> u8 Minute; /* 13 1 */
> __le16 RamCodeSize; /* 14 2 */
> u8 Foundry; /* 16 1 */
> u8 Rsvd2; /* 17 1 */
>
> /* XXX 2 bytes hole, try to pack */
>
> __le32 SvnIdx; /* 20 4 */
> u32 Rsvd3; /* 24 4 */
> u32 Rsvd4; /* 28 4 */
> u32 Rsvd5; /* 32 4 */
>
> /* size: 36, cachelines: 1, members: 17 */
> /* sum members: 33, holes: 2, sum holes: 3 */
> /* last cacheline: 36 bytes */
> };
>
>
> And these comments like 'LONG WORD 1' looks misleading, maybe worth
> removing.
>
>
>> fw_data = fw_data + 32;
>> fw_size = fw_size - 32;
>>
>> with:
>> fw_data = fw_data + sizeof(struct rt_firmware_hdr);
>> fw_size = fw_size - sizeof(struct rt_firmware_hdr);;
>>
>> To me it looks add/sub 32 is correct here but the struct is
>> wrong. I don't know if the firmware for this driver is so much different
>> from firmware for the drivers in drivers/net/wireless/realtek/rtlwifi.
>> They use a struct of size 32.
>>
>
> I am not sure about 32... Why non-packed structures are passed via
> wires? Maybe `__packed` is missing? Is realtek fw open-source?
>
> Just thoughts.
>
>> Also, souldn't the u16 and u32 variables in the struct be __le16 and
>> __le32 ?
>>
>> I wonder if we can just use the rtlwifi_firmware_header structure from
>> drivers/net/wireless/realtek/rtlwifi/wifi.h ?
>>
>
> We _should_ use it, since its size is indeed 32 bytes.
>
> └──$ pahole -s vmlinux | rg rtlwifi_firmware_header
> rtlwifi_firmware_header 32 0
>
>
> Looks like you have found a real bug
>
>
Hi Pavel,
I looked at the hexdump of rtl8188eufw.bin and compared it with
rtl8188efw.bin and rtlwifi_firmware_header.
The comparsion showed that Rsvd1 should be u8 instead of u16:
OFFSET rtl8188eufw.bin
-----------------------------------------------------------
0x00000000 E1 88 10 00 0B 00 01 00 01 21 11 27 30
36 00 00
^ ^ ^ ^ ^ ^
Subversion Rsvd1 Month Date Hour Minute
So rt_firmware_hdr is wrong, but it had no effect because 32 is
hardcoded here:
/* Shift 32 bytes for FW header */
fw_data = fw_data + 32;
fw_size = fw_size - 32;
and all fields after Subversion are not used.
regards,
Michael
Hi Michael,
On 4/13/22 19:27, Michael Straube wrote:
> Hi all,
>
> I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
>
>
> struct rt_firmware_hdr {
> /* 8-byte alinment required */
> /* LONG WORD 0 ---- */
> __le16 Signature; /* 92C0: test chip; 92C,
> * 88C0: test chip; 88C1: MP A-cut;
> * 92C1: MP A-cut */
> u8 Category; /* AP/NIC and USB/PCI */
> u8 Function; /* Reserved for different FW function
> * indcation, for further use when
> * driver needs to download different
> * FW for different conditions */
> __le16 Version; /* FW Version */
> u8 Subversion; /* FW Subversion, default 0x00 */
> u16 Rsvd1;
>
> /* LONG WORD 1 ---- */
> u8 Month; /* Release time Month field */
> u8 Date; /* Release time Date field */
> u8 Hour; /* Release time Hour field */
> u8 Minute; /* Release time Minute field */
> __le16 RamCodeSize; /* The size of RAM code */
> u8 Foundry;
> u8 Rsvd2;
>
> /* LONG WORD 2 ---- */
> __le32 SvnIdx; /* The SVN entry index */
> u32 Rsvd3;
>
> /* LONG WORD 3 ---- */
> u32 Rsvd4;
> u32 Rsvd5;
> };
>
>
> Then we have in rtl8188e_firmware_download():
>
>
> fwhdr = (struct rt_firmware_hdr *)dvobj->firmware.data;
>
> <snip>
>
> if (IS_FW_HEADER_EXIST(fwhdr)) {
> /* Shift 32 bytes for FW header */
> fw_data = fw_data + 32;
> fw_size = fw_size - 32;
> }
>
> We add/sub 32 bytes but the size of struct rt_firmware_hdr is actually
> 33 bytes. I noticed this when I wanted to replace:
>
Looks like size of that structure is 36
└──$ pahole -C rt_firmware_hdr r8188eu.ko
struct rt_firmware_hdr {
__le16 Signature; /* 0 2 */
u8 Category; /* 2 1 */
u8 Function; /* 3 1 */
__le16 Version; /* 4 2 */
u8 Subversion; /* 6 1 */
/* XXX 1 byte hole, try to pack */
u16 Rsvd1; /* 8 2 */
u8 Month; /* 10 1 */
u8 Date; /* 11 1 */
u8 Hour; /* 12 1 */
u8 Minute; /* 13 1 */
__le16 RamCodeSize; /* 14 2 */
u8 Foundry; /* 16 1 */
u8 Rsvd2; /* 17 1 */
/* XXX 2 bytes hole, try to pack */
__le32 SvnIdx; /* 20 4 */
u32 Rsvd3; /* 24 4 */
u32 Rsvd4; /* 28 4 */
u32 Rsvd5; /* 32 4 */
/* size: 36, cachelines: 1, members: 17 */
/* sum members: 33, holes: 2, sum holes: 3 */
/* last cacheline: 36 bytes */
};
And these comments like 'LONG WORD 1' looks misleading, maybe worth
removing.
> fw_data = fw_data + 32;
> fw_size = fw_size - 32;
>
> with:
> fw_data = fw_data + sizeof(struct rt_firmware_hdr);
> fw_size = fw_size - sizeof(struct rt_firmware_hdr);;
>
> To me it looks add/sub 32 is correct here but the struct is
> wrong. I don't know if the firmware for this driver is so much different
> from firmware for the drivers in drivers/net/wireless/realtek/rtlwifi.
> They use a struct of size 32.
>
I am not sure about 32... Why non-packed structures are passed via
wires? Maybe `__packed` is missing? Is realtek fw open-source?
Just thoughts.
> Also, souldn't the u16 and u32 variables in the struct be __le16 and
> __le32 ?
>
> I wonder if we can just use the rtlwifi_firmware_header structure from
> drivers/net/wireless/realtek/rtlwifi/wifi.h ?
>
We _should_ use it, since its size is indeed 32 bytes.
└──$ pahole -s vmlinux | rg rtlwifi_firmware_header
rtlwifi_firmware_header 32 0
Looks like you have found a real bug
With regards,
Pavel Skripkin