diff mbox

[for,stable/back-port] staging: vt6656: fix headers - key.c only

Message ID 1358090782.4514.92.camel@deadeye.wl.decadent.org.uk
State New
Headers show

Commit Message

Ben Hutchings Jan. 13, 2013, 3:26 p.m. UTC 
On Sun, 2012-12-30 at 15:01 +0000, Malcolm Priestley wrote:
> On Sun, 2012-12-30 at 00:50 +0100, Ben Hutchings wrote:
> > On Sat, 2012-12-29 at 13:10 +0000, Malcolm Priestley wrote:
> > > shorted back-ported version of upstream commit
> > > 11d404cb56ecd53bb23499897fbe7be1a9ac4827
> > > staging: vt6656: fix headers and add cfg80211.
> > > key.c only
> > > 
> > > This patch fixes the deadlock of 64 bit systems
> > > on successful association.
> > > 
> > > In key.h void pointer pvKeyTable in SKeyItem is out of alignment
> > > on 64 bit kernel.
> > > 
> > > The upstream arrangement of headers fixes this.
> > [...]
> > 
> > Please explain how.  I don't see anything weird about key.h and mac.h
> > that would cause structure definitions to be interpreted differently
> > depending on inclusion order.
> 
> 
> The difference is that key.h is no longer defined in key.c.
>
> It is declared in path;
> mac.h
> ----->device.h
> -------------->key.h
> 
> The structure is now as seen by rxtx.c and dpc.c.
> 
> Before the patch, key.c has it's own localised version
> declared in key.h and for some reason(?) is not packed the same.
[...]

Here are the key structure definitions and the layout they should have
depending on word size and native/packed alignment:

                                        32n 32p 64n 64p
typedef struct tagSKeyItem
{
    BOOL        bKeyValid;		  0   0   0   0
	u32 uKeyLength;			  4   4   4   4
    BYTE        abyKey[MAX_KEY_LEN];	  8   8   8   8
    QWORD       KeyRSC;			 40  40  40  40
    DWORD       dwTSC47_16;		 48  48  48  48
    WORD        wTSC15_0;		 52  52  52  52
    BYTE        byCipherSuite;		 54  54  54  54
    BYTE        byReserved0;		 55  55  55  55
    DWORD       dwKeyIndex;		 56  56  56  56
    void *pvKeyTable;			 60  60  64  60
} SKeyItem, *PSKeyItem; //64		 64  64  72  68

typedef struct tagSKeyTable
{
    BYTE        abyBSSID[ETH_ALEN];	  0   0   0   0
    BYTE        byReserved0[2];           6   6   6   6
    SKeyItem    PairwiseKey;		  8   8   8   8
    SKeyItem    GroupKey[MAX_GROUP_KEY]; 72  72  80  76
    DWORD       dwGTKeyIndex;           328 328 368 348
    BOOL        bInUse;			332 332 372 352
    WORD        wKeyCtl;		336 336 376 356
    BOOL        bSoftWEP;		340 338 380 358
    BYTE        byReserved1[6];		344 342 384 362
} SKeyTable, *PSKeyTable; //352		352 348 392 368

This agrees with your observed offsets of bSoftWEP, if SKeyItem and
SKeyTable are defined packed or not depending on the order of inclusion.

The only thing that should cause alignment to change in this way is the
abominable '#pragma pack', and sure enough several of the headers have
'#pragma pack(1)' but no '#pragma pack()' to reset this.

Does the following work for you?  (It's based on 3.7; the context is
slightly different for earlier versions.)

---
Subject: vt6656: Fix inconsistent structure packing

vt6656 has several headers that use the #pragma pack(1) directive to
enable structure packing, but never disable it.  The layout of
structures defined in other headers can then depend on which order the
various headers are included in, breaking the One Definition Rule.

This removes those directives and adds __packed to structure
definitions where packing appears to have been intended.

Reported-by: Malcolm Priestley <tvboxspy@gmail.com>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---

Comments

