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 <[email protected]>
>>>> Cc: Michael D Kinney <[email protected]>
>>>> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710
>>>> Issue: scan-1007.txt
>>>> Signed-off-by: Laszlo Ersek <[email protected]>
>>>> ---
>>>> 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
>>>
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#39171): https://edk2.groups.io/g/devel/message/39171
Mute This Topic: https://groups.io/mt/31070302/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
