On 4 October 2017 at 15:40, Laszlo Ersek <ler...@redhat.com> wrote: > On 10/04/17 15:54, Ard Biesheuvel wrote: >> On 4 October 2017 at 14:49, Zeng, Star <star.z...@intel.com> wrote: >>> Creating Boot000@ with gEfiGlobalVariableGuid can not succeed as it will be >>> rejected by MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf that >>> will check the VariableName against UEFI spec “Table 13. Global Variables” >>> if the VendorGuid is gEfiGlobalVariableGuid. >>> >>> >>> >>> I would suspect there is a bug at other place if the code ends up calling >>> this function(EfiBootManagerIsValidLoadOptionVariableName) on L"BootNext". >>> >> >> That still does not mean you should ASSERT() here. The state of the >> variable store != the internals of the code, and so it should be >> considered external input to some extent. > > At least under some circumstances, I disagree with this. The assumption > that the variable store can be written only by privileged firmware > routines is core to Secure Boot, for example. >
That is true. But the firmware that wrote to the varstore may be a different version from the one reading it. >> ASSERTs are meant to catch >> programming errors, not errors in the varstore image. > > I agree. > > However, as a corollary to the above, if said "privileged routines" are > supposed to catch all invalid inputs passed to gRT->SetVariable(), then > the rest of the firmware is right to assume that the contents of the > variable store are valid. If it is found invalid at some point, then it > is indeed due to a programming error (somewhere in the > gRT->SetVariable() machinery, that is), so the ASSERT() is justified. > > Another example in support of this argument is the Fault Tolerant Write > machinery -- the firmware tries very hard to recover from power loss > during a varstore update. If, on reboot, the error proves > non-recoverable (i.e. we cannot even roll back to a previous pristine > state), then that can be considered a bug (or design error) in FTW. > > > That said, I agree with the patch. BmCharToUint() explicitly documents > "conversion failed" as a return condition, and both functions that call > BmCharToUint(), namely EfiBootManagerIsValidLoadOptionVariableName() and > BmIsKeyOptionVariable(), check for that return condition. > Thanks, Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
