Re: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE

["Michael D Kinney" <michael.d.kinney@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7569 bytes --]

Andrew,

My suggestion is to read the 3 bytes and shift and or to build 24-bit value.  That is what is in the patch at the bottom.  It uses an extra layer of macros that I am not in favor of. There is an additional email with a proposed approach that makes the use of the array members more obvious.

Mike

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Andrew Fish via Groups.Io
Sent: Wednesday, April 17, 2019 11:32 AM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: lersek@redhat.com; Gao, Liming <liming.gao@intel.com>
Subject: Re: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE

Mike,

I kind of ratholed us on alignment when Laszlo was more concerned about strict aliasing and the effective type rule. Sorry! I don't think your proposal fixes the effective type issue, and actually may make it worse if I'm correct.

It seems like the safest thing to do is read byte by byte and shift which I thought we did a long time ago?

The last point I poorly tried to make to Laszlo was I thought you could cast around the effective type issue in C99. If you think about it the union and the cast are both telling the compiler the intent is to break the effective type rule and that is the bigger point I was trying to make. Given my insomnia I used alignment examples that I understand better. If we try to convert Size to a UINT32 * I think that does trigger the effect type issue Laszlo referenced. So I was only debating the boundary of enforcement of the effect type rule using alignment as a confusing example.

I was actually writing a mail to some people that sit on the C/C++ standards committee that are UB experts to get some clarification when you sent this mail. I'm concerned I'm conflating behavior with what the standard states, and may have some recency bias with solving alignment issues that makes me think the cast should work.

I'm basically asking if this code pedantic conforms to C99 and C11:

EFI_COMMON_SECTION_HEADER gSec = { { 0x01, 0x02, 0x3 }, 0x10 };

return *(UINT32 *)gSec.Size & 0x00ffffff;

I ran the clang static analyzer and runtime ubsan on the above code and it did not complain (I force strict aliasing via -fstrict-aliasing, and I'm using the Sys V ABI since this is just the command line compiler on my Mac).

Thanks,

Andrew Fish


On Apr 17, 2019, at 10:52 AM, Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:

Laszlo,

I have been following this thread.  I think the style
used here to access the 3 array elements to build the
24-bit size value is the best approach.  I prefer this
over adding the union.

I agree there is a read overrun issue when using UINT32 to
read the Size[3] array contents.

I do not think this is a real issue in practice, because the
Size[3] array accessed is part of the larger
EFI_COMMON_SECTION_HEADER structure.  However, we always should
clean up code to not do any read/write overruns without this
type of analysis and the need to keep track of exceptions.

There is a related set of code in the BaseLib for Read/Write
Unaligned24().

UINT32
EFIAPI
ReadUnaligned24 (
 IN CONST UINT32              *Buffer
 );

UINT32
EFIAPI
WriteUnaligned24 (
 OUT UINT32                    *Buffer,
 IN  UINT32                    Value
 );

This API does not get flagged for read overrun issues because
a UINT32 is passed in.  However, for CPU archs that required aligned
access, the 24-bit value must be read in pieces.  This is why there
are 2 different implementations:

IA32/X64
========
UINT32
EFIAPI
ReadUnaligned24 (
 IN CONST UINT32              *Buffer
 )
{
 ASSERT (Buffer != NULL);

 return *Buffer & 0xffffff;
}


ARM/AARCH64
============
UINT32
EFIAPI
ReadUnaligned24 (
 IN CONST UINT32              *Buffer
 )
{
 ASSERT (Buffer != NULL);

 return (UINT32)(
           ReadUnaligned16 ((UINT16*)Buffer) |
           (((UINT8*)Buffer)[2] << 16)
           );
}

The ARM/ARCH64 implementation is clean because it does
not do a read overrun of the 24-bit field.  The IA32/X64
implementation may have an issue because it reads a 32-bit
value and strips the upper 8 bits.

If we apply the same technique to the Size field of
EFI_COMMON_SECTION_HEADER, then the 24-bit value would be
built from reading only the 3 bytes of the array.

Best regards,

Mike


-----Original Message-----
From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io]
On Behalf Of Laszlo Ersek
Sent: Friday, April 12, 2019 4:31 PM
To: edk2-devel-groups-io <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Cc: Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Kinney, Michael
D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Subject: [edk2-devel] [PATCH 04/10]
MdePkg/PiFirmwareFile: fix undefined behavior in
FFS_FILE_SIZE

Accessing "EFI_FFS_FILE_HEADER.Size", which is of type
UINT8[3], through a
(UINT32*), is undefined behavior. Fix it by accessing
the array elements
individually.

(We can't use a union here, unfortunately, as easily as
with
"EFI_COMMON_SECTION_HEADER", given the fields in
"EFI_FFS_FILE_HEADER".)

Cc: Liming Gao <liming.gao@intel.com<mailto:liming.gao@intel.com>>
Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Bugzilla:
https://bugzilla.tianocore.org/show_bug.cgi?id=1710
Signed-off-by: Laszlo Ersek <lersek@redhat.com<mailto: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 4fce8298d1c0..0668f3fa9af4 100644
--- a/MdePkg/Include/Pi/PiFirmwareFile.h
+++ b/MdePkg/Include/Pi/PiFirmwareFile.h
@@ -174,18 +174,26 @@ typedef struct {
  /// If FFS_ATTRIB_LARGE_FILE is not set then
EFI_FFS_FILE_HEADER is used.
  ///
  UINT64                    ExtendedSize;
} EFI_FFS_FILE_HEADER2;

#define IS_FFS_FILE2(FfsFileHeaderPtr) \
    (((((EFI_FFS_FILE_HEADER *) (UINTN)
FfsFileHeaderPtr)->Attributes) & FFS_ATTRIB_LARGE_FILE)
== FFS_ATTRIB_LARGE_FILE)

+#define FFS_FILE_SIZE_ARRAY(FfsFileHeaderPtr) \
+    (((EFI_FFS_FILE_HEADER *) (UINTN)
(FfsFileHeaderPtr))->Size)
+
+#define FFS_FILE_SIZE_ELEMENT(FfsFileHeaderPtr, Index)
\
+    ((UINT32) FFS_FILE_SIZE_ARRAY
(FfsFileHeaderPtr)[(Index)])
+
#define FFS_FILE_SIZE(FfsFileHeaderPtr) \
-    ((UINT32) (*((UINT32 *) ((EFI_FFS_FILE_HEADER *)
(UINTN) FfsFileHeaderPtr)->Size) & 0x00ffffff))
+    ((FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 0) <<
0) | \
+     (FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 1) <<
8) | \
+     (FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 2) <<
16))

#define FFS_FILE2_SIZE(FfsFileHeaderPtr) \
    ((UINT32) (((EFI_FFS_FILE_HEADER2 *) (UINTN)
FfsFileHeaderPtr)->ExtendedSize))

typedef UINT8 EFI_SECTION_TYPE;

///
/// Pseudo type. It is used as a wild card when
retrieving sections.
--
2.19.1.3.g30247aa5d201



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this
group.

View/Reply Online (#38989):
https://edk2.groups.io/g/devel/message/38989
Mute This Topic: https://groups.io/mt/31070304/1643496
Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io>
Unsubscribe: https://edk2.groups.io/g/devel/unsub
[michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>]
-=-=-=-=-=-=






[-- Attachment #2: Type: text/html, Size: 54145 bytes --]