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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 4 October 2017 at 15:40, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.
>

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.