From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mx.groups.io with SMTP id smtpd.web12.836.1571274060336198728 for ; Wed, 16 Oct 2019 18:01:00 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Oct 2019 18:00:59 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.67,305,1566889200"; d="scan'208";a="199123945" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga003.jf.intel.com with ESMTP; 16 Oct 2019 18:00:59 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 16 Oct 2019 18:00:58 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.166]) by SHSMSX152.ccr.corp.intel.com ([10.239.6.52]) with mapi id 14.03.0439.000; Thu, 17 Oct 2019 09:00:54 +0800 From: "Wu, Hao A" To: "Kubacki, Michael A" , "devel@edk2.groups.io" CC: "Bi, Dandan" , Ard Biesheuvel , "Dong, Eric" , Laszlo Ersek , "Gao, Liming" , "Kinney, Michael D" , "Ni, Ray" , "Wang, Jian J" , "Yao, Jiewen" Subject: Re: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add local auth status in VariableParsing Thread-Topic: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add local auth status in VariableParsing Thread-Index: AQHVdZ6zvg1BoZXtVUOkq6ImrvN3bKdCQ9cggAZ9cACAFCFiYIAAK/OAgAEQjOA= Date: Thu, 17 Oct 2019 01:00:54 +0000 Message-ID: References: <20190928014717.31372-1-michael.a.kubacki@intel.com> <20190928014717.31372-5-michael.a.kubacki@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Kubacki, Michael A > Sent: Thursday, October 17, 2019 12:37 AM > To: Wu, Hao A; devel@edk2.groups.io > Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Liming; K= inney, > Michael D; Ni, Ray; Wang, Jian J; Yao, Jiewen > Subject: RE: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add loca= l > auth status in VariableParsing >=20 > The InitVariableParsing () function has already been removed entirely in= V3+ > to > prevent issues like this. I strongly believe this is the right direction= . It is only a > matter of time before someone else modifies the global and forgets to ca= ll > InitVariableParsing (). >=20 > Furthermore, VariablePei has already added a parameter to most of the > equivalent Thanks for the clarification. I agree with your point, will reply to the corresponding patch in the late= st series. Best Regards, Hao Wu > functions implemented in that driver so there's some potential to furthe= r > consolidate the variable parsing implementation in the future. >=20 > Thanks, > Michael >=20 > > -----Original Message----- > > From: Wu, Hao A > > Sent: Wednesday, October 16, 2019 12:55 AM > > To: Kubacki, Michael A ; > > devel@edk2.groups.io > > Cc: Bi, Dandan ; Ard Biesheuvel > > ; Dong, Eric ; Laszlo > Ersek > > ; Gao, Liming ; Kinney, > Michael > > D ; Ni, Ray ; Wang, Jian= J > > ; Yao, Jiewen > > Subject: RE: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add > local > > auth status in VariableParsing > > > > > -----Original Message----- > > > From: Kubacki, Michael A > > > Sent: Friday, October 04, 2019 2:35 AM > > > To: Wu, Hao A; devel@edk2.groups.io > > > Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Limin= g; > > > Kinney, Michael D; Ni, Ray; Wang, Jian J; Yao, Jiewen > > > Subject: RE: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add > > > local auth status in VariableParsing > > > > > > I will make the following changes in V3: > > > > > > > InitVariableParsing() seems an internal function, the 'EFIAPI' > > > > keyword can > > > be > > > > dropped. Please help to update the function definition in .C file = as well. > > > > > > I will remove the EFIAPI keyword. > > > > > > > > diff --git > a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > > > index 1a57d7e1ba..53d797152c 100644 > > > > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > > > @@ -3326,6 +3326,9 @@ InitNonVolatileVariableStore ( > > > > > mVariableModuleGlobal->MaxVariableSize =3D PcdGet32 > > > > > (PcdMaxVariableSize); > > > > > mVariableModuleGlobal->MaxAuthVariableSize =3D ((PcdGet32 > > > > > (PcdMaxAuthVariableSize) !=3D 0) ? PcdGet32 > (PcdMaxAuthVariableSize) > > : > > > > > mVariableModuleGlobal->MaxVariableSize); > > > > > > > > > > + Status =3D InitVariableParsing (mVariableModuleGlobal- > > > > > >VariableGlobal.AuthFormat); > > > > > + ASSERT_EFI_ERROR (Status); > > > > > + > > > > > > > > > > > > After the above initialization, mVariableModuleGlobal- > > > > >VariableGlobal.AuthFormat > > > > will be changed temporarily within > > > > ConvertNormalVarStorageToAuthVarStorage() if normal HOB variable > > > > store will be converted to the auth format: > > > > > > > > VOID * > > > > ConvertNormalVarStorageToAuthVarStorage ( > > > > VARIABLE_STORE_HEADER *NormalVarStorage > > > > ) > > > > { > > > > ... > > > > // > > > > // Set AuthFormat as FALSE for normal variable storage > > > > // > > > > mVariableModuleGlobal->VariableGlobal.AuthFormat =3D FALSE; > > > > ... > > > > // > > > > // Restore AuthFormat > > > > // > > > > mVariableModuleGlobal->VariableGlobal.AuthFormat =3D TRUE; > > > > return AuthVarStorage; > > > > } > > > > > > > > > > > > I think there will be issues in such converting, since I found tha= t > > > > at least > > > > GetVariableHeaderSize() and NameSizeOfVariable() get called during > > > > the execution of ConvertNormalVarStorageToAuthVarStorage(). And > > they > > > > are checking 'mAuthFormat' rather than 'mVariableModuleGlobal- > > > > >VariableGlobal.AuthFormat'. > > > > > > > > > > > > > > You're right that will be a problem. I missed this temporary change = in > > > the value. > > > I'm going to have all the functions dependent on authentication stat= us > > > in VariableParsing.c take it as a parameter and let the respective > > > drivers linked against it maintain their own single copy of the > authentication > > state. > > > > > > I am really sorry for not raising this question until I saw the latest= patch > > series: > > > > Is it possible to call the InitVariableParsing() function (maybe a ren= ame for > > the function for better understanding) for the temporary changes for > > 'mVariableModuleGlobal->VariableGlobal.AuthFormat' in function > > ConvertNormalVarStorageToAuthVarStorage()? > > > > In my opinion, doing so can avoid changing many function interfaces. > > > > Best Regards, > > Hao Wu > > > > > > > > > > > > // > > > > > // Parse non-volatile variable data and get last variable off= set. > > > > > // > > > > > @@ -3756,18 +3759,13 @@ VariableCommonInitialize ( > > > > > > > > > > // > > > > > // mVariableModuleGlobal->VariableGlobal.AuthFormat > > > > > - // has been initialized in InitNonVolatileVariableStore(). > > > > > + // is initialized in InitNonVolatileVariableStore(). > > > > > // > > > > > if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > > > DEBUG ((EFI_D_INFO, "Variable driver will work with auth > > > > > variable format!\n")); > > > > > - // > > > > > - // Set AuthSupport to FALSE first, VariableWriteServiceInit= ialize() > will > > > > > initialize it. > > > > > - // > > > > > - mVariableModuleGlobal->VariableGlobal.AuthSupport =3D FALSE= ; > > > > > VariableGuid =3D &gEfiAuthenticatedVariableGuid; > > > > > } else { > > > > > DEBUG ((EFI_D_INFO, "Variable driver will work without auth > > > > > variable support!\n")); > > > > > - mVariableModuleGlobal->VariableGlobal.AuthSupport =3D FALSE= ; > > > > > > > > > > > > Not sure why the above changes belong to this patch. > > > > Could you help to double confirm? > > > > > > This was used during testing and is not needed. I will remove it. > > > > > > Thanks, > > > Michael > > > > > > > -----Original Message----- > > > > From: Wu, Hao A > > > > Sent: Thursday, October 3, 2019 1:04 AM > > > > To: devel@edk2.groups.io; Kubacki, Michael A > > > > > > > > Cc: Bi, Dandan ; Ard Biesheuvel > > > > ; Dong, Eric ; > > > > Laszlo > > > Ersek > > > > ; Gao, Liming ; Kinney, > > > Michael > > > > D ; Ni, Ray ; Wang, > > > > Jian J ; Yao, Jiewen > > > > Subject: RE: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: > Add > > > local > > > > auth status in VariableParsing > > > > > > > > Inline comments below: > > > > > > > > > > > > > -----Original Message----- > > > > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On > Behalf > > > Of > > > > > Kubacki, Michael A > > > > > Sent: Saturday, September 28, 2019 9:47 AM > > > > > To: devel@edk2.groups.io > > > > > Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, > > > > > Liming; Kinney, Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao= , > > > > > Jiewen > > > > > Subject: [edk2-devel] [PATCH V2 4/9] MdeModulePkg/Variable: Add > > > > > local auth status in VariableParsing > > > > > > > > > > The file VariableParsing.c provides generic functionality relate= d > > > > > to parsing variable related structures and information. In order > > > > > to calculate offsets for certain operations, the functions must > > > > > know if authenticated variables are enabled as this increases th= e > > > > > size of variable headers. > > > > > > > > > > This change removes linking against a global variable in an > > > > > external file in favor of a statically scoped variable in > > > > > VariableParsing.c Because this file is unaware of how the > > > > > authenticated variable status is determined, the variable is set > > > > > through a function interface invoked during variable driver > initialization. > > > > > > > > > > Cc: Dandan Bi > > > > > Cc: Ard Biesheuvel > > > > > Cc: Eric Dong > > > > > Cc: Laszlo Ersek > > > > > Cc: Liming Gao > > > > > Cc: Michael D Kinney > > > > > Cc: Ray Ni > > > > > Cc: Jian J Wang > > > > > Cc: Hao A Wu > > > > > Cc: Jiewen Yao > > > > > Signed-off-by: Michael Kubacki > > > > > --- > > > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h | > > 14 > > > > > +++++++++ > > > > > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | = 10 > > > +++--- > > > > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c | > > 33 > > > > > ++++++++++++++++---- > > > > > 3 files changed, 45 insertions(+), 12 deletions(-) > > > > > > > > > > diff --git > > > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h > > > > > index 6f2000f3ee..3eba590634 100644 > > > > > --- > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h > > > > > +++ > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h > > > > > @@ -308,4 +308,18 @@ UpdateVariableInfo ( > > > > > IN OUT VARIABLE_INFO_ENTRY **VariableInfo > > > > > ); > > > > > > > > > > +/** > > > > > + Initializes context needed for variable parsing functions. > > > > > + > > > > > + @param[in] AuthFormat If true then indicates > authenticated > > > > > variables are supported > > > > > + > > > > > + @retval EFI_SUCCESS Initialized successfully > > > > > + @retval Others An error occurred during= initialization > > > > > +**/ > > > > > +EFI_STATUS > > > > > +EFIAPI > > > > > +InitVariableParsing ( > > > > > > > > > > > > InitVariableParsing() seems an internal function, the 'EFIAPI' > > > > keyword can > > > be > > > > dropped. Please help to update the function definition in .C file = as well. > > > > > > > > > > > > > + IN BOOLEAN AuthFormat > > > > > + ); > > > > > + > > > > > #endif > > > > > diff --git > a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > > > index 1a57d7e1ba..53d797152c 100644 > > > > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c > > > > > @@ -3326,6 +3326,9 @@ InitNonVolatileVariableStore ( > > > > > mVariableModuleGlobal->MaxVariableSize =3D PcdGet32 > > > > > (PcdMaxVariableSize); > > > > > mVariableModuleGlobal->MaxAuthVariableSize =3D ((PcdGet32 > > > > > (PcdMaxAuthVariableSize) !=3D 0) ? PcdGet32 > (PcdMaxAuthVariableSize) > > : > > > > > mVariableModuleGlobal->MaxVariableSize); > > > > > > > > > > + Status =3D InitVariableParsing (mVariableModuleGlobal- > > > > > >VariableGlobal.AuthFormat); > > > > > + ASSERT_EFI_ERROR (Status); > > > > > + > > > > > > > > > > > > After the above initialization, mVariableModuleGlobal- > > > > >VariableGlobal.AuthFormat > > > > will be changed temporarily within > > > > ConvertNormalVarStorageToAuthVarStorage() if normal HOB variable > > > > store will be converted to the auth format: > > > > > > > > VOID * > > > > ConvertNormalVarStorageToAuthVarStorage ( > > > > VARIABLE_STORE_HEADER *NormalVarStorage > > > > ) > > > > { > > > > ... > > > > // > > > > // Set AuthFormat as FALSE for normal variable storage > > > > // > > > > mVariableModuleGlobal->VariableGlobal.AuthFormat =3D FALSE; > > > > ... > > > > // > > > > // Restore AuthFormat > > > > // > > > > mVariableModuleGlobal->VariableGlobal.AuthFormat =3D TRUE; > > > > return AuthVarStorage; > > > > } > > > > > > > > > > > > I think there will be issues in such converting, since I found tha= t > > > > at least > > > > GetVariableHeaderSize() and NameSizeOfVariable() get called during > > > > the execution of ConvertNormalVarStorageToAuthVarStorage(). And > > they > > > > are checking 'mAuthFormat' rather than 'mVariableModuleGlobal- > > > > >VariableGlobal.AuthFormat'. > > > > > > > > > > > > > // > > > > > // Parse non-volatile variable data and get last variable off= set. > > > > > // > > > > > @@ -3756,18 +3759,13 @@ VariableCommonInitialize ( > > > > > > > > > > // > > > > > // mVariableModuleGlobal->VariableGlobal.AuthFormat > > > > > - // has been initialized in InitNonVolatileVariableStore(). > > > > > + // is initialized in InitNonVolatileVariableStore(). > > > > > // > > > > > if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > > > DEBUG ((EFI_D_INFO, "Variable driver will work with auth > > > > > variable format!\n")); > > > > > - // > > > > > - // Set AuthSupport to FALSE first, VariableWriteServiceInit= ialize() > will > > > > > initialize it. > > > > > - // > > > > > - mVariableModuleGlobal->VariableGlobal.AuthSupport =3D FALSE= ; > > > > > VariableGuid =3D &gEfiAuthenticatedVariableGuid; > > > > > } else { > > > > > DEBUG ((EFI_D_INFO, "Variable driver will work without auth > > > > > variable support!\n")); > > > > > - mVariableModuleGlobal->VariableGlobal.AuthSupport =3D FALSE= ; > > > > > > > > > > > > Not sure why the above changes belong to this patch. > > > > Could you help to double confirm? > > > > > > > > Best Regards, > > > > Hao Wu > > > > > > > > > > > > > VariableGuid =3D &gEfiVariableGuid; > > > > > } > > > > > > > > > > diff --git > > > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c > > > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c > > > > > index 394195342d..0a47f6d10d 100644 > > > > > --- > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c > > > > > +++ > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c > > > > > @@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > > > > > > > > #include "VariableParsing.h" > > > > > > > > > > +STATIC BOOLEAN mAuthFormat; > > > > > + > > > > > /** > > > > > > > > > > This code checks if variable header is valid or not. > > > > > @@ -88,7 +90,7 @@ GetVariableHeaderSize ( { > > > > > UINTN Value; > > > > > > > > > > - if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > > > + if (mAuthFormat) { > > > > > Value =3D sizeof (AUTHENTICATED_VARIABLE_HEADER); > > > > > } else { > > > > > Value =3D sizeof (VARIABLE_HEADER); @@ -114,7 +116,7 @@ > > > > > NameSizeOfVariable ( > > > > > AUTHENTICATED_VARIABLE_HEADER *AuthVariable; > > > > > > > > > > AuthVariable =3D (AUTHENTICATED_VARIABLE_HEADER *) Variable; > > > > > - if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > > > + if (mAuthFormat) { > > > > > if (AuthVariable->State =3D=3D (UINT8) (-1) || > > > > > AuthVariable->DataSize =3D=3D (UINT32) (-1) || > > > > > AuthVariable->NameSize =3D=3D (UINT32) (-1) || @@ -149,7 > > > > > +151,7 @@ SetNameSizeOfVariable ( > > > > > AUTHENTICATED_VARIABLE_HEADER *AuthVariable; > > > > > > > > > > AuthVariable =3D (AUTHENTICATED_VARIABLE_HEADER *) Variable; > > > > > - if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > > > + if (mAuthFormat) { > > > > > AuthVariable->NameSize =3D (UINT32) NameSize; > > > > > } else { > > > > > Variable->NameSize =3D (UINT32) NameSize; @@ -173,7 +175,7 = @@ > > > > > DataSizeOfVariable ( > > > > > AUTHENTICATED_VARIABLE_HEADER *AuthVariable; > > > > > > > > > > AuthVariable =3D (AUTHENTICATED_VARIABLE_HEADER *) Variable; > > > > > - if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > > > + if (mAuthFormat) { > > > > > if (AuthVariable->State =3D=3D (UINT8) (-1) || > > > > > AuthVariable->DataSize =3D=3D (UINT32) (-1) || > > > > > AuthVariable->NameSize =3D=3D (UINT32) (-1) || @@ -208,7 > > > > > +210,7 @@ SetDataSizeOfVariable ( > > > > > AUTHENTICATED_VARIABLE_HEADER *AuthVariable; > > > > > > > > > > AuthVariable =3D (AUTHENTICATED_VARIABLE_HEADER *) Variable; > > > > > - if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > > > + if (mAuthFormat) { > > > > > AuthVariable->DataSize =3D (UINT32) DataSize; > > > > > } else { > > > > > Variable->DataSize =3D (UINT32) DataSize; @@ -248,7 +250,7 = @@ > > > > > GetVendorGuidPtr ( > > > > > AUTHENTICATED_VARIABLE_HEADER *AuthVariable; > > > > > > > > > > AuthVariable =3D (AUTHENTICATED_VARIABLE_HEADER *) Variable; > > > > > - if (mVariableModuleGlobal->VariableGlobal.AuthFormat) { > > > > > + if (mAuthFormat) { > > > > > return &AuthVariable->VendorGuid; > > > > > } else { > > > > > return &Variable->VendorGuid; @@ -746,3 +748,22 @@ > > > > > UpdateVariableInfo ( > > > > > } > > > > > } > > > > > } > > > > > + > > > > > +/** > > > > > + Initializes context needed for variable parsing functions. > > > > > + > > > > > + @param[in] AuthFormat If true then indicates > authenticated > > > > > variables are supported > > > > > + > > > > > + @retval EFI_SUCCESS Initialized successfully > > > > > + @retval Others An error occurred during= initialization > > > > > +**/ > > > > > +EFI_STATUS > > > > > +EFIAPI > > > > > +InitVariableParsing ( > > > > > + IN BOOLEAN AuthFormat > > > > > + ) > > > > > +{ > > > > > + mAuthFormat =3D AuthFormat; > > > > > + > > > > > + return EFI_SUCCESS; > > > > > +} > > > > > -- > > > > > 2.16.2.windows.1 > > > > > > > > > > > > > > >=20 > > > > > > > > > >=20