Malcolm Priestley Jan. 13, 2013, 8:50 p.m. UTC | #1 
On Sun, 2013-01-13 at 15:26 +0000, Ben Hutchings wrote:
> On Sun, 2012-12-30 at 15:01 +0000, Malcolm Priestley wrote:
> > On Sun, 2012-12-30 at 00:50 +0100, Ben Hutchings wrote:
> > > On Sat, 2012-12-29 at 13:10 +0000, Malcolm Priestley wrote:
> > > > shorted back-ported version of upstream commit
> > > > 11d404cb56ecd53bb23499897fbe7be1a9ac4827
> > > > staging: vt6656: fix headers and add cfg80211.
> > > > key.c only
> > > > 
> > > > This patch fixes the deadlock of 64 bit systems
> > > > on successful association.
> > > > 
> > > > In key.h void pointer pvKeyTable in SKeyItem is out of alignment
> > > > on 64 bit kernel.
> > > > 
> > > > The upstream arrangement of headers fixes this.
> > > [...]
> > > 
> > > Please explain how.  I don't see anything weird about key.h and mac.h
> > > that would cause structure definitions to be interpreted differently
> > > depending on inclusion order.
> > 
> > 
> > The difference is that key.h is no longer defined in key.c.
> >
> > It is declared in path;
> > mac.h
> > ----->device.h
> > -------------->key.h
> > 
> > The structure is now as seen by rxtx.c and dpc.c.
> > 
> > Before the patch, key.c has it's own localised version
> > declared in key.h and for some reason(?) is not packed the same.
> [...]
> 
> Here are the key structure definitions and the layout they should have
> depending on word size and native/packed alignment:
> 
>                                         32n 32p 64n 64p
> typedef struct tagSKeyItem
> {
>     BOOL        bKeyValid;		  0   0   0   0
> 	u32 uKeyLength;			  4   4   4   4
>     BYTE        abyKey[MAX_KEY_LEN];	  8   8   8   8
>     QWORD       KeyRSC;			 40  40  40  40
>     DWORD       dwTSC47_16;		 48  48  48  48
>     WORD        wTSC15_0;		 52  52  52  52
>     BYTE        byCipherSuite;		 54  54  54  54
>     BYTE        byReserved0;		 55  55  55  55
>     DWORD       dwKeyIndex;		 56  56  56  56
>     void *pvKeyTable;			 60  60  64  60
> } SKeyItem, *PSKeyItem; //64		 64  64  72  68
> 
> typedef struct tagSKeyTable
> {
>     BYTE        abyBSSID[ETH_ALEN];	  0   0   0   0
>     BYTE        byReserved0[2];           6   6   6   6
>     SKeyItem    PairwiseKey;		  8   8   8   8
>     SKeyItem    GroupKey[MAX_GROUP_KEY]; 72  72  80  76
>     DWORD       dwGTKeyIndex;           328 328 368 348
>     BOOL        bInUse;			332 332 372 352
>     WORD        wKeyCtl;		336 336 376 356
>     BOOL        bSoftWEP;		340 338 380 358
>     BYTE        byReserved1[6];		344 342 384 362
> } SKeyTable, *PSKeyTable; //352		352 348 392 368
> 
> This agrees with your observed offsets of bSoftWEP, if SKeyItem and
> SKeyTable are defined packed or not depending on the order of inclusion.
> 
> The only thing that should cause alignment to change in this way is the
> abominable '#pragma pack', and sure enough several of the headers have
> '#pragma pack(1)' but no '#pragma pack()' to reset this.
> 
> Does the following work for you?  (It's based on 3.7; the context is
> slightly different for earlier versions.)
> 
> ---
> Subject: vt6656: Fix inconsistent structure packing
> 
> vt6656 has several headers that use the #pragma pack(1) directive to
> enable structure packing, but never disable it.  The layout of
> structures defined in other headers can then depend on which order the
> various headers are included in, breaking the One Definition Rule.
> 
> This removes those directives and adds __packed to structure
> definitions where packing appears to have been intended.
> 
> Reported-by: Malcolm Priestley <tvboxspy@gmail.com>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Thanks Ben

I have tested the patch on 3.7 and 3.8-rc2 and it has resolved the
issue.

Regards


