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)
Then again, I see SECTION_SIZE is not in the spec, so maybe it's ok to
add the typedef.
-Jordan
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#39002): https://edk2.groups.io/g/devel/message/39002
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]
-=-=-=-=-=-=-=-=-=-=-=-
