Re: [PATCH v2] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM

[Laszlo Ersek <lersek@redhat.com>]

On 10/25/17 03:33, Zeng, Star wrote:
> Reviewed-by: Star Zeng <star.zeng@intel.com>

Commit 704b71d7e11f.

Thank you, Star!
Laszlo

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Tuesday, October 24, 2017 11:38 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH v2] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM
> 
> VariableRuntimeDxe deletes and locks the MorLock variable in
> MorLockInit(), with the argument that any protection provided by MorLock
> can be circumvented if MorLock can be overwritten by unprivileged code
> (i.e., outside of SMM).
> 
> Extend the argument and the logic to the MOR variable, which is supposed
> to be protected by MorLock. Pass Attributes=0 when deleting MorLock and
> MOR both.
> 
> This change was suggested by Star; it is inspired by earlier VariableSmm
> commit fda8f631edbb ("MdeModulePkg/Variable/RuntimeDxe: delete and lock
> OS-created MOR variable", 2017-10-03).
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Suggested-by: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     v2:
>     - Use Attributes=0 for deleting MorLock too [Star]
>     - Branch: del_and_lock_mor_without_smm_v2
>     
>     v1:
>     - Branch: del_and_lock_mor_without_smm
>     
>     Repo: https://github.com/lersek/edk2.git
> 
>  MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 30 ++++++++++++++++++--
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> index 7142e2da2073..fb4e13ab25a7 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> @@ -78,15 +78,39 @@ MorLockInit (
>    VariableServiceSetVariable (
>      MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME,
>      &gEfiMemoryOverwriteRequestControlLockGuid,
> -    EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
> -    0,
> -    NULL
> +    0,                                          // Attributes
> +    0,                                          // DataSize
> +    NULL                                        // Data
>      );
>  
>    //
>    // Need set this variable to be read-only to prevent other module set it.
>    //
>    VariableLockRequestToLock (&mVariableLock, MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME, &gEfiMemoryOverwriteRequestControlLockGuid);
> +
> +  //
> +  // The MOR variable can effectively improve platform security only when the
> +  // MorLock variable protects the MOR variable. In turn MorLock cannot be made
> +  // secure without SMM support in the platform firmware (see above).
> +  //
> +  // Thus, delete the MOR variable, should it exist for any reason (some OSes
> +  // are known to create MOR unintentionally, in an attempt to set it), then
> +  // also lock the MOR variable, in order to prevent other modules from
> +  // creating it.
> +  //
> +  VariableServiceSetVariable (
> +    MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
> +    &gEfiMemoryOverwriteControlDataGuid,
> +    0,                                      // Attributes
> +    0,                                      // DataSize
> +    NULL                                    // Data
> +    );
> +  VariableLockRequestToLock (
> +    &mVariableLock,
> +    MEMORY_OVERWRITE_REQUEST_VARIABLE_NAME,
> +    &gEfiMemoryOverwriteControlDataGuid
> +    );
> +
>    return EFI_SUCCESS;
>  }
>  
>