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

["Laszlo Ersek" <lersek@redhat.com>]

On 04/17/19 13:44, Andrew Fish wrote:

> Sorry I digressed into the C specification discussion, and did not
> deal with the patch in general. My point is the original code is legal
> C code. If you lookup CWE-119 it is written as a restriction on what
> the C language allows.
>
> As I mentioned casting to specific alignment is legal, as is defining
> a structure that is pragma pack(1) that can make a UINT32 not be 4
> byte aligned. Thus the cast created a legal UINT32 value. A cast to
> *(UINT32 *) is different that a cast to (UINT32 *). The rules you
> quote a triggered by the = and not the cast.
>
> Thus this is undefined behavior in C:
>   UINT32 *Ub = (UINT32 *)gSection.Sec.Size;
>   Size = *Ub & 0x00ffffff;
>
> And this is not Undefined behavior:
>   UINT32 NotUb = *(UINT32 *)gSection.Sec.Size & 0x00ffffff;

I agree the 2nd snippet may not be UB due to alignment violations
*alone*.

It is still UB due to violating the effective type rules.

> I also had a hard time interpreting what C spec was saying, but
> talking to the people who write the compiler and ubsan cleared it up
> for me. It also makes sense when you think about it. If you tell the
> compiler *(UINT32 *) it can know to generate byte reads if the
> hardware requires aligned access. If you do a (UINT32 *) that new
> pointer no longer carries the information about the alignment
> requirement. Thus the  *(UINT32 *) cast is like making a packed
> structure.

Yes, I think I'm clear on how the alignment information is carried
around, and when it is lost. In your first example above, due to us
forming a separate (standalone) pointer, we lose the alignment
information, and then the assignment is undefined due to an alignment
violation. While in the second example, the alignment information is not
lost, and the assignment is not undefined on an alignment basis *alone*.

However: the second assignment is *still* undefined, because it violates
the effective type rules. Here's a more direct example for the same:

STATIC UINT64 mUint64;

int main(void)
{
  UINT16 *Uint16Ptr;

  Uint16Ptr = (UINT16 *)&mUint64;
  *Uint16Ptr = 1;
  return 0;
}

The assignment to (*Uint16Ptr) is fine from the alignment perspective,
but it is nevertheless undefined, because it breaks the effective type
rules. Namely, UINT16 (the type used for the access) is not compatible
with UINT64 (the effective type of mUint64).

Normally, we don't care about this situation in edk2 -- in fact we write
loads of code like the above --, but we get away with that only because
we force the toolchains to ignore the effective type rules. For GCC in
particular, the option is "-fno-strict-aliasing".

The only reason I've posted this patch is that "cppcheck" (invoked as a
part of "RH covscan") doesn't care about "-fno-strict-aliasing". And
while "cppcheck" happens to report the overrun, and not the type
punning, the way to remove the warning is to adhere to all the C rules
in the expression, even though we have "-fno-strict-aliasing" in place.

> I agree the union is a good solution to CWE-119 and it better matches
> the alignment requirement in the PI spec.

Thank you.

I'll wait a bit longer to see if Jordan accepts this 02/10 patch based
on the most recent comments, and whether Liming or Mike accepts 04/10.

If Jordan remains unconvinced on SECTION_SIZE (in this 02/10 patch), and
Liming or Mike are fine with 04/10, I can rework 02/10 to follow 04/10.

If Jordan remains unconvinced but Mike or Liming prefers the union, then
we have a stalemate and I'll abandon the patch set.

Thanks,
Laszlo