[Patch 1/1] MdeModulePkg/Library/VarCheckLib: Allow SetVariable from SMM

[Patch 1/1] MdeModulePkg/Library/VarCheckLib: Allow SetVariable from SMM

[Michael D Kinney]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3154

Update VarCheckLibSetVariableCheck() to allow locked variables
to be updated if the RequestSource is VarCheckFromTrusted even
if one or more variable check handlers return EFI_WRITE_PROTECTED.
RequestSource is only set to VarCheckFromTrusted if the request
is through the EFI_SMM_VARAIBLE_PROTOCOL.

Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdeModulePkg/Library/VarCheckLib/VarCheckLib.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c b/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
index 470d782444bf..9596d760e945 100644
--- a/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
+++ b/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
@@ -1,7 +1,7 @@
 /** @file
   Implementation functions and structures for var check services.
 
-Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -655,6 +655,13 @@ VarCheckLibSetVariableCheck (
                DataSize,
                Data
                );
+    if (Status == EFI_WRITE_PROTECTED && RequestSource == VarCheckFromTrusted) {
+      //
+      // If RequestSource is trusted, then allow variable to be set even if it
+      // is write protected.
+      //
+      continue;
+    }
     if (EFI_ERROR (Status)) {
       DEBUG ((EFI_D_INFO, "Variable Check handler fail %r - %g:%s\n", Status, VendorGuid, VariableName));
       return Status;
-- 
2.29.2.windows.2

Re: [EXTERNAL] [edk2-devel] [Patch 1/1] MdeModulePkg/Library/VarCheckLib: Allow SetVariable from SMM

[Bret Barkelew]

[-- Attachment #1: Type: text/plain, Size: 2672 bytes --]

Reviewed-by: Bret Barkelew <bret.barkelew@microsoft.com>

- Bret

From: Michael D Kinney via groups.io<mailto:michael.d.kinney=intel.com@groups.io>
Sent: Tuesday, January 5, 2021 7:51 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Hao A Wu<mailto:hao.a.wu@intel.com>; Liming Gao<mailto:gaoliming@byosoft.com.cn>
Subject: [EXTERNAL] [edk2-devel] [Patch 1/1] MdeModulePkg/Library/VarCheckLib: Allow SetVariable from SMM

REF: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3154&amp;data=04%7C01%7Cbret.barkelew%40microsoft.com%7Cdd8b9b71d99a4ae7056e08d8b1f64465%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637455018562833685%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=0UiR50Bobd3%2BQdK9lRD3ffTSoaHCNbPlY40sVeTIx3s%3D&amp;reserved=0

Update VarCheckLibSetVariableCheck() to allow locked variables
to be updated if the RequestSource is VarCheckFromTrusted even
if one or more variable check handlers return EFI_WRITE_PROTECTED.
RequestSource is only set to VarCheckFromTrusted if the request
is through the EFI_SMM_VARAIBLE_PROTOCOL.

Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdeModulePkg/Library/VarCheckLib/VarCheckLib.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c b/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
index 470d782444bf..9596d760e945 100644
--- a/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
+++ b/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
@@ -1,7 +1,7 @@
 /** @file
   Implementation functions and structures for var check services.

-Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent

 **/
@@ -655,6 +655,13 @@ VarCheckLibSetVariableCheck (
                DataSize,
                Data
                );
