1. Patchew
  2. EDK2
  3. View series

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

Laszlo Ersek posted 1 patch 7 years, 2 months ago
Failed in applying to current master (apply log)
MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c | 30 ++++++++++++++++++--
1 file changed, 27 insertions(+), 3 deletions(-)
[edk2] [PATCH v2] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM
Posted by Laszlo Ersek 7 years, 2 months ago 
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;
 }
 
-- 
2.14.1.3.gb7cf6e02401b

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM
Posted by Zeng, Star 7 years, 2 months ago 
Reviewed-by: Star Zeng <star.zeng@intel.com>

-----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;
 }
 
-- 
2.14.1.3.gb7cf6e02401b

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] MdeModulePkg/Variable/RuntimeDxe: delete & lock MOR in the absence of SMM
Posted by Laszlo Ersek 7 years, 2 months ago 
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;
>  }
>  
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.