Re: [PATCH] MdeModulePkg VariableSmm: Check InfoSize correctly

["Yao, Jiewen" <jiewen.yao@intel.com>]

Reviewed-by: jiewen.yao@intel.com

> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, December 9, 2016 4:48 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH] MdeModulePkg VariableSmm: Check InfoSize correctly
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=290
> 
> Current SmmVariableGetStatistics() in VariableSmm.c is always
> checking input InfoSize against the first variable info,
> it is incorrect.
> 
> For instance, there are three variables.
> BootOrder
> Boot0000
> Boot0001
> 
> If the input InfoEntry is holding the second variable info (Boot0000)
> and InfoSize is sizeof (VARIABLE_INFO_ENTRY) + StrSize (L"Boot0000"),
> current code will return EFI_BUFFER_TOO_SMALL, but it should return
> the third variable info (Boot0001).
> 
> This patch is to refine the code logic.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  .../Universal/Variable/RuntimeDxe/VariableSmm.c    | 25
> +++++++++++++++-------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> index eafb53322e8c..85158d8b46ae 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> @@ -349,9 +349,10 @@ SmmVariableGetStatistics (
>    )
>  {
>    VARIABLE_INFO_ENTRY                                  *VariableInfo;
> -  UINTN                                                NameLength;
> +  UINTN                                                NameSize;
>    UINTN
> StatisticsInfoSize;
>    CHAR16                                               *InfoName;
> +  UINTN
> InfoNameMaxSize;
>    EFI_GUID                                             VendorGuid;
> 
>    if (InfoEntry == NULL) {
> @@ -363,12 +364,13 @@ SmmVariableGetStatistics (
>      return EFI_UNSUPPORTED;
>    }
> 
> -  StatisticsInfoSize = sizeof (VARIABLE_INFO_ENTRY) + StrSize
> (VariableInfo->Name);
> +  StatisticsInfoSize = sizeof (VARIABLE_INFO_ENTRY);
>    if (*InfoSize < StatisticsInfoSize) {
>      *InfoSize = StatisticsInfoSize;
>      return EFI_BUFFER_TOO_SMALL;
>    }
>    InfoName = (CHAR16 *)(InfoEntry + 1);
> +  InfoNameMaxSize = (*InfoSize - sizeof (VARIABLE_INFO_ENTRY));
> 
>    CopyGuid (&VendorGuid, &InfoEntry->VendorGuid);
> 
> @@ -376,8 +378,14 @@ SmmVariableGetStatistics (
>      //
>      // Return the first variable info
>      //
> +    NameSize = StrSize (VariableInfo->Name);
> +    StatisticsInfoSize = sizeof (VARIABLE_INFO_ENTRY) + NameSize;
> +    if (*InfoSize < StatisticsInfoSize) {
> +      *InfoSize = StatisticsInfoSize;
> +      return EFI_BUFFER_TOO_SMALL;
> +    }
>      CopyMem (InfoEntry, VariableInfo, sizeof (VARIABLE_INFO_ENTRY));
> -    CopyMem (InfoName, VariableInfo->Name, StrSize (VariableInfo->Name));
> +    CopyMem (InfoName, VariableInfo->Name, NameSize);
>      *InfoSize = StatisticsInfoSize;
>      return EFI_SUCCESS;
>    }
> @@ -387,9 +395,9 @@ SmmVariableGetStatistics (
>    //
>    while (VariableInfo != NULL) {
>      if (CompareGuid (&VariableInfo->VendorGuid, &VendorGuid)) {
> -      NameLength = StrSize (VariableInfo->Name);
> -      if (NameLength == StrSize (InfoName)) {
> -        if (CompareMem (VariableInfo->Name, InfoName, NameLength) == 0)
> {
> +      NameSize = StrSize (VariableInfo->Name);
> +      if (NameSize <= InfoNameMaxSize) {
> +        if (CompareMem (VariableInfo->Name, InfoName, NameSize) == 0) {
>            //
>            // Find the match one
>            //
> @@ -409,14 +417,15 @@ SmmVariableGetStatistics (
>    //
>    // Output the new variable info
>    //
> -  StatisticsInfoSize = sizeof (VARIABLE_INFO_ENTRY) + StrSize
> (VariableInfo->Name);
> +  NameSize = StrSize (VariableInfo->Name);
> +  StatisticsInfoSize = sizeof (VARIABLE_INFO_ENTRY) + NameSize;
>    if (*InfoSize < StatisticsInfoSize) {
>      *InfoSize = StatisticsInfoSize;
>      return EFI_BUFFER_TOO_SMALL;
>    }
> 
>    CopyMem (InfoEntry, VariableInfo, sizeof (VARIABLE_INFO_ENTRY));
> -  CopyMem (InfoName, VariableInfo->Name, StrSize (VariableInfo->Name));
> +  CopyMem (InfoName, VariableInfo->Name, NameSize);
>    *InfoSize = StatisticsInfoSize;
> 
>    return EFI_SUCCESS;
> --
> 2.7.0.windows.1