Malcolm
gregkh@linuxfoundation.org Jan. 17, 2013, 9:06 p.m. UTC | #2 
On Sun, Jan 13, 2013 at 08:50:43PM +0000, Malcolm Priestley wrote:
> On Sun, 2013-01-13 at 15:26 +0000, Ben Hutchings wrote:
> > On Sun, 2012-12-30 at 15:01 +0000, Malcolm Priestley wrote:
> > > On Sun, 2012-12-30 at 00:50 +0100, Ben Hutchings wrote:
> > > > On Sat, 2012-12-29 at 13:10 +0000, Malcolm Priestley wrote:
> > > > > shorted back-ported version of upstream commit
> > > > > 11d404cb56ecd53bb23499897fbe7be1a9ac4827
> > > > > staging: vt6656: fix headers and add cfg80211.
> > > > > key.c only
> > > > > 
> > > > > This patch fixes the deadlock of 64 bit systems
> > > > > on successful association.
> > > > > 
> > > > > In key.h void pointer pvKeyTable in SKeyItem is out of alignment
> > > > > on 64 bit kernel.
> > > > > 
> > > > > The upstream arrangement of headers fixes this.
> > > > [...]
> > > > 
> > > > Please explain how.  I don't see anything weird about key.h and mac.h
> > > > that would cause structure definitions to be interpreted differently
> > > > depending on inclusion order.
> > > 
> > > 
> > > The difference is that key.h is no longer defined in key.c.
> > >
> > > It is declared in path;
> > > mac.h
> > > ----->device.h
> > > -------------->key.h
> > > 
> > > The structure is now as seen by rxtx.c and dpc.c.
> > > 
> > > Before the patch, key.c has it's own localised version
> > > declared in key.h and for some reason(?) is not packed the same.
> > [...]
> > 
> > Here are the key structure definitions and the layout they should have
> > depending on word size and native/packed alignment:
> > 
> >                                         32n 32p 64n 64p
> > typedef struct tagSKeyItem
> > {
> >     BOOL        bKeyValid;		  0   0   0   0
> > 	u32 uKeyLength;			  4   4   4   4
> >     BYTE        abyKey[MAX_KEY_LEN];	  8   8   8   8
> >     QWORD       KeyRSC;			 40  40  40  40
> >     DWORD       dwTSC47_16;		 48  48  48  48
> >     WORD        wTSC15_0;		 52  52  52  52
> >     BYTE        byCipherSuite;		 54  54  54  54
> >     BYTE        byReserved0;		 55  55  55  55
> >     DWORD       dwKeyIndex;		 56  56  56  56
> >     void *pvKeyTable;			 60  60  64  60
> > } SKeyItem, *PSKeyItem; //64		 64  64  72  68
> > 
> > typedef struct tagSKeyTable
> > {
> >     BYTE        abyBSSID[ETH_ALEN];	  0   0   0   0
> >     BYTE        byReserved0[2];           6   6   6   6
> >     SKeyItem    PairwiseKey;		  8   8   8   8
> >     SKeyItem    GroupKey[MAX_GROUP_KEY]; 72  72  80  76
> >     DWORD       dwGTKeyIndex;           328 328 368 348
> >     BOOL        bInUse;			332 332 372 352
> >     WORD        wKeyCtl;		336 336 376 356
> >     BOOL        bSoftWEP;		340 338 380 358
> >     BYTE        byReserved1[6];		344 342 384 362
> > } SKeyTable, *PSKeyTable; //352		352 348 392 368
> > 
> > This agrees with your observed offsets of bSoftWEP, if SKeyItem and
> > SKeyTable are defined packed or not depending on the order of inclusion.
> > 
> > The only thing that should cause alignment to change in this way is the
> > abominable '#pragma pack', and sure enough several of the headers have
> > '#pragma pack(1)' but no '#pragma pack()' to reset this.
> > 
> > Does the following work for you?  (It's based on 3.7; the context is
> > slightly different for earlier versions.)
> > 
> > ---
> > Subject: vt6656: Fix inconsistent structure packing
> > 
> > vt6656 has several headers that use the #pragma pack(1) directive to
> > enable structure packing, but never disable it.  The layout of
> > structures defined in other headers can then depend on which order the
> > various headers are included in, breaking the One Definition Rule.
> > 
> > This removes those directives and adds __packed to structure
> > definitions where packing appears to have been intended.
> > 
> > Reported-by: Malcolm Priestley <tvboxspy@gmail.com>
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> 
> Thanks Ben
> 
> I have tested the patch on 3.7 and 3.8-rc2 and it has resolved the
> issue.

