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

["Jordan Justen" <jordan.l.justen@intel.com>]

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.

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