On Apr 16, 2019, at 11:48 AM, Jordan Justen <jordan.l.justen@intel.com> 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.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:

I think maybe this all applies to lvalues, not rvalues. 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.


The thing that is really saving us is the 4 byte alignment requirement for FV sections in the PI Spec, but that is not expressed by EFI_COMMON_SECTION_HEADER to the compiler. The union is better as it enforces alignment of the structure. This enforced alignment is likely what makes static analysis tools happy. 

I don't think the casting in the existing SECTION_SIZE is UB (Undefined Behavior) as the extraction is not the issue as you are telling the compiler to extract 4 unaligned bytes. That is the same as accessing a packed structure. But this is only because you started with a byte pointer so you are not really violating any C alignment rules. Thus in a case like our cast, or pragma pack(1),  the compiler has extra alignment info provided so all is good, the issue is if you assign a pointer into the type the only alignment info the compiler has is the rules for that type, and non of packing rules. A lot of the above C rules are making it clear what the compiler does in these cases. 

Ironically the new union macro is undefined behavior at runtime during the cast if the section header is not 4 byte algined since you did not start with something (unsigned char pointer) that allowed byte access. See my example of the ubsan fault. 

The nice thing about the union is it tells the compiler our intended constraint from the PI spec. 

// This 5 byte structure will align EFI_COMMON_SECTION_HEADER on an odd byte boundary. 
typedef struct {
  UINT8                     Count;
  EFI_COMMON_SECTION_HEADER Sec;
} UNALIGNED_SECTION_HEADER;

// This will properly align EFI_COMMON_SECTION_HEADER_UNION on a 4 byte boundary. 
typedef struct {
  UINT8                           Count;
  EFI_COMMON_SECTION_HEADER_UNION Sec;
} ALIGNED_SECTION_HEADER2;

I tried to validate this with the clang ubsan... The only runtime UB was the new macro against UNALIGNED_SECTION_HEADER. ALIGNED_SECTION_HEADER2 with the new union aligned everything correctly and works fine. 

./a.out
Fv.c:55:11: runtime error: member access within misaligned address 0x000104b23021 for type 'EFI_COMMON_SECTION_HEADER_UNION', which requires 4 byte alignment
0x000104b23021: note: pointer points here
 00 00 00  ff 01 02 03 10 00 00 00  aa 00 00 00 01 02 03 10  2a 2f b2 04 01 00 00 00  34 00 00 00 0d
              ^ 
Fv.c:55:11: runtime error: load of misaligned address 0x000104b23021 for type 'UINT32' (aka 'unsigned int'), which requires 4 byte alignment
0x000104b23021: note: pointer points here
 00 00 00  ff 01 02 03 10 00 00 00  aa 00 00 00 01 02 03 10  2a 2f b2 04 01 00 00 00  34 00 00 00 0d
              ^ 

$ clang -fsanitize=undefined Fv.c
$ cat Fv.c
typedef unsigned char       UINT8;
typedef UINT8               EFI_SECTION_TYPE;
typedef unsigned int        UINT32;
typedef unsigned long long  UINTN;

typedef UINT8 EFI_SECTION_TYPE;

///
/// Common section header.
/// 
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;
  ///
  /// Declares the section type.
  ///
} EFI_COMMON_SECTION_HEADER;

typedef union {
  EFI_COMMON_SECTION_HEADER Hdr;
  UINT32                    Uint32;
} EFI_COMMON_SECTION_HEADER_UNION;


#define SECTION_SIZE(SectionHeaderPtr) \
    ((UINT32) (*((UINT32 *) ((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Size) & 0x00ffffff))

#define SECTION_SIZE2(SectionHeaderPtr) \
    (((EFI_COMMON_SECTION_HEADER_UNION *) (UINTN) (SectionHeaderPtr))->Uint32 & 0x00ffffff)


typedef struct {
  UINT8                     Count;
  EFI_COMMON_SECTION_HEADER Sec;
} UNALIGNED_SECTION_HEADER;

typedef struct {
  UINT8                           Count;
  EFI_COMMON_SECTION_HEADER_UNION Sec;
} ALIGNED_SECTION_HEADER2;

UNALIGNED_SECTION_HEADER  gSection  = { 0xff, { { 0x01, 0x02, 0x3 }, 0x10 } };
ALIGNED_SECTION_HEADER2   gSection2 = { 0xaa, {   } };
int
main (int argc, char **argv)
{
  UINT32 Size, Size2;
  gSection2.Sec.Uint32 = 0x10030201;
  
  Size  = SECTION_SIZE(&gSection.Sec);
  Size2 = SECTION_SIZE2(&gSection.Sec);  // runtime error:

  Size  = SECTION_SIZE(&gSection2.Sec);
  Size2 = SECTION_SIZE2(&gSection2.Sec);
  return Size == Size2;
}

Thanks,

Andrew Fish

PS If folks disagree I know a person that owns the clang ubsan architecture and sits on the C/C++ standards committees. I had a long talk with this person recently about C alignment UB and I think I understand it a little better from the compiler point of view now. 

-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