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....@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.
>
> 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
> >>>
> >>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#39186): https://edk2.groups.io/g/devel/message/39186
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]
-=-=-=-=-=-=-=-=-=-=-=-
