public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: "Kinney, Michael D" <michael.d.kinney@intel.com>,
	"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2-devel] [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE
Date: Thu, 18 Apr 2019 11:06:52 +0200	[thread overview]
Message-ID: <e4d2c8dc-e5cb-2c4b-c494-fd511e90d1cf@redhat.com> (raw)
In-Reply-To: <E92EE9817A31E24EB0585FDF735412F5B9C9A4C1@ORSMSX113.amr.corp.intel.com>

On 04/17/19 20:31, Kinney, Michael D wrote:
> Laszlo,
>
> We should also be able to do a consistent fix without
> adding any new unions or macros:
>
> #define FFS_FILE_SIZE(FfsFileHeaderPtr) ((UINT32)( \
>   (((EFI_FFS_FILE_HEADER *) (UINTN) FfsFileHeaderPtr)->Size[0]       ) | \
>   (((EFI_FFS_FILE_HEADER *) (UINTN) FfsFileHeaderPtr)->Size[1] << 8  ) | \
>   (((EFI_FFS_FILE_HEADER *) (UINTN) FfsFileHeaderPtr)->Size[2] << 16 ) ))
>
> #define SECTION_SIZE(SectionHeaderPtr) ((UINT32)( \
>   (((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Size[0]       ) | \
>   (((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Size[1] << 8  ) | \
>   (((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Size[2] << 16 ) ))

I'll adopt this (see my other reply), with one small change: I think
I'll wrap "SectionHeaderPtr" in an extra set of parens.

My main reason for introducing the intermediate macros was that I wanted
to cast each UINT8 *individually* to UINT32, before applying the shift:

#define FFS_FILE_SIZE_ELEMENT(FfsFileHeaderPtr, Index) \
    ((UINT32) FFS_FILE_SIZE_ARRAY (FfsFileHeaderPtr)[(Index)])

#define FFS_FILE_SIZE(FfsFileHeaderPtr) \
    ((FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 0) <<  0) | \
     (FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 1) <<  8) | \
     (FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 2) << 16))

- Such casting is generally a good idea because UINT8 is promoted to
  INT32 as part of the integer promotions, and I consider bit-shifting
  signed integers bad hygiene (it carries a risk of undefined behavior).

- But then I felt that spelling out the same (UINT32) cast on every line
  of the expanded / flattened version of FFS_FILE_SIZE would make the
  replacement text simply too wide and hard to read. Hence the helper
  macros.

Now, in this particular case however (i.e. producing 24-bit unsigned
integers in the end), you have convinced me that casting to UINT32, as
the outermost operation only, is safe enough. Because, we never shift an
INT32 to the left by more than 16 bits, and that INT32 (pre-shift) is in
[0, UINT8_MAX] inclusive (because it comes from Size[2]). So the result
is always representable in an INT32.

... I've now also checked that no invocation of FFS_FILE_SIZE or
SECTION_SIZE in edk2 passes an argument with side effects, so evaluating
the arguments in the new macros multiple times should be safe. I might
add a new comment about this restriction.

Thanks!
Laszlo

  reply	other threads:[~2019-04-18  9:06 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-12 23:31 [PATCH 00/10] patches for some warnings raised by "RH covscan" Laszlo Ersek
2019-04-12 23:31 ` [PATCH 01/10] MdePkg/PiFirmwareFile: express IS_SECTION2 in terms of SECTION_SIZE Laszlo Ersek
2019-04-15 17:01   ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-12 23:31 ` [PATCH 02/10] MdePkg/PiFirmwareFile: fix undefined behavior in SECTION_SIZE Laszlo Ersek
2019-04-14  7:19   ` [edk2-devel] " Jordan Justen
2019-04-15 16:15     ` Laszlo Ersek
2019-04-16  8:28       ` Liming Gao
2019-04-16  9:04       ` Jordan Justen
2019-04-16 10:59         ` Laszlo Ersek
2019-04-16 16:50           ` Philippe Mathieu-Daudé
2019-04-17 10:08             ` Laszlo Ersek
2019-04-16 18:48           ` Jordan Justen
2019-04-16 23:25             ` Andrew Fish
2019-04-17 10:29               ` Laszlo Ersek
2019-04-17 11:44                 ` Andrew Fish
2019-04-17 14:59                   ` Laszlo Ersek
2019-04-17 19:35                     ` Jordan Justen
2019-04-18  9:38                       ` Laszlo Ersek
2019-04-18 15:18                         ` Liming Gao
2019-04-17 10:01             ` Laszlo Ersek
2019-04-12 23:31 ` [PATCH 03/10] BaseTools/PiFirmwareFile: " Laszlo Ersek
2019-04-12 23:31 ` [PATCH 04/10] MdePkg/PiFirmwareFile: fix undefined behavior in FFS_FILE_SIZE Laszlo Ersek
2019-04-15 17:23   ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-17 17:52   ` Michael D Kinney
2019-04-17 18:31     ` Michael D Kinney
2019-04-18  9:06       ` Laszlo Ersek [this message]
2019-04-17 18:31     ` Andrew Fish
2019-04-17 18:36       ` Michael D Kinney
2019-04-18  8:48         ` Laszlo Ersek
2019-04-18  8:45       ` Laszlo Ersek
2019-04-18 23:12         ` Laszlo Ersek
2019-04-18 17:20     ` Philippe Mathieu-Daudé
2019-04-18 17:59       ` Michael D Kinney
2019-04-18 18:12         ` Philippe Mathieu-Daudé
2019-04-12 23:31 ` [PATCH 05/10] OvmfPkg/Sec: fix out-of-bounds reads Laszlo Ersek
2019-04-15 17:24   ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-12 23:31 ` [PATCH 06/10] OvmfPkg/QemuVideoDxe: avoid arithmetic on null pointer Laszlo Ersek
2019-04-12 23:31 ` [PATCH 07/10] OvmfPkg/AcpiPlatformDxe: suppress invalid "deref of undef pointer" warning Laszlo Ersek
2019-04-15 17:26   ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-12 23:31 ` [PATCH 08/10] OvmfPkg: suppress "Value stored to ... is never read" analyzer warnings Laszlo Ersek
2019-04-14  8:03   ` [edk2-devel] " Jordan Justen
2019-04-15 16:25     ` Laszlo Ersek
2019-04-16  9:26       ` Jordan Justen
2019-04-16 11:44         ` Laszlo Ersek
2019-04-12 23:31 ` [PATCH 09/10] OvmfPkg/AcpiPlatformDxe: catch theoretical nullptr deref in Xen code Laszlo Ersek
2019-04-15 17:28   ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-12 23:31 ` [PATCH 10/10] OvmfPkg/BasePciCapLib: suppress invalid "nullptr deref" warning Laszlo Ersek
2019-04-15 17:31   ` [edk2-devel] " Philippe Mathieu-Daudé
2019-04-16 11:01     ` Laszlo Ersek
2019-04-12 23:36 ` [PATCH 00/10] patches for some warnings raised by "RH covscan" Ard Biesheuvel
2019-04-15 16:16   ` Laszlo Ersek
2019-04-18 14:20 ` [edk2-devel] " Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e4d2c8dc-e5cb-2c4b-c494-fd511e90d1cf@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox