staging: r8188eu: struct rt_firmware_header issues

DISCUSSION Michael Straube 4 participants 9 messages Apr 13, 2022 – Apr 14, 2022 Staging

lore

Share & tools

lore

Michael Straube

author

2022-04-13 16:27 UTC #0 lore raw

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

Michael Straube

author

2022-04-13 19:42 UTC #1 lore raw

Quoted 70 lines · Michael Straube

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 ?

Quoted 9 lines

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

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.

regards,
Michael

Larry Finger

2022-04-13 21:12 UTC #2 lore raw

Quoted 76 lines · Michael Straube

Quoted 75 lines · Michael Straube
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. :)

Quoted 7 lines

Quoted 4 lines
regards,
Michael
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 agree. I had prepared an analysis, but did not get it mailed before I got this one.

Larry

David Laight

2022-04-14 08:41 UTC #3 lore raw

In reply to Michael Straube

From: Michael Straube

Quoted 10 lines

Sent: 13 April 2022 20:42
Quoted 7 lines · Michael Straube
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?

Quoted 30 lines

Quoted 30 lines
    /* 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;
};

...

Quoted 3 lines

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.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Michael Straube

author

2022-04-14 10:08 UTC #4 lore raw

Quoted 6 lines · David Laight

From: Michael Straube
Quoted 5 lines
Sent: 13 April 2022 20:42
Quoted 2 lines · Michael Straube
Hi all,

Quoted 7 lines

Quoted 5 lines
Quoted 5 lines
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?

Quoted 30 lines

Quoted 22 lines
Quoted 22 lines
    /* 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;
Quoted 8 lines
Quoted 8 lines
    /* LONG WORD 2 ---- */
    __le32        SvnIdx;    /* The SVN entry index */ u32        Rsvd3;
    /* LONG WORD 3 ---- */
    u32        Rsvd4;
    u32        Rsvd5;
};

Quoted 6 lines

...
Quoted 3 lines
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

David Laight

2022-04-14 10:14 UTC #5 lore raw

From: Michael Straube

Quoted 12 lines

Sent: 14 April 2022 11:08
Quoted 9 lines · David Laight
From: Michael Straube
Quoted 8 lines
Sent: 13 April 2022 20:42
Quoted 5 lines · Michael Straube
Hi all,
I think the rt_firmware_hdr structure in rtw_fw.c has some issues.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
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)

Larry Finger

2022-04-14 15:46 UTC #6 lore raw

Quoted 13 lines · David Laight

From: Michael Straube
Quoted 12 lines
Sent: 14 April 2022 11:08
Quoted 9 lines · David Laight
From: Michael Straube
Quoted 8 lines
Sent: 13 April 2022 20:42
Quoted 5 lines · Michael Straube
Hi all,
I think the rt_firmware_hdr structure in rtw_fw.c has some issues.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
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

Pavel Skripkin

2022-04-13 20:58 UTC #7 lore raw

In reply to Michael Straube

Hi Michael,

Quoted 52 lines · Michael Straube

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

Quoted 3 lines

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.

Quoted 12 lines

		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.

Quoted 6 lines

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

Michael Straube

author

2022-04-13 21:26 UTC #8 lore raw

Quoted 120 lines · Pavel Skripkin

Hi Michael,
Quoted 46 lines · Michael Straube
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>
Quoted 9 lines
    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.
Quoted 12 lines
        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.
Quoted 6 lines
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 ?

Quoted 9 lines

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