+    if (Status == EFI_WRITE_PROTECTED && RequestSource == VarCheckFromTrusted) {
+      //
+      // If RequestSource is trusted, then allow variable to be set even if it
+      // is write protected.
+      //
+      continue;
+    }
     if (EFI_ERROR (Status)) {
       DEBUG ((EFI_D_INFO, "Variable Check handler fail %r - %g:%s\n", Status, VendorGuid, VariableName));
       return Status;
--
2.29.2.windows.2







[-- Attachment #2: Type: text/html, Size: 5747 bytes --]

Re: [Patch 1/1] MdeModulePkg/Library/VarCheckLib: Allow SetVariable from SMM

[Wu, Hao A]

> -----Original Message-----
> From: Michael D Kinney <michael.d.kinney@intel.com>
> Sent: Wednesday, January 6, 2021 11:51 AM
> To: devel@edk2.groups.io
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>
> Subject: [Patch 1/1] MdeModulePkg/Library/VarCheckLib: Allow SetVariable
> from SMM
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3154
> 
> Update VarCheckLibSetVariableCheck() to allow locked variables to be
> updated if the RequestSource is VarCheckFromTrusted even if one or more
> variable check handlers return EFI_WRITE_PROTECTED.
> RequestSource is only set to VarCheckFromTrusted if the request is through
> the EFI_SMM_VARAIBLE_PROTOCOL.


Hello Mike,

Sorry for a question.

If a SetVar request is blocked by a registered VarCheck handler, I think it would better to change that handler to allow requests from SMM.
I am not sure if there is a VarCheck handler that has its own specific rule to return 'EFI_WRITE_PROTECTED' to block some SetVar requests.
Is there any special consideration (e.g. VarCheck handler not being able to get the source of the SetVar request) for not doing this way?

Thanks in advance.

Best Regards,
Hao Wu


> 
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  MdeModulePkg/Library/VarCheckLib/VarCheckLib.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
> b/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
> index 470d782444bf..9596d760e945 100644
> --- a/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
> +++ b/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Implementation functions and structures for var check services.
> 
> -Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -655,6 +655,13 @@ VarCheckLibSetVariableCheck (
>                 DataSize,
>                 Data
>                 );
> +    if (Status == EFI_WRITE_PROTECTED && RequestSource ==
> VarCheckFromTrusted) {
> +      //
> +      // If RequestSource is trusted, then allow variable to be set even if it
> +      // is write protected.
> +      //
> +      continue;
> +    }
>      if (EFI_ERROR (Status)) {
>        DEBUG ((EFI_D_INFO, "Variable Check handler fail %r - %g:%s\n", Status,
> VendorGuid, VariableName));
>        return Status;
> --
> 2.29.2.windows.2

Re: [Patch 1/1] MdeModulePkg/Library/VarCheckLib: Allow SetVariable from SMM

[Michael D Kinney]

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Tuesday, January 5, 2021 9:47 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Liming Gao <gaoliming@byosoft.com.cn>
> Subject: RE: [Patch 1/1] MdeModulePkg/Library/VarCheckLib: Allow SetVariable from SMM
> 
> > -----Original Message-----
> > From: Michael D Kinney <michael.d.kinney@intel.com>
> > Sent: Wednesday, January 6, 2021 11:51 AM
> > To: devel@edk2.groups.io
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>
> > Subject: [Patch 1/1] MdeModulePkg/Library/VarCheckLib: Allow SetVariable
> > from SMM
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3154
> >
> > Update VarCheckLibSetVariableCheck() to allow locked variables to be
> > updated if the RequestSource is VarCheckFromTrusted even if one or more
> > variable check handlers return EFI_WRITE_PROTECTED.
> > RequestSource is only set to VarCheckFromTrusted if the request is through
> > the EFI_SMM_VARAIBLE_PROTOCOL.
> 
> 
> Hello Mike,
> 
> Sorry for a question.
> 
> If a SetVar request is blocked by a registered VarCheck handler, I think it would better to change that handler to allow
> requests from SMM.
> I am not sure if there is a VarCheck handler that has its own specific rule to return 'EFI_WRITE_PROTECTED' to block some
> SetVar requests.
> Is there any special consideration (e.g. VarCheck handler not being able to get the source of the SetVar request) for not
> doing this way?

The VarCheck handlers do not have the context information.

The VarCheckHandlers function type is the same as EFI_SET_VARIABLE.

    typedef EFI_SET_VARIABLE VAR_CHECK_SET_VARIABLE_CHECK_HANDLER;

EFI_SET_VARIABLE defines EFI_WRITE_PROTECTED return status for
read-only condition.  So using EFI_WRITE_PROTECTED to block a write
for other reasons would not be conformant.

> 
> Thanks in advance.
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> > ---
> >  MdeModulePkg/Library/VarCheckLib/VarCheckLib.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
> > b/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
> > index 470d782444bf..9596d760e945 100644
> > --- a/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
> > +++ b/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >    Implementation functions and structures for var check services.
> >
> > -Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
> > +Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  **/
> > @@ -655,6 +655,13 @@ VarCheckLibSetVariableCheck (
> >                 DataSize,
> >                 Data
> >                 );
> > +    if (Status == EFI_WRITE_PROTECTED && RequestSource ==
> > VarCheckFromTrusted) {
> > +      //
> > +      // If RequestSource is trusted, then allow variable to be set even if it
> > +      // is write protected.
> > +      //
> > +      continue;
> > +    }
> >      if (EFI_ERROR (Status)) {
> >        DEBUG ((EFI_D_INFO, "Variable Check handler fail %r - %g:%s\n", Status,
> > VendorGuid, VariableName));
> >        return Status;
> > --
> > 2.29.2.windows.2

Re: [Patch 1/1] MdeModulePkg/Library/VarCheckLib: Allow SetVariable from SMM

[Wu, Hao A]

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Thursday, January 7, 2021 12:53 AM
> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Liming Gao
> <gaoliming@byosoft.com.cn>
> Subject: RE: [Patch 1/1] MdeModulePkg/Library/VarCheckLib: Allow
> SetVariable from SMM
> 
> > -----Original Message-----
> > From: Wu, Hao A <hao.a.wu@intel.com>
> > Sent: Tuesday, January 5, 2021 9:47 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > devel@edk2.groups.io
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Liming Gao
> > <gaoliming@byosoft.com.cn>
> > Subject: RE: [Patch 1/1] MdeModulePkg/Library/VarCheckLib: Allow
> > SetVariable from SMM
> >
> > > -----Original Message-----
> > > From: Michael D Kinney <michael.d.kinney@intel.com>
> > > Sent: Wednesday, January 6, 2021 11:51 AM
> > > To: devel@edk2.groups.io
> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>
> > > Subject: [Patch 1/1] MdeModulePkg/Library/VarCheckLib: Allow
> > > SetVariable from SMM
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3154
> > >
> > > Update VarCheckLibSetVariableCheck() to allow locked variables to be
> > > updated if the RequestSource is VarCheckFromTrusted even if one or
> > > more variable check handlers return EFI_WRITE_PROTECTED.
> > > RequestSource is only set to VarCheckFromTrusted if the request is
> > > through the EFI_SMM_VARAIBLE_PROTOCOL.
> >
> >
> > Hello Mike,
> >
> > Sorry for a question.
> >
> > If a SetVar request is blocked by a registered VarCheck handler, I
> > think it would better to change that handler to allow requests from SMM.
> > I am not sure if there is a VarCheck handler that has its own specific
> > rule to return 'EFI_WRITE_PROTECTED' to block some SetVar requests.
> > Is there any special consideration (e.g. VarCheck handler not being
> > able to get the source of the SetVar request) for not doing this way?
> 
> The VarCheck handlers do not have the context information.
> 
> The VarCheckHandlers function type is the same as EFI_SET_VARIABLE.
> 
>     typedef EFI_SET_VARIABLE VAR_CHECK_SET_VARIABLE_CHECK_HANDLER;
> 
> EFI_SET_VARIABLE defines EFI_WRITE_PROTECTED return status for read-
> only condition.  So using EFI_WRITE_PROTECTED to block a write for other
> reasons would not be conformant.


Thanks.
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> 
> >
> > Thanks in advance.
> >
> > Best Regards,
> > Hao Wu
> >
> >
> > >
> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> > > ---
> > >  MdeModulePkg/Library/VarCheckLib/VarCheckLib.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
> > > b/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
> > > index 470d782444bf..9596d760e945 100644
> > > --- a/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
> > > +++ b/MdeModulePkg/Library/VarCheckLib/VarCheckLib.c
> > > @@ -1,7 +1,7 @@
> > >  /** @file
> > >    Implementation functions and structures for var check services.
> > >
> > > -Copyright (c) 2015 - 2016, Intel Corporation. All rights
> > > reserved.<BR>
> > > +Copyright (c) 2015 - 2021, Intel Corporation. All rights
> > > +reserved.<BR>
> > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >  **/
> > > @@ -655,6 +655,13 @@ VarCheckLibSetVariableCheck (
> > >                 DataSize,
> > >                 Data
> > >                 );
> > > +    if (Status == EFI_WRITE_PROTECTED && RequestSource ==
> > > VarCheckFromTrusted) {
> > > +      //
> > > +      // If RequestSource is trusted, then allow variable to be set even if it
> > > +      // is write protected.
> > > +      //
> > > +      continue;
> > > +    }
> > >      if (EFI_ERROR (Status)) {
> > >        DEBUG ((EFI_D_INFO, "Variable Check handler fail %r -
> > > %g:%s\n", Status, VendorGuid, VariableName));
> > >        return Status;
> > > --
> > > 2.29.2.windows.2