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

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

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)

Then again, I see SECTION_SIZE is not in the spec, so maybe it's ok to
add the typedef.

-Jordan