public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: Jordan Justen <jordan.l.justen@intel.com>,
	Michael D Kinney <michael.d.kinney@intel.com>,
	edk2-devel-groups-io <devel@edk2.groups.io>
Cc: Liming Gao <liming.gao@intel.com>
Subject: Re: [edk2-devel] [PATCH 02/10] MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE
Date: Tue, 16 Apr 2019 12:59:48 +0200	[thread overview]
Message-ID: <413ac018-bcf2-f510-00d0-33315974a3c2@redhat.com> (raw)
In-Reply-To: <155540548458.13612.11281694046292591090@jljusten-skl>

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.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.
> 
> 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:
>
>   — a type compatible with the effective type of the object,
>   — a qualified version of a type compatible with the effective type
>     of the object,
>   — a type that is the signed or unsigned type corresponding to the
>     effective type of the object,
>   — a type that is the signed or unsigned type corresponding to a
>     qualified version of the effective type of the object,
>   — 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
>   — 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
>>>
>>


  reply	other threads:[~2019-04-16 10:59 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
2019-04-16  9:04       ` Jordan Justen
2019-04-16 10:59         ` Laszlo Ersek [this message]
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=413ac018-bcf2-f510-00d0-33315974a3c2@redhat.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