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....@intel.com>
> >> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> >> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710
> >> Issue: scan-1007.txt
> >> Signed-off-by: Laszlo Ersek <ler...@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.
I guess it might more well defined to shift the bytes, like is
sometimes done with the FFS file sizes.
-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
> >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#39161): https://edk2.groups.io/g/devel/message/39161
Mute This Topic: https://groups.io/mt/31070302/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
