From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.85.221.68, mailfrom: philmd@redhat.com) Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by groups.io with SMTP; Tue, 16 Apr 2019 09:50:53 -0700 Received: by mail-wr1-f68.google.com with SMTP id q1so28094170wrp.0 for ; Tue, 16 Apr 2019 09:50:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:openpgp:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=WoZU4v77up6mOOTBcVb5AfmAawVxduIYW2YfGOyDoHQ=; b=A61EnBee0Ua8wg9nAGCPaxM//b1Wlal8gue7sUuxL2+4fO1tBJqUF0ARqdSt+LCMt+ BP2k4ZPkbqc04hQ0rfZGqBvvt+2LPm5vFB1wRCWyFPqSOfzUHvmWe5wNTA5mvhaJh3mn Mx/8djpM7v6bFHAJHwJKCVJM2eG+qi2ZsKR41v3kplm6wN62Y2fMiq4zMhjZiRJlF3VW r9TYpG6pDSk/miH4jqhj5TdQbWgjKLXDjClrU+P0N66nJelfMB1Eeq0mLA0yMJ4HR5sq HildHWOfYyp829ZuCEPWFN0c5fgieHVgqwq5hzkwl2Qev3lOcj1Egok8PlBF/Z1aLYJ1 0ILw== X-Gm-Message-State: APjAAAUEpHu4b1hUqp0X/K7o6aYf9VYkVakxb4jOn06WsOXZtfEMwVrq gbPv3Bwar7YTTfVBPHa8dRi3sA== X-Google-Smtp-Source: APXvYqwFgbYHrpJDPgbXQEa7Pd9kw5brbyHkBU5i9OgzoOLUy22rsgciA83eAkWSFF5r1f5LRsv78Q== X-Received: by 2002:a5d:4710:: with SMTP id y16mr45195118wrq.176.1555433451354; Tue, 16 Apr 2019 09:50:51 -0700 (PDT) Return-Path: Received: from [192.168.1.33] (193.red-88-21-103.staticip.rima-tde.net. [88.21.103.193]) by smtp.gmail.com with ESMTPSA id g10sm47603062wrq.2.2019.04.16.09.50.50 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Tue, 16 Apr 2019 09:50:50 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 02/10] MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE To: devel@edk2.groups.io, lersek@redhat.com, Jordan Justen , Michael D Kinney Cc: Liming Gao References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-3-lersek@redhat.com> <155522637661.21857.4743822681286277764@jljusten-skl> <3bbbb85e-5557-d99b-1c3b-50a844455d20@redhat.com> <155540548458.13612.11281694046292591090@jljusten-skl> <413ac018-bcf2-f510-00d0-33315974a3c2@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Openpgp: id=89C1E78F601EE86C867495CBA2A3FD6EDEADC0DE; url=http://pgp.mit.edu/pks/lookup?op=get&search=0xA2A3FD6EDEADC0DE Message-ID: <5202f61c-10db-c535-1d17-0f08764de1a9@redhat.com> Date: Tue, 16 Apr 2019 18:50:49 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <413ac018-bcf2-f510-00d0-33315974a3c2@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 >>>>> Cc: Michael D Kinney >>>>> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710 >>>>> Issue: scan-1007.txt >>>>> Signed-off-by: Laszlo Ersek >>>>> --- >>>>> 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 >>>> >>>