On 04/16/19 20:48, Jordan Justen wrote:
> 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.
Why "maybe"? The standard explicitly writes "lvalue expression" above.
And (PointerToUnion->Member) is equivalent to
((*PointerToUnion).Member); and (*PointerToUnion) is an lvalue.
> 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.
Post-patch, we are *not* dereferencing a (UINT32*). We are dereferencing
a (EFI_COMMON_SECTION_HEADER_UNION*), and then accessing the Uint32
member in the pointed-to EFI_COMMON_SECTION_HEADER_UNION.
If we continued dereferencing a (UINT32*), then the effective type rules
that I quoted would indeed not apply. But that's not what the patch does.
Thanks
Laszlo
>
> -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 (#39242): https://edk2.groups.io/g/devel/message/39242
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]
-=-=-=-=-=-=-=-=-=-=-=-
