> On Apr 16, 2019, at 11:48 AM, Jordan Justen wrote: > > On 2019-04-16 03:59:48, Laszlo Ersek wrote: >> On 04/16/19 11:04, Jordan Justen wrote: >>> On 2019-04-15 09:15:31, Laszlo Ersek wrote: >>>> On 04/14/19 09:19, Jordan Justen wrote: >>>>> On 2019-04-12 16:31:20, Laszlo Ersek wrote: >>>>>> RH covscan justifiedly reports that accessing >>>>>> "EFI_COMMON_SECTION_HEADER.Size", which is of type UINT8[3], through a >>>>>> (UINT32*), is undefined behavior: >>>>>> >>>>>>> Error: OVERRUN (CWE-119): >>>>>>> edk2-89910a39dcfd/OvmfPkg/Sec/SecMain.c:178: overrun-local: Overrunning >>>>>>> array of 3 bytes at byte offset 3 by dereferencing pointer >>>>>>> "(UINT32 *)((EFI_COMMON_SECTION_HEADER *)(UINTN)Section)->Size". >>>>>>> # 176| Section = (EFI_COMMON_SECTION_HEADER*)(UINTN) CurrentAddress; >>>>>>> # 177| >>>>>>> # 178|-> Size = SECTION_SIZE (Section); >>>>>>> # 179| if (Size < sizeof (*Section)) { >>>>>>> # 180| return EFI_VOLUME_CORRUPTED; >>>>>> >>>>>> Fix this by introducing EFI_COMMON_SECTION_HEADER_UNION, and expressing >>>>>> SECTION_SIZE() in terms of "EFI_COMMON_SECTION_HEADER_UNION.Uint32". >>>>>> >>>>>> Cc: Liming Gao >>>>>> Cc: Michael D Kinney >>>>>> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710 >>>>>> Issue: scan-1007.txt >>>>>> Signed-off-by: Laszlo Ersek >>>>>> --- >>>>>> MdePkg/Include/Pi/PiFirmwareFile.h | 10 +++++++++- >>>>>> 1 file changed, 9 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/MdePkg/Include/Pi/PiFirmwareFile.h b/MdePkg/Include/Pi/PiFirmwareFile.h >>>>>> index a9f3bcc4eb8e..4fce8298d1c0 100644 >>>>>> --- a/MdePkg/Include/Pi/PiFirmwareFile.h >>>>>> +++ b/MdePkg/Include/Pi/PiFirmwareFile.h >>>>>> @@ -229,16 +229,24 @@ typedef struct { >>>>>> /// >>>>>> UINT8 Size[3]; >>>>>> EFI_SECTION_TYPE Type; >>>>>> /// >>>>>> /// Declares the section type. >>>>>> /// >>>>>> } EFI_COMMON_SECTION_HEADER; >>>>>> >>>>>> +/// >>>>>> +/// Union that permits accessing EFI_COMMON_SECTION_HEADER as a UINT32 object. >>>>>> +/// >>>>>> +typedef union { >>>>>> + EFI_COMMON_SECTION_HEADER Hdr; >>>>>> + UINT32 Uint32; >>>>>> +} EFI_COMMON_SECTION_HEADER_UNION; >>>>>> + >>>>>> typedef struct { >>>>>> /// >>>>>> /// A 24-bit unsigned integer that contains the total size of the section in bytes, >>>>>> /// including the EFI_COMMON_SECTION_HEADER. >>>>>> /// >>>>>> UINT8 Size[3]; >>>>>> >>>>>> EFI_SECTION_TYPE Type; >>>>>> @@ -476,17 +484,17 @@ typedef struct { >>>>>> /// A UINT16 that represents a particular build. Subsequent builds have monotonically >>>>>> /// increasing build numbers relative to earlier builds. >>>>>> /// >>>>>> UINT16 BuildNumber; >>>>>> CHAR16 VersionString[1]; >>>>>> } EFI_VERSION_SECTION2; >>>>>> >>>>>> #define SECTION_SIZE(SectionHeaderPtr) \ >>>>>> - ((UINT32) (*((UINT32 *) ((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Size) & 0x00ffffff)) >>>>>> + (((EFI_COMMON_SECTION_HEADER_UNION *) (UINTN) (SectionHeaderPtr))->Uint32 & 0x00ffffff) >>>>> >>>>> Mike, all, >>>>> >>>>> Can we add a typedef for EFI_COMMON_SECTION_HEADER_UNION if it's not >>>>> in the PI spec? >>>>> >>>>> If it's not allowed, I think something like this might work too: >>>>> >>>>> #define SECTION_SIZE(SectionHeaderPtr) \ >>>>> (*((UINT32*)(UINTN)(SectionHeaderPtr)) & 0x00ffffff) >>>> >>>> (Less importantly:) >>>> >>>> It might shut up the static analyzer, but regarding the C standard, it's >>>> equally undefined behavior. >>> >>> I think you are still accessing it through a UINT32*, since you are >>> using a pointer to a union, and an field of type UINT32 within the >>> union. >> >> Using a union makes the behavior well-defined. >> >>> 6.2.7 Compatible type and composite type >>> >>> 1 Two types have compatible type if their types are the same. >>> Additional rules for determining whether two types are compatible >>> are described in [...] >> >>> 6.5 Expressions >>> >>> 6 The /effective type/ of an object for an access to its stored value >>> is the declared type of the object, if any. [...] >>> >>> 7 An object shall have its stored value accessed only by an lvalue >>> expression that has one of the following types: > > I think maybe this all applies to lvalues, not rvalues. The boon and > bane of C is that any pointer can be easily cast to a pointer of any > other type and then dereferenced. > > If the pointer is dereferenced to a type that is larger, then I > understand that there are cases where deferencing the pointer could > have unintended side effects, but that is not the case here. > > The dereference could also be undefined if big-endian was in the > picture, but UEFI restricts that. > > Of course none of this stops a tool from trying to delve further into > the pointer usage to look for possible issues. > > But, I don't see how any of this changes the fact that with or without > the union, we are dereferencing a UINT32 pointer. > The thing that is really saving us is the 4 byte alignment requirement for FV sections in the PI Spec, but that is not expressed by EFI_COMMON_SECTION_HEADER to the compiler. The union is better as it enforces alignment of the structure. This enforced alignment is likely what makes static analysis tools happy. I don't think the casting in the existing SECTION_SIZE is UB (Undefined Behavior) as the extraction is not the issue as you are telling the compiler to extract 4 unaligned bytes. That is the same as accessing a packed structure. But this is only because you started with a byte pointer so you are not really violating any C alignment rules. Thus in a case like our cast, or pragma pack(1), the compiler has extra alignment info provided so all is good, the issue is if you assign a pointer into the type the only alignment info the compiler has is the rules for that type, and non of packing rules. A lot of the above C rules are making it clear what the compiler does in these cases. Ironically the new union macro is undefined behavior at runtime during the cast if the section header is not 4 byte algined since you did not start with something (unsigned char pointer) that allowed byte access. See my example of the ubsan fault. The nice thing about the union is it tells the compiler our intended constraint from the PI spec. // This 5 byte structure will align EFI_COMMON_SECTION_HEADER on an odd byte boundary. typedef struct { UINT8 Count; EFI_COMMON_SECTION_HEADER Sec; } UNALIGNED_SECTION_HEADER; // This will properly align EFI_COMMON_SECTION_HEADER_UNION on a 4 byte boundary. typedef struct { UINT8 Count; EFI_COMMON_SECTION_HEADER_UNION Sec; } ALIGNED_SECTION_HEADER2; I tried to validate this with the clang ubsan... The only runtime UB was the new macro against UNALIGNED_SECTION_HEADER. ALIGNED_SECTION_HEADER2 with the new union aligned everything correctly and works fine. ./a.out Fv.c:55:11: runtime error: member access within misaligned address 0x000104b23021 for type 'EFI_COMMON_SECTION_HEADER_UNION', which requires 4 byte alignment 0x000104b23021: note: pointer points here 00 00 00 ff 01 02 03 10 00 00 00 aa 00 00 00 01 02 03 10 2a 2f b2 04 01 00 00 00 34 00 00 00 0d ^ Fv.c:55:11: runtime error: load of misaligned address 0x000104b23021 for type 'UINT32' (aka 'unsigned int'), which requires 4 byte alignment 0x000104b23021: note: pointer points here 00 00 00 ff 01 02 03 10 00 00 00 aa 00 00 00 01 02 03 10 2a 2f b2 04 01 00 00 00 34 00 00 00 0d ^ $ clang -fsanitize=undefined Fv.c $ cat Fv.c typedef unsigned char UINT8; typedef UINT8 EFI_SECTION_TYPE; typedef unsigned int UINT32; typedef unsigned long long UINTN; typedef UINT8 EFI_SECTION_TYPE; /// /// Common section header. /// typedef struct { /// /// A 24-bit unsigned integer that contains the total size of the section in bytes, /// including the EFI_COMMON_SECTION_HEADER. /// UINT8 Size[3]; EFI_SECTION_TYPE Type; /// /// Declares the section type. /// } EFI_COMMON_SECTION_HEADER; typedef union { EFI_COMMON_SECTION_HEADER Hdr; UINT32 Uint32; } EFI_COMMON_SECTION_HEADER_UNION; #define SECTION_SIZE(SectionHeaderPtr) \ ((UINT32) (*((UINT32 *) ((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Size) & 0x00ffffff)) #define SECTION_SIZE2(SectionHeaderPtr) \ (((EFI_COMMON_SECTION_HEADER_UNION *) (UINTN) (SectionHeaderPtr))->Uint32 & 0x00ffffff) typedef struct { UINT8 Count; EFI_COMMON_SECTION_HEADER Sec; } UNALIGNED_SECTION_HEADER; typedef struct { UINT8 Count; EFI_COMMON_SECTION_HEADER_UNION Sec; } ALIGNED_SECTION_HEADER2; UNALIGNED_SECTION_HEADER gSection = { 0xff, { { 0x01, 0x02, 0x3 }, 0x10 } }; ALIGNED_SECTION_HEADER2 gSection2 = { 0xaa, { } }; int main (int argc, char **argv) { UINT32 Size, Size2; gSection2.Sec.Uint32 = 0x10030201; Size = SECTION_SIZE(&gSection.Sec); Size2 = SECTION_SIZE2(&gSection.Sec); // runtime error: Size = SECTION_SIZE(&gSection2.Sec); Size2 = SECTION_SIZE2(&gSection2.Sec); return Size == Size2; } Thanks, Andrew Fish PS If folks disagree I know a person that owns the clang ubsan architecture and sits on the C/C++ standards committees. I had a long talk with this person recently about C alignment UB and I think I understand it a little better from the compiler point of view now. > -Jordan > >>> >>> \u2014 a type compatible with the effective type of the object, >>> \u2014 a qualified version of a type compatible with the effective type >>> of the object, >>> \u2014 a type that is the signed or unsigned type corresponding to the >>> effective type of the object, >>> \u2014 a type that is the signed or unsigned type corresponding to a >>> qualified version of the effective type of the object, >>> \u2014 an aggregate or union type that includes one of the aforementioned >>> types among its members (including, recursively, a member of a >>> subaggregate or contained union), or >>> \u2014 a character type. >> >> - Regarding 6.5p6, the original object we intend to access has >> (declared) type EFI_COMMON_SECTION_HEADER. Therefore the effective type >> is EFI_COMMON_SECTION_HEADER. >> >> - Based on 6.2.7p1, EFI_COMMON_SECTION_HEADER is compatible with >> EFI_COMMON_SECTION_HEADER. (Because they are the same.) >> >> - Based on 6.5p7 item #5, EFI_COMMON_SECTION_HEADER can be accessed as >> EFI_COMMON_SECTION_HEADER_UNION, because EFI_COMMON_SECTION_HEADER_UNION >> includes "a type compatible with the effective type of the object" (#1) >> among its members -- namely an EFI_COMMON_SECTION_HEADER, which is >> compatible with EFI_COMMON_SECTION_HEADER, because they are the same. >> >>> I guess it might more well defined to shift the bytes, like is >>> sometimes done with the FFS file sizes. >> >> I did that (i.e. byte-shifting) in the other patch: >> >> [edk2-devel] [PATCH 04/10] >> MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE >> >> but for SECTION_SIZE, the union is well-defined too. >> >> Thanks, >> Laszlo >> >>> >>> -Jordan >>> >>>> Anyway I don't feel too strongly about this, given that we disable the >>>> strict aliasing / effective type rules in "tools_def.template" >>>> ("-fno-strict-aliasing"). >>>> >>>>> Then again, I see SECTION_SIZE is not in the spec, so maybe it's ok to >>>>> add the typedef. >>>> >>>> (More importantly:) >>>> >>>> Indeed the doubt you voice about ..._UNION crossed my mind, but then I >>>> too searched the PI spec for SECTION_SIZE, with no hits. >>>> >>>> Beyond that, I searched both the PI and UEFI specs, for "_UNION" -- >>>> again no hits, despite our definitions of: >>>> >>>> - EFI_IMAGE_OPTIONAL_HEADER_UNION >>>> - EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION >>>> >>>> in >>>> >>>> - "MdePkg/Include/IndustryStandard/PeImage.h" >>>> - "MdePkg/Include/Protocol/GraphicsOutput.h" >>>> >>>> respectively. >>>> >>>> Thanks, >>>> Laszlo >>>> >>>>> >>>>> -Jordan >>>>> >>>> >> > > >