Great, can someone send me the patch so that I can apply it upstream?

thanks,

greg k-h
gregkh@linuxfoundation.org Jan. 17, 2013, 9:07 p.m. UTC | #3 
On Thu, Jan 17, 2013 at 01:06:52PM -0800, Greg Kroah-Hartman wrote:
> On Sun, Jan 13, 2013 at 08:50:43PM +0000, Malcolm Priestley wrote:
> > On Sun, 2013-01-13 at 15:26 +0000, Ben Hutchings wrote:
> > > On Sun, 2012-12-30 at 15:01 +0000, Malcolm Priestley wrote:
> > > > On Sun, 2012-12-30 at 00:50 +0100, Ben Hutchings wrote:
> > > > > On Sat, 2012-12-29 at 13:10 +0000, Malcolm Priestley wrote:
> > > > > > shorted back-ported version of upstream commit
> > > > > > 11d404cb56ecd53bb23499897fbe7be1a9ac4827
> > > > > > staging: vt6656: fix headers and add cfg80211.
> > > > > > key.c only
> > > > > > 
> > > > > > This patch fixes the deadlock of 64 bit systems
> > > > > > on successful association.
> > > > > > 
> > > > > > In key.h void pointer pvKeyTable in SKeyItem is out of alignment
> > > > > > on 64 bit kernel.
> > > > > > 
> > > > > > The upstream arrangement of headers fixes this.
> > > > > [...]
> > > > > 
> > > > > Please explain how.  I don't see anything weird about key.h and mac.h
> > > > > that would cause structure definitions to be interpreted differently
> > > > > depending on inclusion order.
> > > > 
> > > > 
> > > > The difference is that key.h is no longer defined in key.c.
> > > >
> > > > It is declared in path;
> > > > mac.h
> > > > ----->device.h
> > > > -------------->key.h
> > > > 
> > > > The structure is now as seen by rxtx.c and dpc.c.
> > > > 
> > > > Before the patch, key.c has it's own localised version
> > > > declared in key.h and for some reason(?) is not packed the same.
> > > [...]
> > > 
> > > Here are the key structure definitions and the layout they should have
> > > depending on word size and native/packed alignment:
> > > 
> > >                                         32n 32p 64n 64p
> > > typedef struct tagSKeyItem
> > > {
> > >     BOOL        bKeyValid;		  0   0   0   0
> > > 	u32 uKeyLength;			  4   4   4   4
> > >     BYTE        abyKey[MAX_KEY_LEN];	  8   8   8   8
> > >     QWORD       KeyRSC;			 40  40  40  40
> > >     DWORD       dwTSC47_16;		 48  48  48  48
> > >     WORD        wTSC15_0;		 52  52  52  52
> > >     BYTE        byCipherSuite;		 54  54  54  54
> > >     BYTE        byReserved0;		 55  55  55  55
> > >     DWORD       dwKeyIndex;		 56  56  56  56
> > >     void *pvKeyTable;			 60  60  64  60
> > > } SKeyItem, *PSKeyItem; //64		 64  64  72  68
> > > 
> > > typedef struct tagSKeyTable
> > > {
> > >     BYTE        abyBSSID[ETH_ALEN];	  0   0   0   0
> > >     BYTE        byReserved0[2];           6   6   6   6
> > >     SKeyItem    PairwiseKey;		  8   8   8   8
> > >     SKeyItem    GroupKey[MAX_GROUP_KEY]; 72  72  80  76
> > >     DWORD       dwGTKeyIndex;           328 328 368 348
> > >     BOOL        bInUse;			332 332 372 352
> > >     WORD        wKeyCtl;		336 336 376 356
> > >     BOOL        bSoftWEP;		340 338 380 358
> > >     BYTE        byReserved1[6];		344 342 384 362
> > > } SKeyTable, *PSKeyTable; //352		352 348 392 368
> > > 
> > > This agrees with your observed offsets of bSoftWEP, if SKeyItem and
> > > SKeyTable are defined packed or not depending on the order of inclusion.
> > > 
> > > The only thing that should cause alignment to change in this way is the
> > > abominable '#pragma pack', and sure enough several of the headers have
> > > '#pragma pack(1)' but no '#pragma pack()' to reset this.
> > > 
> > > Does the following work for you?  (It's based on 3.7; the context is
> > > slightly different for earlier versions.)
> > > 
> > > ---
> > > Subject: vt6656: Fix inconsistent structure packing
> > > 
> > > vt6656 has several headers that use the #pragma pack(1) directive to
> > > enable structure packing, but never disable it.  The layout of
> > > structures defined in other headers can then depend on which order the
> > > various headers are included in, breaking the One Definition Rule.
> > > 
> > > This removes those directives and adds __packed to structure
> > > definitions where packing appears to have been intended.
> > > 
> > > Reported-by: Malcolm Priestley <tvboxspy@gmail.com>
> > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > 
> > Thanks Ben
> > 
> > I have tested the patch on 3.7 and 3.8-rc2 and it has resolved the
> > issue.
> 
> Great, can someone send me the patch so that I can apply it upstream?

