1. Patchew
  2. EDK2
  3. View series

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

Ruiyu Ni posted 1 patch 6 years, 6 months ago
Failed in applying to current master (apply log)
.../Library/UefiBootManagerLib/BmLoadOption.c      | 45 ++++++++++++++--------
1 file changed, 30 insertions(+), 15 deletions(-)
[edk2] [PATCH] MdeModulePkg/Bds: Check variable name even OptionNumber is NULL
Posted by Ruiyu Ni 6 years, 6 months ago 
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;
 }
 
 /**
-- 
2.12.2.windows.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/Bds: Check variable name even OptionNumber is NULL
Posted by Laszlo Ersek 6 years, 6 months ago 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/Bds: Check variable name even OptionNumber is NULL
Posted by Ard Biesheuvel 6 years, 6 months ago 
On 10 October 2017 at 17:59, Laszlo Ersek <lersek@redhat.com> wrote:
> 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?
>

I will try this as soon as I get a chance (I have to switch back to an
older branch of the platform I am currently developing for)
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.