Re: [PATCH] MdeModulePkg/Bds: Check variable name even OptionNumber is NULL

[Laszlo Ersek <lersek@redhat.com>]

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 <ruiyu.ni@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  .../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 <lersek@redhat.com>

Ard, can you test this patch in your original environment?

Thanks!
Laszlo