public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Liming Gao" <liming.gao@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
	"lersek@redhat.com" <lersek@redhat.com>,
	"Justen, Jordan L" <jordan.l.justen@intel.com>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH 02/10] MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE
Date: Tue, 16 Apr 2019 08:28:30 +0000	[thread overview]
Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E4287CB@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <3bbbb85e-5557-d99b-1c3b-50a844455d20@redhat.com>

Laszlo:
  I think it is OK to add UNION type. UNION is not new data structure. It is convenience for developer to consume the data structure. This change is good to me. Reviewed-by: Liming Gao <liming.gao@intel.com>

Thanks
Liming
>-----Original Message-----
>From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>Laszlo Ersek
>Sent: Tuesday, April 16, 2019 12:16 AM
>To: Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel-groups-io
><devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@intel.com>
>Cc: Gao, Liming <liming.gao@intel.com>
>Subject: Re: [edk2-devel] [PATCH 02/10] MdePkg/PiFirmwareFile: fix
>undefined behavior in SECTION_SIZE
>
>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.
>
>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
>>
>
>
>


  reply	other threads:[~2019-04-16  8:28 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-12 23:31 [PATCH 00/10] patches for some warnings raised by "RH covscan" Laszlo Ersek
2019-04-12 23:31 ` [PATCH 01/10] MdePkg/PiFirmwareFile: express IS_SECTION2 in terms of SECTION_SIZE Laszlo Ersek
2019-04-15 17:01   ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-12 23:31 ` [PATCH 02/10] MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE Laszlo Ersek
2019-04-14  7:19   ` [edk2-devel] " Jordan Justen
2019-04-15 16:15     ` Laszlo Ersek
2019-04-16  8:28       ` Liming Gao [this message]
2019-04-16  9:04       ` Jordan Justen
2019-04-16 10:59         ` Laszlo Ersek
2019-04-16 16:50           ` Philippe Mathieu-Daudé
2019-04-17 10:08             ` Laszlo Ersek
2019-04-16 18:48           ` Jordan Justen
2019-04-16 23:25             ` Andrew Fish
2019-04-17 10:29               ` Laszlo Ersek
2019-04-17 11:44                 ` Andrew Fish
2019-04-17 14:59                   ` Laszlo Ersek
2019-04-17 19:35                     ` Jordan Justen
2019-04-18  9:38                       ` Laszlo Ersek
2019-04-18 15:18                         ` Liming Gao
2019-04-17 10:01             ` Laszlo Ersek
2019-04-12 23:31 ` [PATCH 03/10] BaseTools/PiFirmwareFile: " Laszlo Ersek
2019-04-12 23:31 ` [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE Laszlo Ersek
2019-04-15 17:23   ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-17 17:52   ` Michael D Kinney
2019-04-17 18:31     ` Michael D Kinney
2019-04-18  9:06       ` Laszlo Ersek
2019-04-17 18:31     ` Andrew Fish
2019-04-17 18:36       ` Michael D Kinney
2019-04-18  8:48         ` Laszlo Ersek
2019-04-18  8:45       ` Laszlo Ersek
2019-04-18 23:12         ` Laszlo Ersek
2019-04-18 17:20     ` Philippe Mathieu-Daudé
2019-04-18 17:59       ` Michael D Kinney
2019-04-18 18:12         ` Philippe Mathieu-Daudé
2019-04-12 23:31 ` [PATCH 05/10] OvmfPkg/Sec: fix out-of-bounds reads Laszlo Ersek
2019-04-15 17:24   ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-12 23:31 ` [PATCH 06/10] OvmfPkg/QemuVideoDxe: avoid arithmetic on null pointer Laszlo Ersek
2019-04-12 23:31 ` [PATCH 07/10] OvmfPkg/AcpiPlatformDxe: suppress invalid "deref of undef pointer" warning Laszlo Ersek
2019-04-15 17:26   ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-12 23:31 ` [PATCH 08/10] OvmfPkg: suppress "Value stored to ... is never read" analyzer warnings Laszlo Ersek
2019-04-14  8:03   ` [edk2-devel] " Jordan Justen
2019-04-15 16:25     ` Laszlo Ersek
2019-04-16  9:26       ` Jordan Justen
2019-04-16 11:44         ` Laszlo Ersek
2019-04-12 23:31 ` [PATCH 09/10] OvmfPkg/AcpiPlatformDxe: catch theoretical nullptr deref in Xen code Laszlo Ersek
2019-04-15 17:28   ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-12 23:31 ` [PATCH 10/10] OvmfPkg/BasePciCapLib: suppress invalid "nullptr deref" warning Laszlo Ersek
2019-04-15 17:31   ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-16 11:01     ` Laszlo Ersek
2019-04-12 23:36 ` [PATCH 00/10] patches for some warnings raised by "RH covscan" Ard Biesheuvel
2019-04-15 16:16   ` Laszlo Ersek
2019-04-18 14:20 ` [edk2-devel] " Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4A89E2EF3DFEDB4C8BFDE51014F606A14E4287CB@SHSMSX104.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox