On 07/15/16 10:58, Ard Biesheuvel wrote: > 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) >
You are perfectly right. Thank you for catching this! And, to our collective relief, this means that there should be no problem with this series. :) I think I can add my Tested-by: Tested-by: Laszlo Ersek <[email protected]> Cheers! Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
