Re: [edk2-devel] [PATCH 02/10] MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE

["Laszlo Ersek" <lersek@redhat.com>]

Hi Andrew,

On 04/17/19 01:25, Andrew Fish wrote:
> 
> 
>> On Apr 16, 2019, at 11:48 AM, Jordan Justen <jordan.l.justen@intel.com> 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 <liming.gao@intel.com>
>>>>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>>>>> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710
>>>>>>> Issue: scan-1007.txt
>>>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>>>> ---
>>>>>>> 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;
> }

Thank you for this investigation! I really appreciate that you wrote it
up, and the example code makes your point clear.

If my understanding is correct, you are saying that, from an alignment
perspective, both the pre-patch code and the post-patch code are
correct. In other words, we might want to apply the patch, but there
isn't a strong necessity for it. Is my understanding correct?

If so, I'd like to point out that the original warning (see the commit
message) wasn't about alignment, but about "overrun":

-------
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"
-------

This warning was reported by "RH covscan" as "CPPCHECK_WARNING"; that
is, it comes from the cppcheck tool, not from clang-analyzer. So while I
agree that the pre-patch code may not imply an invalid alignment for the
compiler, I claim that the *overrun* (i.e., a different issue) does exist:

- The pre-patch code takes the Size member, which is an array of 3 UINT8
elements,
- in that context, the Size member is implicitly converted to a pointer
to the first UINT8 element of the array, i.e. &Size[0],
- the resultant pointer is explicitly cast to a (UINT32*), and then
dereferenced.

And the last step violates the effective type rules that I quoted
up-thread, because UINT32 is not a type that is compatible with UINT8.

> 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. 

I certainly accept (and thank you for) your explanation of the
alignment. The patch intends to fix an issue that is not
alignment-related though.

Thanks!
Laszlo

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