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

["Zeng, Star" <star.zeng@intel.com>]

I prefer to set the attributes to 0 for the MorLock deletion in the same patch.

It is fine to have empty MorLockInitAtEndOfDxe() in TcgMorLockDxe.c with your explanation.


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, October 11, 2017 1:22 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [PATCH] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM

On 10/10/17 15:54, Zeng, Star wrote:
> Could you help make the code consistent to call
> VariableServiceSetVariable() for the Attributes? One original for 
> MorLock is using real attributes, the new you added for Mor will use 0 
> attributes. How about to both use 0 attributes.

Sure, I can do that -- do you want me to set the attributes to 0 for the MorLock deletion in the same patch, or should I do it in a separate patch?

> Another question, why empty MorLockInitAtEndOfDxe() needs to be 
> implemented in TcgMorLockDxe.c and called in VariableDxe.c?

Because otherwise we would break the promise we make in
"PrivilegePolymorphic.h":

>   Polymorphic functions that are called from both the privileged driver (i.e.,
>   the DXE_SMM variable module) and the non-privileged drivers (i.e., one or
>   both of the DXE_RUNTIME variable modules).
>
>   Each of these functions has two implementations, appropriate for privileged
>   vs. non-privileged driver code.

If I don't implement MorLockInitAtEndOfDxe() for VariableRuntimeDxe, then we'll have a declaration with no definition in VariableRuntimeDxe, and only one definition in total.

The empty function call should be optimized out in RELEASE builds anyway (assuming LTO is used).

It's easy to remove the empty definition and the calls to it, but then the purpose of "PrivilegePolymorphic.h" becomes less clear.

For complete clarity, I would have had to introduce *two* new header
files:

- one for SecureBootHook(), MorLockInit() and
  SetVariableCheckHandlerMor() -- to be used by both SMM and non-SMM,

- and another header file for just MorLockInitAtEndOfDxe() -- to be used
  only by SMM. (This is necessary because the call is made from
  "VariableSmm.c" to "TcgMorLockSmm.c").

I thought that introducing two new header files would not be accepted, so I opted for one header file only. But then I felt that the empty
MorLockInitAtEndOfDxe() hook should be correctly implemented for VariableRuntimeDxe too.

Thanks,
Laszlo


>
>
> Thanks,
> Star
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, October 10, 2017 9:25 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen 
> <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH] 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.
>
> 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:
>     Repo:   https://github.com/lersek/edk2.git
>     Branch: del_and_lock_mor_without_smm
>
>  MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 24 
> ++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git 
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c 
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> index 7142e2da2073..1610c7aa0706 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> @@ -69,29 +69,53 @@ EFI_STATUS
>  MorLockInit (
>    VOID
>    )
>  {
>    //
>    // Always clear variable to report unsupported to OS.
>    // The reason is that the DXE version is not proper to provide *protection*.
>    // BIOS should use SMM version variable driver to provide such capability.
>    //
>    VariableServiceSetVariable (
>      MEMORY_OVERWRITE_REQUEST_CONTROL_LOCK_NAME,
>      &gEfiMemoryOverwriteRequestControlLockGuid,
>      EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
>      0,
>      NULL
>      );
>
>    //
>    // 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;
>  }
>
>  /**
>    Delayed initialization for MOR Control Lock at EndOfDxe.
>
>    This function performs any operations queued by MorLockInit().
>  **/
>