Re: [edk2] [PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC
Reviewed-by: Fu Siyuan> -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Tuesday, April 3, 2018 10:52 PM > To: edk2-devel-01 > Cc: Wu, Jiaxin ; Gao, Liming ; > Kinney, Michael D ; Fu, Siyuan > > Subject: [PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from > the TLS RFC > > The structures defined in RFC 5246 are not to have any padding between > fields or at the end; use the "pack" pragma as necessary. > > Cc: Jiaxin Wu > Cc: Liming Gao > Cc: Michael D Kinney > Cc: Siyuan Fu > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > --- > MdePkg/Include/Protocol/Tls.h | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/MdePkg/Include/Protocol/Tls.h b/MdePkg/Include/Protocol/Tls.h > index 2119f33c0f5b..dafaabcd2a8b 100644 > --- a/MdePkg/Include/Protocol/Tls.h > +++ b/MdePkg/Include/Protocol/Tls.h > @@ -138,33 +138,37 @@ typedef enum { > /// > /// EFI_TLS_CIPHER > /// Note: The definition of EFI_TLS_CIPHER definition is from "RFC 5246, > A.4.1. > /// Hello Messages". The value of EFI_TLS_CIPHER is from TLS Cipher > /// Suite Registry of IANA. > /// > +#pragma pack (1) > typedef struct { >UINT8 Data1; >UINT8 Data2; > } EFI_TLS_CIPHER; > +#pragma pack () > > /// > /// EFI_TLS_COMPRESSION > /// Note: The value of EFI_TLS_COMPRESSION definition is from "RFC 3749". > /// > typedef UINT8 EFI_TLS_COMPRESSION; > > /// > /// EFI_TLS_EXTENSION > /// Note: The definition of EFI_TLS_EXTENSION if from "RFC 5246 A.4.1. > /// Hello Messages". > /// > +#pragma pack (1) > typedef struct { >UINT16ExtensionType; >UINT16Length; >UINT8 Data[1]; > } EFI_TLS_EXTENSION; > +#pragma pack () > > /// > /// EFI_TLS_VERIFY > /// Use either EFI_TLS_VERIFY_NONE or EFI_TLS_VERIFY_PEER, the last two > options > /// are 'ORed' with EFI_TLS_VERIFY_PEER if they are desired. > /// > @@ -191,35 +195,41 @@ typedef UINT32 EFI_TLS_VERIFY; > > /// > /// EFI_TLS_RANDOM > /// Note: The definition of EFI_TLS_RANDOM is from "RFC 5246 A.4.1. > /// Hello Messages". > /// > +#pragma pack (1) > typedef struct { >UINT32GmtUnixTime; >UINT8 RandomBytes[28]; > } EFI_TLS_RANDOM; > +#pragma pack () > > /// > /// EFI_TLS_MASTER_SECRET > /// Note: The definition of EFI_TLS_MASTER_SECRET is from "RFC 5246 8.1. > /// Computing the Master Secret". > /// > +#pragma pack (1) > typedef struct { >UINT8 Data[48]; > } EFI_TLS_MASTER_SECRET; > +#pragma pack () > > /// > /// EFI_TLS_SESSION_ID > /// Note: The definition of EFI_TLS_SESSION_ID is from "RFC 5246 A.4.1. > Hello Messages". > /// > #define MAX_TLS_SESSION_ID_LENGTH 32 > +#pragma pack (1) > typedef struct { >UINT16Length; >UINT8 Data[MAX_TLS_SESSION_ID_LENGTH]; > } EFI_TLS_SESSION_ID; > +#pragma pack () > > /// > /// EFI_TLS_SESSION_STATE > /// > typedef enum { >/// > -- > 2.14.1.3.gb7cf6e02401b > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC
On 04/03/18 17:08, Gao, Liming wrote: > Laszlo: > Could you use one pack to scope all structure definitions? I didn't do that originally because the affected structure definitions are spread out over a larger part of the code, intermixed with enums, #defines, and other structures, *and* it is not clear from the source code comments whether *all* of the structure definitions come from RFC 5246. Wherever the code comments did *not* indicate that a given structure came from the RFC, I didn't add packing either. Example structures for this: - EFI_TLS_VERSION - EFI_TLS_FRAGMENT_DATA Looking at these in more detail, I don't think either of them should be packed: - EfiTlsVersion / EFI_TLS_VERSION are used in TlsSetSessionData() [NetworkPkg/TlsDxe/TlsProtocol.c], and the constituent fields Major and Minor are passed individually to TlsSetVersion() [CryptoPkg/Library/TlsLib/TlsConfig.c]. So packing the structure is not necessary. - EFI_TLS_FRAGMENT_DATA contains a (VOID*) pointer, so it cannot be serialized to the wire by definition. And, it is used in TlsProcessPacket() [NetworkPkg/TlsDxe/TlsProtocol.c], and TlsEncryptPacket() / TlsDecryptPacket() [NetworkPkg/TlsDxe/TlsImpl.c], without any need for packing. Given that this protocol header file defines both packed and non-packed structures, without a very clear separation between them, I feel it is cleaner to use the pack #pragma for individual structures. If you disagree, then I think we should implement that "clear separation" in the header file first. Namely, move all the packed structures either after or before all the non-packed structures, and add a very visible comment that "all of the following structures come from RFC 5246 and need packing". Then we can use a single pack #pragma to cover them all. Thanks, Laszlo >> -Original Message- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Tuesday, April 3, 2018 10:52 PM >> To: edk2-devel-01>> Cc: Wu, Jiaxin ; Gao, Liming ; >> Kinney, Michael D ; Fu, >> Siyuan >> Subject: [PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from >> the TLS RFC >> >> The structures defined in RFC 5246 are not to have any padding between >> fields or at the end; use the "pack" pragma as necessary. >> >> Cc: Jiaxin Wu >> Cc: Liming Gao >> Cc: Michael D Kinney >> Cc: Siyuan Fu >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915 >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Laszlo Ersek >> --- >> MdePkg/Include/Protocol/Tls.h | 10 ++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/MdePkg/Include/Protocol/Tls.h b/MdePkg/Include/Protocol/Tls.h >> index 2119f33c0f5b..dafaabcd2a8b 100644 >> --- a/MdePkg/Include/Protocol/Tls.h >> +++ b/MdePkg/Include/Protocol/Tls.h >> @@ -138,33 +138,37 @@ typedef enum { >> /// >> /// EFI_TLS_CIPHER >> /// Note: The definition of EFI_TLS_CIPHER definition is from "RFC 5246, >> A.4.1. >> /// Hello Messages". The value of EFI_TLS_CIPHER is from TLS Cipher >> /// Suite Registry of IANA. >> /// >> +#pragma pack (1) >> typedef struct { >>UINT8 Data1; >>UINT8 Data2; >> } EFI_TLS_CIPHER; >> +#pragma pack () >> >> /// >> /// EFI_TLS_COMPRESSION >> /// Note: The value of EFI_TLS_COMPRESSION definition is from "RFC 3749". >> /// >> typedef UINT8 EFI_TLS_COMPRESSION; >> >> /// >> /// EFI_TLS_EXTENSION >> /// Note: The definition of EFI_TLS_EXTENSION if from "RFC 5246 A.4.1. >> /// Hello Messages". >> /// >> +#pragma pack (1) >> typedef struct { >>UINT16ExtensionType; >>UINT16Length; >>UINT8 Data[1]; >> } EFI_TLS_EXTENSION; >> +#pragma pack () >> >> /// >> /// EFI_TLS_VERIFY >> /// Use either EFI_TLS_VERIFY_NONE or EFI_TLS_VERIFY_PEER, the last two >> options >> /// are 'ORed' with EFI_TLS_VERIFY_PEER if they are desired. >> /// >> @@ -191,35 +195,41 @@ typedef UINT32 EFI_TLS_VERIFY; >> >> /// >> /// EFI_TLS_RANDOM >> /// Note: The definition of EFI_TLS_RANDOM is from "RFC 5246 A.4.1. >> /// Hello Messages". >> /// >> +#pragma pack (1) >> typedef struct { >>UINT32GmtUnixTime; >>UINT8 RandomBytes[28]; >> } EFI_TLS_RANDOM; >> +#pragma pack () >> >> /// >> /// EFI_TLS_MASTER_SECRET >> /// Note: The definition of EFI_TLS_MASTER_SECRET is from "RFC 5246 8.1. >> /// Computing the Master Secret". >> /// >> +#pragma pack (1) >> typedef struct { >>UINT8 Data[48]; >> } EFI_TLS_MASTER_SECRET; >> +#pragma pack () >> >> /// >> /// EFI_TLS_SESSION_ID >> /// Note: The definition of EFI_TLS_SESSION_ID is from "RFC 5246
Re: [edk2] [PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC
Laszlo: Could you use one pack to scope all structure definitions? Thanks Liming > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Tuesday, April 3, 2018 10:52 PM > To: edk2-devel-01> Cc: Wu, Jiaxin ; Gao, Liming ; > Kinney, Michael D ; Fu, > Siyuan > Subject: [PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from > the TLS RFC > > The structures defined in RFC 5246 are not to have any padding between > fields or at the end; use the "pack" pragma as necessary. > > Cc: Jiaxin Wu > Cc: Liming Gao > Cc: Michael D Kinney > Cc: Siyuan Fu > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > --- > MdePkg/Include/Protocol/Tls.h | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/MdePkg/Include/Protocol/Tls.h b/MdePkg/Include/Protocol/Tls.h > index 2119f33c0f5b..dafaabcd2a8b 100644 > --- a/MdePkg/Include/Protocol/Tls.h > +++ b/MdePkg/Include/Protocol/Tls.h > @@ -138,33 +138,37 @@ typedef enum { > /// > /// EFI_TLS_CIPHER > /// Note: The definition of EFI_TLS_CIPHER definition is from "RFC 5246, > A.4.1. > /// Hello Messages". The value of EFI_TLS_CIPHER is from TLS Cipher > /// Suite Registry of IANA. > /// > +#pragma pack (1) > typedef struct { >UINT8 Data1; >UINT8 Data2; > } EFI_TLS_CIPHER; > +#pragma pack () > > /// > /// EFI_TLS_COMPRESSION > /// Note: The value of EFI_TLS_COMPRESSION definition is from "RFC 3749". > /// > typedef UINT8 EFI_TLS_COMPRESSION; > > /// > /// EFI_TLS_EXTENSION > /// Note: The definition of EFI_TLS_EXTENSION if from "RFC 5246 A.4.1. > /// Hello Messages". > /// > +#pragma pack (1) > typedef struct { >UINT16ExtensionType; >UINT16Length; >UINT8 Data[1]; > } EFI_TLS_EXTENSION; > +#pragma pack () > > /// > /// EFI_TLS_VERIFY > /// Use either EFI_TLS_VERIFY_NONE or EFI_TLS_VERIFY_PEER, the last two > options > /// are 'ORed' with EFI_TLS_VERIFY_PEER if they are desired. > /// > @@ -191,35 +195,41 @@ typedef UINT32 EFI_TLS_VERIFY; > > /// > /// EFI_TLS_RANDOM > /// Note: The definition of EFI_TLS_RANDOM is from "RFC 5246 A.4.1. > /// Hello Messages". > /// > +#pragma pack (1) > typedef struct { >UINT32GmtUnixTime; >UINT8 RandomBytes[28]; > } EFI_TLS_RANDOM; > +#pragma pack () > > /// > /// EFI_TLS_MASTER_SECRET > /// Note: The definition of EFI_TLS_MASTER_SECRET is from "RFC 5246 8.1. > /// Computing the Master Secret". > /// > +#pragma pack (1) > typedef struct { >UINT8 Data[48]; > } EFI_TLS_MASTER_SECRET; > +#pragma pack () > > /// > /// EFI_TLS_SESSION_ID > /// Note: The definition of EFI_TLS_SESSION_ID is from "RFC 5246 A.4.1. > Hello Messages". > /// > #define MAX_TLS_SESSION_ID_LENGTH 32 > +#pragma pack (1) > typedef struct { >UINT16Length; >UINT8 Data[MAX_TLS_SESSION_ID_LENGTH]; > } EFI_TLS_SESSION_ID; > +#pragma pack () > > /// > /// EFI_TLS_SESSION_STATE > /// > typedef enum { >/// > -- > 2.14.1.3.gb7cf6e02401b > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC
The structures defined in RFC 5246 are not to have any padding between fields or at the end; use the "pack" pragma as necessary. Cc: Jiaxin WuCc: Liming Gao Cc: Michael D Kinney Cc: Siyuan Fu Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek --- MdePkg/Include/Protocol/Tls.h | 10 ++ 1 file changed, 10 insertions(+) diff --git a/MdePkg/Include/Protocol/Tls.h b/MdePkg/Include/Protocol/Tls.h index 2119f33c0f5b..dafaabcd2a8b 100644 --- a/MdePkg/Include/Protocol/Tls.h +++ b/MdePkg/Include/Protocol/Tls.h @@ -138,33 +138,37 @@ typedef enum { /// /// EFI_TLS_CIPHER /// Note: The definition of EFI_TLS_CIPHER definition is from "RFC 5246, A.4.1. /// Hello Messages". The value of EFI_TLS_CIPHER is from TLS Cipher /// Suite Registry of IANA. /// +#pragma pack (1) typedef struct { UINT8 Data1; UINT8 Data2; } EFI_TLS_CIPHER; +#pragma pack () /// /// EFI_TLS_COMPRESSION /// Note: The value of EFI_TLS_COMPRESSION definition is from "RFC 3749". /// typedef UINT8 EFI_TLS_COMPRESSION; /// /// EFI_TLS_EXTENSION /// Note: The definition of EFI_TLS_EXTENSION if from "RFC 5246 A.4.1. /// Hello Messages". /// +#pragma pack (1) typedef struct { UINT16ExtensionType; UINT16Length; UINT8 Data[1]; } EFI_TLS_EXTENSION; +#pragma pack () /// /// EFI_TLS_VERIFY /// Use either EFI_TLS_VERIFY_NONE or EFI_TLS_VERIFY_PEER, the last two options /// are 'ORed' with EFI_TLS_VERIFY_PEER if they are desired. /// @@ -191,35 +195,41 @@ typedef UINT32 EFI_TLS_VERIFY; /// /// EFI_TLS_RANDOM /// Note: The definition of EFI_TLS_RANDOM is from "RFC 5246 A.4.1. /// Hello Messages". /// +#pragma pack (1) typedef struct { UINT32GmtUnixTime; UINT8 RandomBytes[28]; } EFI_TLS_RANDOM; +#pragma pack () /// /// EFI_TLS_MASTER_SECRET /// Note: The definition of EFI_TLS_MASTER_SECRET is from "RFC 5246 8.1. /// Computing the Master Secret". /// +#pragma pack (1) typedef struct { UINT8 Data[48]; } EFI_TLS_MASTER_SECRET; +#pragma pack () /// /// EFI_TLS_SESSION_ID /// Note: The definition of EFI_TLS_SESSION_ID is from "RFC 5246 A.4.1. Hello Messages". /// #define MAX_TLS_SESSION_ID_LENGTH 32 +#pragma pack (1) typedef struct { UINT16Length; UINT8 Data[MAX_TLS_SESSION_ID_LENGTH]; } EFI_TLS_SESSION_ID; +#pragma pack () /// /// EFI_TLS_SESSION_STATE /// typedef enum { /// -- 2.14.1.3.gb7cf6e02401b ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel