Messages in this thread | | | Date | Wed, 13 Apr 2022 23:26:10 +0200 | Subject | Re: staging: r8188eu: struct rt_firmware_header issues | From | Michael Straube <> |
| |
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
| |