Hi Laszlo,
On 4/16/19 12:59 PM, 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:
>>
>> — 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.
Why not use a 8-bit access macro similar to FFS_FILE_SIZE(), the one you
added in patch 4/10 of this series?
>
> 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 (#39182): https://edk2.groups.io/g/devel/message/39182
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]
-=-=-=-=-=-=-=-=-=-=-=-
