Re: [PATCH] MdeModulePkg/UefiBootManagerLib: don't ASSERT on 'BootNext' varname

[Laszlo Ersek <lersek@redhat.com>]

On 10/04/17 15:54, Ard Biesheuvel wrote:
> On 4 October 2017 at 14:49, Zeng, Star <star.zeng@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.

> 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,
Laszlo