On 10/10/17 10:58, Ruiyu Ni wrote:
> Current implementation skips to check whether the last four
> characters are digits when the OptionNumber is NULL.
> Even worse, it may incorrectly return FALSE when OptionNumber is
> NULL.
>
> The patch fixes it to always check the variable name even
> OptionNumber is NULL.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Laszlo Ersek <[email protected]>
> ---
> .../Library/UefiBootManagerLib/BmLoadOption.c | 45
> ++++++++++++++--------
> 1 file changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> index b0a35058d0..32918caf32 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> @@ -785,6 +785,8 @@ EfiBootManagerIsValidLoadOptionVariableName (
> UINTN VariableNameLen;
> UINTN Index;
> UINTN Uint;
> + EFI_BOOT_MANAGER_LOAD_OPTION_TYPE LocalOptionType;
> + UINT16 LocalOptionNumber;
>
> if (VariableName == NULL) {
> return FALSE;
> @@ -792,39 +794,52 @@ EfiBootManagerIsValidLoadOptionVariableName (
>
> VariableNameLen = StrLen (VariableName);
>
> + //
> + // Return FALSE when the variable name length is too small.
> + //
> if (VariableNameLen <= 4) {
> return FALSE;
> }
>
> - for (Index = 0; Index < ARRAY_SIZE (mBmLoadOptionName); Index++) {
> - if ((VariableNameLen - 4 == StrLen (mBmLoadOptionName[Index])) &&
> - (StrnCmp (VariableName, mBmLoadOptionName[Index], VariableNameLen -
> 4) == 0)
> + //
> + // Return FALSE when the variable name doesn't start with
> Driver/SysPrep/Boot/PlatformRecovery.
> + //
> + for (LocalOptionType = 0; LocalOptionType < ARRAY_SIZE
> (mBmLoadOptionName); LocalOptionType++) {
> + if ((VariableNameLen - 4 == StrLen (mBmLoadOptionName[LocalOptionType]))
> &&
> + (StrnCmp (VariableName, mBmLoadOptionName[LocalOptionType],
> VariableNameLen - 4) == 0)
> ) {
> break;
> }
> }
> + if (LocalOptionType == ARRAY_SIZE (mBmLoadOptionName)) {
> + return FALSE;
> + }
>
> - if (Index == ARRAY_SIZE (mBmLoadOptionName)) {
> + //
> + // Return FALSE when the last four characters are not hex digits.
> + //
> + LocalOptionNumber = 0;
> + for (Index = VariableNameLen - 4; Index < VariableNameLen; Index++) {
> + Uint = BmCharToUint (VariableName[Index]);
> + if (Uint == -1) {
> + break;
> + } else {
> + LocalOptionNumber = (UINT16) Uint + LocalOptionNumber * 0x10;
> + }
> + }
> + if (Index != VariableNameLen) {
> return FALSE;
> }
>
> if (OptionType != NULL) {
> - *OptionType = (EFI_BOOT_MANAGER_LOAD_OPTION_TYPE) Index;
> + *OptionType = LocalOptionType;
> }
>
> if (OptionNumber != NULL) {
> - *OptionNumber = 0;
> - for (Index = VariableNameLen - 4; Index < VariableNameLen; Index++) {
> - Uint = BmCharToUint (VariableName[Index]);
> - if (Uint == -1) {
> - break;
> - } else {
> - *OptionNumber = (UINT16) Uint + *OptionNumber * 0x10;
> - }
> - }
> + *OptionNumber = LocalOptionNumber;
> }
>
> - return (BOOLEAN) (Index == VariableNameLen);
> + return TRUE;
> }
>
> /**
>
I have some superficial comments:
- I think the subject should say
MdeModulePkg/Bds: Check variable name even *if* OptionNumber is NULL
- I'm not a huge fan of using enum types for loop counters; however, in
this case, I verified that there is LoadOptionTypeMax, and it equals
ARRAY_SIZE (mBmLoadOptionName). So it should be safe. If we want to be
more explicit about this, we could replace
CHAR16 *mBmLoadOptionName[] = {
with
CHAR16 *mBmLoadOptionName[LoadOptionTypeMax] = {
Anyway,
Reviewed-by: Laszlo Ersek <[email protected]>
Ard, can you test this patch in your original environment?
Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
