lkml.org 
[lkml]   [2022]   [Apr]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: staging: r8188eu: struct rt_firmware_header issues
From
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

\
 
 \ /
  Last update: 2022-04-13 23:27    [W:0.079 / U:0.684 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site