Re: staging: r8188eu: struct rt_firmware_header issues
From: Michael Straube
Date: Wed Apr 13 2022 - 17:26:23 EST
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