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

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

On 04/17/19 21:35, Jordan Justen wrote:
> On 2019-04-17 07:59:41, Laszlo Ersek wrote:
>> 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.
> 
> I did have a (slight) concern about adding a typedef to a public
> header that wasn't in the spec. It seemed like something that someone
> somewhere might not like in case it could interfere with future
> versions of the spec. According to Liming, we don't have to worry
> about that.
> 
> Regarding the UINT32* discussion, I didn't think the union really
> would make a difference vs skipping the union and casting the original
> struct pointer directly to a UINT32*.

The gcc docs on "-fstrict-aliasing" seem to suggest the same, using an
example -- but to me that example looks wrong (it seems well-defined,
based on the same rules I quoted from the standard); i.e. that looks
like a bug to me in the gcc docs. I've contacted a colleague at RH in
the toolchain team about it.

> I can see Andrew's point that the union may add some alignment
> assumptions to the dereference, so I can see how that does potentially
> change something subtle. Maybe on some machines it will allow for more
> efficient reading of the data with the (valid) alignment assumption.
> 
> I was also not seeing why you were saying it produced *undefined*
> results. I don't think it does in our case, but when you point out
> that we are aliasing data access, I can see how that quickly gets into
> *undefined* territory from a compiler's perspective.

Right, I approached this purely from the standard's perspective. The
standard says, in "4. Conformance":

1 In this International Standard, ‘‘shall’’ is to be interpreted as a
  requirement on an implementation or on a program; conversely, ‘‘shall
  not’’ is to be interpreted as a prohibition.

2 If a ‘‘shall’’ or ‘‘shall not’’ requirement that appears outside of a
  constraint is violated, the behavior is undefined. Undefined behavior
  is otherwise indicated in this International Standard by the words
  ‘‘undefined behavior’’ or by the omission of any explicit definition
  of behavior. There is no difference in emphasis among these three;
  they all describe ‘‘behavior that is undefined’’.

In this particular case, 6.5p7 goes "An object *shall* have its stored
value accessed *only* by [...]" (emphasis mine), and that paragraph
doesn't fall in a Constraints section, so 4p2 clearly applies.

That's why I called it undefined -- it might work perfectly well in
practice on all the edk2 platforms, possibly due to our carefully
selected compiler switches, but that's not what static analyzers care
about. AIUI they only care about the standard, hence my focus on the
standard for the fix.

(

Side note: the classification "outside of a constraint" in 4p2 above is
relevant because we also have:

5.1.1.3 Diagnostics

1 A conforming implementation shall produce at least one diagnostic
  message (identified in an implementation-defined manner) if a
  preprocessing translation unit or translation unit contains a
  violation of any syntax rule or constraint, even if the behavior is
  also explicitly specified as undefined or implementation-defined.
  [...]

IOW, if we break a "shall" or "shall not" that is under a Constraint,
then the compiler *must* yell at us; it cannot allow the UB to slip our
attention.

)

> Anyway, given Liming's feedback that it is ok to add the union, I'm ok
> with this patch.

Funnily enough, in parallel, Mike has expressed a preference for the
byte-wise access plus the shifting (without helper macros) :)

I don't see why Liming would dislike that, and I gather you too would
still prefer that to the union, so I'm going to incorporate it in v2.

Thank you!
Laszlo

> 
>