Oh nevermind, you did on the 14th, I have it in my inbox...

greg k-h
diff mbox

Patch

--- a/drivers/staging/vt6656/bssdb.h
+++ b/drivers/staging/vt6656/bssdb.h
@@ -90,7 +90,6 @@  typedef struct tagSRSNCapObject {
 } SRSNCapObject, *PSRSNCapObject;
 
 // BSS info(AP)
-#pragma pack(1)
 typedef struct tagKnownBSS {
     // BSS info
     BOOL            bActive;
--- a/drivers/staging/vt6656/int.h
+++ b/drivers/staging/vt6656/int.h
@@ -34,7 +34,6 @@ 
 #include "device.h"
 
 /*---------------------  Export Definitions -------------------------*/
-#pragma pack(1)
 typedef struct tagSINTData {
 	BYTE byTSR0;
 	BYTE byPkt0;
--- a/drivers/staging/vt6656/iocmd.h
+++ b/drivers/staging/vt6656/iocmd.h
@@ -95,13 +95,12 @@  typedef enum tagWZONETYPE {
 // Ioctl interface structure
 // Command structure
 //
-#pragma pack(1)
 typedef struct tagSCmdRequest {
 	u8 name[16];
 	void	*data;
 	u16	    wResult;
 	u16     wCmdCode;
-} SCmdRequest, *PSCmdRequest;
+} __packed SCmdRequest, *PSCmdRequest;
 
 //
 // Scan
@@ -111,7 +110,7 @@  typedef struct tagSCmdScan {
 
     u8	    ssid[SSID_MAXLEN + 2];
 
-} SCmdScan, *PSCmdScan;
+} __packed SCmdScan, *PSCmdScan;
 
 //
 // BSS Join
@@ -126,7 +125,7 @@  typedef struct tagSCmdBSSJoin {
     BOOL    bPSEnable;
     BOOL    bShareKeyAuth;
 
-} SCmdBSSJoin, *PSCmdBSSJoin;
+} __packed SCmdBSSJoin, *PSCmdBSSJoin;
 
 //
 // Zonetype Setting
