On 15 July 2016 at 10:48, Laszlo Ersek <[email protected]> wrote:
> On 07/15/16 10:40, Ard Biesheuvel wrote:
>> On 15 July 2016 at 08:33, Laszlo Ersek <[email protected]> wrote:
>>> On 07/15/16 08:07, Ard Biesheuvel wrote:
>>>> On 15 July 2016 at 01:27, Laszlo Ersek <[email protected]> wrote:
>>>
>>>>> First, the build tests. I built OVMF 84 times, with the following
>>>>> settings:
>>>>>
>>>>> * Dimension 1: whether your and Steven's patches were applied or not.
>>>>
>>>> I take it this means this series only?
>>>
>>> Ah, yes, sorry about the ambiguity -- with "your and Steven's patches" I
>>> meant the five patches in this series -- one patch from Steven, four
>>> patches from you.
>>>
>>>>> * However, then I built my program -- find it attached -- with -b DEBUG
>>>>> and -b RELEASE too, and the -b RELEASE binary is broken. Here's the
>>>>> difference in output:
>>>>>
>>>>> FS2:\> DebugEnrollDefaultKeys.efi
>>>>> info: SetupMode=1 SecureBoot=0 SecureBootEnable=1 CustomMode=1
>>>>> VendorKeys=0
>>>>> info: SetupMode=0 SecureBoot=1 SecureBootEnable=1 CustomMode=0
>>>>> VendorKeys=0
>>>>> info: success
>>>>>
>>>>> versus
>>>>>
>>>>> FS2:\> OptEnrollDefaultKeys.efi
>>>>> info: SetupMode=1 SecureBoot=0 SecureBootEnable=1 CustomMode=1
>>>>> VendorKeys=0
>>>>> error: EnrollListOfX509Certs("db",
>>>>> D719B2CB-3D3A-4596-A3BC-DAD00E67656F): Invalid Parameter
>>>>>
>>>>> I don't know why this happens, but it definitely relates to varargs --
>>>>> the program uses them liberally.
>>>>>
>>>>
>>>> If this code ultimately uses __builtin_ms_va_lists with -O2 and fails,
>>>> it makes sense to report it to the GCC maintainers (assuming we can
>>>> create a test case). I noticed that this code iterates over the same
>>>> VA_LIST twice, I wonder how well that was tested ...
>>>
>>> You make a good point about using VA_LIST twice possibly tickling gcc
>>> the wrong way. Plus, there's one (VOID)VA_ARG() macro invocation in my
>>> code -- in order to step over some arguments --, which might not be all
>>> that usual as well.
>>>
>>> In any case, the way I use varargs seems to be standards conformant.
>>>
>>> With regard to reporting this to gcc developers: I won't try that. I'm
>>> discouraged by two facts:
>>>
>>> * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50818 -- open since
>>> October 2011. You'll find some recent comments from Steven and David in
>>> it :)
>>>
>>> * http://thread.gmane.org/gmane.comp.bios.tianocore.devel/10963 --
>>> shortly after I ran into the -Os corruption issue with GCC48, I created
>>> and posted this small reproducer. Creating the reproducer wasn't
>>> trivial. In parallel I sent the same reproducer to one of my (indirect)
>>> colleagues at Red Hat, who had been a veteran in upstream GCC
>>> development and maintenance. I also asked him how/where I should report
>>> the bug. I got no answer from him.
>>>
>>> If you'd like to report a GCC bug, please go ahead, but I won't waste my
>>> time :)
>>>
>>
>> It seems that it is your testcase that is broken:
>>
>> --- a/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c
>> +++ b/OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.c
>> @@ -663,6 +663,8 @@ EnrollListOfX509Certs (
>> UINT8 *Data;
>> UINT8 *Position;
>>
>> + Status = EFI_SUCCESS;
>> +
>> //
>> // compute total size first, for UINT32 range check, and allocation
>> //
>>
>> With that change, everything works fine for me
>
> I agree that proving my code wrong would be the best outcome here, but
> can you please explain to me on what path we return from
> EnrollListOfX509Certs() without setting Status?
>
Well, it seems that in the success case, we are supposed to end up at line 692
if (EFI_ERROR (Status)) {
goto Out;
}
and expect the condition to be FALSE. If we have successfully tallied
the size of at least one cert, 'DataSize == sizeof *SingleHeader' will
be FALSE, and so Status will never have been assigned a value.
Apparently, the optimizer sees some kind of undefined behavior here
which it can exploit to bail unconditionally (or Status happens to be
0 under -O0)
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