@@ -137,7 +136,7 @@  typedef struct tagSCmdZoneTypeSet {
  BOOL       bWrite;
  WZONETYPE  ZoneType;
 
-} SCmdZoneTypeSet, *PSCmdZoneTypeSet;
+} __packed SCmdZoneTypeSet, *PSCmdZoneTypeSet;
 
 typedef struct tagSWPAResult {
          char	ifname[100];
@@ -145,7 +144,7 @@  typedef struct tagSWPAResult {
 	u8 key_mgmt;
 	u8 eap_type;
          BOOL authenticated;
-} SWPAResult, *PSWPAResult;
+} __packed SWPAResult, *PSWPAResult;
 
 typedef struct tagSCmdStartAP {
 
@@ -157,7 +156,7 @@  typedef struct tagSCmdStartAP {
     BOOL    bShareKeyAuth;
     u8      byBasicRate;
 
-} SCmdStartAP, *PSCmdStartAP;
+} __packed SCmdStartAP, *PSCmdStartAP;
 
 typedef struct tagSCmdSetWEP {
 
@@ -167,7 +166,7 @@  typedef struct tagSCmdSetWEP {
     BOOL    bWepKeyAvailable[WEP_NKEYS];
     u32     auWepKeyLength[WEP_NKEYS];
 
-} SCmdSetWEP, *PSCmdSetWEP;
+} __packed SCmdSetWEP, *PSCmdSetWEP;
 
 typedef struct tagSBSSIDItem {
 
@@ -180,14 +179,14 @@  typedef struct tagSBSSIDItem {
     BOOL    bWEPOn;
     u32     uRSSI;
 
-} SBSSIDItem;
+} __packed SBSSIDItem;
 
 
 typedef struct tagSBSSIDList {
 
 	u32		    uItem;
 	SBSSIDItem	sBSSIDList[0];
-} SBSSIDList, *PSBSSIDList;
+} __packed SBSSIDList, *PSBSSIDList;
 
 
 typedef struct tagSNodeItem {
@@ -208,7 +207,7 @@  typedef struct tagSNodeItem {
     u32            uTxAttempts;
     u16            wFailureRatio;
 
-} SNodeItem;
+} __packed SNodeItem;
 
 
 typedef struct tagSNodeList {
@@ -216,7 +215,7 @@  typedef struct tagSNodeList {
 	u32		    uItem;
 	SNodeItem	sNodeList[0];
 
-} SNodeList, *PSNodeList;
+} __packed SNodeList, *PSNodeList;
 
 
 typedef struct tagSCmdLinkStatus {
@@ -229,7 +228,7 @@  typedef struct tagSCmdLinkStatus {
     u32     uChannel;
     u32     uLinkRate;
 
-} SCmdLinkStatus, *PSCmdLinkStatus;
+} __packed SCmdLinkStatus, *PSCmdLinkStatus;
 
 //
 // 802.11 counter
@@ -247,7 +246,7 @@  typedef struct tagSDot11MIBCount {
     u32 ReceivedFragmentCount;
     u32 MulticastReceivedFrameCount;
     u32 FCSErrorCount;
-} SDot11MIBCount, *PSDot11MIBCount;
+} __packed SDot11MIBCount, *PSDot11MIBCount;
 
 
 
@@ -355,13 +354,13 @@  typedef struct tagSStatMIBCount {
     u32   ullTxBroadcastBytes[2];
     u32   ullTxMulticastBytes[2];
     u32   ullTxDirectedBytes[2];
-} SStatMIBCount, *PSStatMIBCount;
+} __packed SStatMIBCount, *PSStatMIBCount;
 
 typedef struct tagSCmdValue {
 
     u32     dwValue;
 
-} SCmdValue,  *PSCmdValue;
+} __packed SCmdValue,  *PSCmdValue;
 
 //
 // hostapd & viawget ioctl related
@@ -431,7 +430,7 @@  struct viawget_hostapd_param {
 			u8 ssid[32];
 		} scan_req;
 	} u;
-};
+} __packed;
 
 /*---------------------  Export Classes  ----------------------------*/
 
--- a/drivers/staging/vt6656/iowpa.h
+++ b/drivers/staging/vt6656/iowpa.h
@@ -67,12 +67,11 @@  enum {
 
 
 
-#pragma pack(1)
 typedef struct viawget_wpa_header {
 	u8 type;
 	u16 req_ie_len;
 	u16 resp_ie_len;
-} viawget_wpa_header;
+} __packed viawget_wpa_header;
 
 struct viawget_wpa_param {
 	u32 cmd;
@@ -113,9 +112,8 @@  struct viawget_wpa_param {
 			u8 *buf;
 		} scan_results;
 	} u;
-};
+} __packed;
 
-#pragma pack(1)
 struct viawget_scan_result {
 	u8 bssid[6];
 	u8 ssid[32];
@@ -130,7 +128,7 @@  struct viawget_scan_result {
 	int noise;
 	int level;
 	int maxrate;
-};
+} __packed;
 
 /*---------------------  Export Classes  ----------------------------*/