回复: [edk2-devel] [PATCH] MdeModulePkg/HiiDatabaseDxe: Add Support for authenticated variable
gaoliming
gaoliming at byosoft.com.cn
Fri Jan 21 05:15:59 UTC 2022
Di:
> -----邮件原件-----
> 发件人: Zhang, Di <di.zhang at intel.com>
> 发送时间: 2022年1月20日 14:05
> 收件人: Gao, Liming <gaoliming at byosoft.com.cn>; Chen, Lin Z
> <lin.z.chen at intel.com>; devel at edk2.groups.io; Huang, Long1
> <long1.huang at intel.com>
> 抄送: Bi, Dandan <dandan.bi at intel.com>; Feng, Bob C
> <bob.c.feng at intel.com>; Li, Zhuangzhi <zhuangzhi.li at intel.com>
> 主题: RE: [edk2-devel] [PATCH] MdeModulePkg/HiiDatabaseDxe: Add
> Support for authenticated variable
>
>
> Liming:
>
> > -----Original Message-----
> > From: gaoliming <gaoliming at byosoft.com.cn>
> > Sent: Wednesday, January 19, 2022 10:57 AM
> > To: Chen, Lin Z <lin.z.chen at intel.com>; devel at edk2.groups.io; Huang,
> Long1
> > <long1.huang at intel.com>
> > Cc: Bi, Dandan <dandan.bi at intel.com>; Feng, Bob C
> <bob.c.feng at intel.com>;
> > Zhang, Di <di.zhang at intel.com>; Li, Zhuangzhi <zhuangzhi.li at intel.com>
> > Subject: 回复: [edk2-devel] [PATCH] MdeModulePkg/HiiDatabaseDxe: Add
> > Support for authenticated variable
> >
> > Lin:
> >
> > > -----邮件原件-----
> > > 发件人: Chen, Lin Z <lin.z.chen at intel.com>
> > > 发送时间: 2022年1月18日 8:42
> > > 收件人: Gao, Liming <gaoliming at byosoft.com.cn>;
> devel at edk2.groups.io;
> > > Huang, Long1 <long1.huang at intel.com>
> > > 抄送: Bi, Dandan <dandan.bi at intel.com>; Feng, Bob C
> > > <bob.c.feng at intel.com>; Zhang, Di <di.zhang at intel.com>; Li, Zhuangzhi
> > > <zhuangzhi.li at intel.com>
> > > 主题: RE: [edk2-devel] [PATCH] MdeModulePkg/HiiDatabaseDxe: Add
> > > Support for authenticated variable
> > >
> > > Hi Liming,
> > >
> > > Pls see my comments below.
> > >
> > > 1.
> > > PcdNvStoreDefaultValueBuffer type is DynamicEx. Its PcdGetPtr is same
> to
> > > PcdGetExPtr. This change is not required.
> > >
> > > [Lin] It'll get alignment with PEI phase reference If using PcdGetExPtr
> > version.
> > > Edk2/MdeModulePkg/Universal/PCD/Pei/Pcd.c#166
> > > DataBuffer = (UINT8 *)PeiPcdGetPtrEx
> (&gEfiMdeModulePkgTokenSpaceGuid,
> > > PcdToken (PcdNvStoreDefaultValueBuffer));
> > >
> > PcdPei is different. It is Pcd driver. It can't consume the real PcdLib.
> > So, it doesn't use PcdLib PcdGetPtr() API.
> > It uses the internal function PeiPcdGetPtrEx() to get PCD value and size.
> >
> > HiiDataBase is normal DXE driver. It can depend on the real PcdLib. So, it
> > uses PcdLib PcdGetPtr() API.
> > Seemly, current code brings confuse. If so, I am OK to make this change.
> > And, please also update PcdGetSize (PcdNvStoreDefaultValueBuffer) with
> > LibPcdGetExSize()
> >
> > > 2.
> > > VariableStorage data buffer is from PcdNvStoreDefaultValueBuffer.
> > > PcdNvStoreDefaultValueBuffer is auto generated by BaseTools.
> > > By design, its data format is always normal variable storage format.
> > > So, its value can't be auth variable format.
> > >
> > > [Lin] BaseTools can generate authenticated variable storage format now
> > (see
> > > https://edk2.groups.io/g/devel/message/83329),
> > > since previous FCE tool generates authenticated format, we want to
> keep
> > > variable storage format no changes after switching to StructurePcd.
> > >
> > Sorry, I miss the previous change in BaseTools. Normal variable storage is
> > enough for Tools and Code.
> > The default setting format can be always normal variable storage. The
> > consumer code logic can be simple.
> > I don't know what purpose to keep the alignment with FCE tool behavior. Is
> > there other tool or code to use
> > the generated PcdNvStoreDefaultValueBuffer?
> >
>
> We have the usage that writing the entire VPD (which has default setup value)
> to variable region upon first boot. And the variable region needs to be in
> authenticated format when secure boot is enabled, so we also need the VPD
> in
> authenticated format. Hope it clarifies your question.
>
Current usage is to build VariableHob based on PcdNvStoreDefaultValueBuffer data in PEI,
and write VariableHob into NV space in DXE. Variable driver is responsible to convert
normal variable format to the auth variable format.
Have you another usage model to write entire VPD data to variable region? How is
VPD data placed into variable region? Is VPD data wrapped as the variable format?
If so, VPD data is also variable data. I think Variable driver should check NV region
to decide the variable format instead of VPD data.
Can you add the background about new usage model into https://bugzilla.tianocore.org/show_bug.cgi?id=3796?
Thanks
Liming
> > Thanks
> > Liming
> > > Thanks,
> > > Lin
> > >
> > > -----Original Message-----
> > > From: gaoliming <gaoliming at byosoft.com.cn>
> > > Sent: Monday, January 17, 2022 6:56 PM
> > > To: devel at edk2.groups.io; Huang, Long1 <long1.huang at intel.com>
> > > Cc: Chen, Lin Z <lin.z.chen at intel.com>; Bi, Dandan
> <dandan.bi at intel.com>;
> > > Feng, Bob C <bob.c.feng at intel.com>
> > > Subject: 回复: [edk2-devel] [PATCH] MdeModulePkg/HiiDatabaseDxe:
> Add
> > > Support for authenticated variable
> > >
> > > Long:
> > > I add my comments below.
> > >
> > > > -----邮件原件-----
> > > > 发件人: devel at edk2.groups.io <devel at edk2.groups.io> 代表 Long1
> > > Huang
> > > > 发送时间: 2022年1月11日 1:03
> > > > 收件人: devel at edk2.groups.io
> > > > 抄送: Huang Long <long1.huang at intel.com>; Liming Gao
> > > > <gaoliming at byosoft.com.cn>; Chen Lin Z <lin.z.chen at intel.com>;
> Dandan
> > > > Bi <dandan.bi at intel.com>
> > > > 主题: [edk2-devel] [PATCH] MdeModulePkg/HiiDatabaseDxe: Add
> Support
> > > for
> > > > authenticated variable
> > > >
> > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3796
> > > >
> > > > Database.c:
> > > > 1. Replace PcdGetExPtr with PcdGetExPtr.
> > > > 2. Add FindAuthVariableData function to parse authenticated
> variable
> > > > type for getting a correct default value in
> > PcdNvStoreDefaultValueBuffer.
> > > >
> > > > Cc: Liming Gao <gaoliming at byosoft.com.cn>
> > > > Cc: Chen Lin Z <lin.z.chen at intel.com>
> > > > Cc: Dandan Bi <dandan.bi at intel.com>
> > > >
> > > > Signed-off-by: Huang Long <long1.huang at intel.com>
> > > > ---
> > > > .../Universal/HiiDatabaseDxe/Database.c | 147
> > > +++++++++++++-----
> > > > .../HiiDatabaseDxe/HiiDatabaseDxe.inf | 3 +
> > > > 2 files changed, 114 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
> > > > b/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
> > > > index 0b09c24d52..c055fa0f29 100644
> > > > --- a/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
> > > > +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
> > > > @@ -603,6 +603,45 @@ FindVariableData (
> > > > return NULL;
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > > +/**
> > > >
> > > > + Find the matched authenticated variable from the input variable
> > > storage.
> > > >
> > > > +
> > > >
> > > > + @param[in] VariableStorage Point to the variable storage header.
> > > >
> > > > + @param[in] VarGuid A unique identifier for the variable.
> > > >
> > > > + @param[in] VarAttribute The attributes bitmask for the
> variable.
> > > >
> > > > + @param[in] VarName A Null-terminated ascii string that
> is
> > > the
> > > > name of the variable.
> > > >
> > > > +
> > > >
> > > > + @return Pointer to the matched variable header or NULL if not
> found.
> > > >
> > > > +**/
> > > >
> > > > +AUTHENTICATED_VARIABLE_HEADER *
> > > >
> > > > +FindAuthVariableData (
> > > >
> > > > + IN VARIABLE_STORE_HEADER *VariableStorage,
> > > >
> > > > + IN EFI_GUID *VarGuid,
> > > >
> > > > + IN UINT32 VarAttribute,
> > > >
> > > > + IN CHAR16 *VarName
> > > >
> > > > + )
> > > >
> > > > +{
> > > >
> > > > + AUTHENTICATED_VARIABLE_HEADER *AuthVariableHeader;
> > > >
> > > > + AUTHENTICATED_VARIABLE_HEADER *AuthVariableEnd;
> > > >
> > > > +
> > > >
> > > > + AuthVariableEnd = (AUTHENTICATED_VARIABLE_HEADER
> > > *)((UINT8
> > > > *)VariableStorage + VariableStorage->Size);
> > > >
> > > > + AuthVariableHeader = (AUTHENTICATED_VARIABLE_HEADER
> > > > *)(VariableStorage + 1);
> > > >
> > > > + AuthVariableHeader = (AUTHENTICATED_VARIABLE_HEADER
> > > > *)HEADER_ALIGN (AuthVariableHeader);
> > > >
> > > > + while (AuthVariableHeader < AuthVariableEnd) {
> > > >
> > > > + if (CompareGuid (&AuthVariableHeader->VendorGuid, VarGuid)
> &&
> > > >
> > > > + (AuthVariableHeader->Attributes == VarAttribute) &&
> > > >
> > > > + (StrCmp (VarName, (CHAR16 *)(AuthVariableHeader + 1)) ==
> 0))
> > > >
> > > > + {
> > > >
> > > > + return AuthVariableHeader;
> > > >
> > > > + }
> > > >
> > > > +
> > > >
> > > > + AuthVariableHeader = (AUTHENTICATED_VARIABLE_HEADER
> > > > *)((UINT8 *)AuthVariableHeader + sizeof
> > > > (AUTHENTICATED_VARIABLE_HEADER) +
> AuthVariableHeader->NameSize +
> > > > AuthVariableHeader->DataSize);
> > > >
> > > > + AuthVariableHeader = (AUTHENTICATED_VARIABLE_HEADER
> > > > *)HEADER_ALIGN (AuthVariableHeader);
> > > >
> > > > + }
> > > >
> > > > +
> > > >
> > > > + return NULL;
> > > >
> > > > +}
> > > >
> > > > +
> > > >
> > > > /**
> > > >
> > > > Find question default value from PcdNvStoreDefaultValueBuffer
> > > >
> > > >
> > > >
> > > > @@ -626,25 +665,27 @@ FindQuestionDefaultSetting (
> > > > IN BOOLEAN BitFieldQuestion
> > > >
> > > > )
> > > >
> > > > {
> > > >
> > > > - VARIABLE_HEADER *VariableHeader;
> > > >
> > > > - VARIABLE_STORE_HEADER *VariableStorage;
> > > >
> > > > - LIST_ENTRY *Link;
> > > >
> > > > - VARSTORAGE_DEFAULT_DATA *Entry;
> > > >
> > > > - VARIABLE_STORE_HEADER *NvStoreBuffer;
> > > >
> > > > - UINT8 *DataBuffer;
> > > >
> > > > - UINT8 *BufferEnd;
> > > >
> > > > - BOOLEAN IsFound;
> > > >
> > > > - UINTN Index;
> > > >
> > > > - UINT32 BufferValue;
> > > >
> > > > - UINT32 BitFieldVal;
> > > >
> > > > - UINTN BitOffset;
> > > >
> > > > - UINTN ByteOffset;
> > > >
> > > > - UINTN BitWidth;
> > > >
> > > > - UINTN StartBit;
> > > >
> > > > - UINTN EndBit;
> > > >
> > > > - PCD_DEFAULT_DATA *DataHeader;
> > > >
> > > > - PCD_DEFAULT_INFO *DefaultInfo;
> > > >
> > > > - PCD_DATA_DELTA *DeltaData;
> > > >
> > > > + VARIABLE_HEADER *VariableHeader;
> > > >
> > > > + AUTHENTICATED_VARIABLE_HEADER *AuthVariableHeader;
> > > >
> > > > + VARIABLE_STORE_HEADER *VariableStorage;
> > > >
> > > > + LIST_ENTRY *Link;
> > > >
> > > > + VARSTORAGE_DEFAULT_DATA *Entry;
> > > >
> > > > + VARIABLE_STORE_HEADER *NvStoreBuffer;
> > > >
> > > > + UINT8 *DataBuffer;
> > > >
> > > > + UINT8 *BufferEnd;
> > > >
> > > > + BOOLEAN AuthFormat;
> > > >
> > > > + BOOLEAN IsFound;
> > > >
> > > > + UINTN Index;
> > > >
> > > > + UINT32 BufferValue;
> > > >
> > > > + UINT32 BitFieldVal;
> > > >
> > > > + UINTN BitOffset;
> > > >
> > > > + UINTN ByteOffset;
> > > >
> > > > + UINTN BitWidth;
> > > >
> > > > + UINTN StartBit;
> > > >
> > > > + UINTN EndBit;
> > > >
> > > > + PCD_DEFAULT_DATA *DataHeader;
> > > >
> > > > + PCD_DEFAULT_INFO *DefaultInfo;
> > > >
> > > > + PCD_DATA_DELTA *DeltaData;
> > > >
> > > >
> > > >
> > > > if (gSkuId == 0xFFFFFFFFFFFFFFFF) {
> > > >
> > > > gSkuId = LibPcdGetSku ();
> > > >
> > > > @@ -666,7 +707,7 @@ FindQuestionDefaultSetting (
> > > > }
> > > >
> > > >
> > > >
> > > > if (Link == &gVarStorageList) {
> > > >
> > > > - DataBuffer = (UINT8 *)PcdGetPtr
> > > > (PcdNvStoreDefaultValueBuffer);
> > > >
> > > > + DataBuffer = (UINT8 *)PcdGetExPtr
> > > > (&gEfiMdeModulePkgTokenSpaceGuid, PcdNvStoreDefaultValueBuffer);
> > > >
> > > PcdNvStoreDefaultValueBuffer type is DynamicEx. Its PcdGetPtr is same
> to
> > > PcdGetExPtr. This change is not required.
> > >
> > > > gNvDefaultStoreSize =
> > > ((PCD_NV_STORE_DEFAULT_BUFFER_HEADER
> > > > *)DataBuffer)->Length;
> > > >
> > > > //
> > > >
> > > > // The first section data includes NV storage default setting.
> > > >
> > > > @@ -750,12 +791,27 @@ FindQuestionDefaultSetting (
> > > > return EFI_NOT_FOUND;
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > > + //
> > > >
> > > > + // Judge if the variable type is authenticated, default is false
> > > >
> > > > + //
> > > >
> > > > + AuthFormat = FALSE;
> > > >
> > > > + if (CompareGuid (&VariableStorage->Signature,
> > > > &gEfiAuthenticatedVariableGuid)) {
> > > >
> > > > + AuthFormat = TRUE;
> > > >
> > > > + }
> > > >
> > > > +
> > > >
> > >
> > >
> > > > //
> > > >
> > > > // Find the question default value from the variable storage
> > > >
> > > > //
> > > >
> > > > - VariableHeader = FindVariableData (VariableStorage,
> > > > &EfiVarStore->Guid,
> > > > EfiVarStore->Attributes, (CHAR16 *)EfiVarStore->Name);
> > > >
> > > > - if (VariableHeader == NULL) {
> > > >
> > > > - return EFI_NOT_FOUND;
> > > >
> > > > + if(AuthFormat) {
> > > >
> > > > + AuthVariableHeader = FindAuthVariableData (VariableStorage,
> > > > &EfiVarStore->Guid, EfiVarStore->Attributes, (CHAR16
> > > > *)EfiVarStore->Name);
> > > >
> > > > + if (AuthVariableHeader == NULL) {
> > > >
> > > > + return EFI_NOT_FOUND;
> > > >
> > > > + }
> > > >
> > > > + } else {
> > > >
> > > > + VariableHeader = FindVariableData (VariableStorage,
> > > > &EfiVarStore->Guid, EfiVarStore->Attributes, (CHAR16
> > > > *)EfiVarStore->Name);
> > > >
> > > > + if (VariableHeader == NULL) {
> > > >
> > > > + return EFI_NOT_FOUND;
> > > >
> > > > + }
> > > >
> > > > }
> > > >
> > > VariableStorage data buffer is from PcdNvStoreDefaultValueBuffer.
> > > PcdNvStoreDefaultValueBuffer is auto generated by BaseTools.
> > > By design, its data format is always normal variable storage format.
> > > So, its value can't be auth variable format.
> > >
> > > Thanks
> > > Liming
> > > >
> > > >
> > > > StartBit = 0;
> > > >
> > > > @@ -770,20 +826,39 @@ FindQuestionDefaultSetting (
> > > > Width = EndBit / 8 + 1;
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > > - if (VariableHeader->DataSize < ByteOffset + Width) {
> > > >
> > > > - return EFI_INVALID_PARAMETER;
> > > >
> > > > - }
> > > >
> > > > + if(AuthFormat) {
> > > >
> > > > + if (AuthVariableHeader->DataSize < ByteOffset + Width) {
> > > >
> > > > + return EFI_INVALID_PARAMETER;
> > > >
> > > > + }
> > > >
> > > >
> > > >
> > > > - //
> > > >
> > > > - // Copy the question value
> > > >
> > > > - //
> > > >
> > > > - if (ValueBuffer != NULL) {
> > > >
> > > > - if (BitFieldQuestion) {
> > > >
> > > > - CopyMem (&BufferValue, (UINT8 *)VariableHeader + sizeof
> > > > (VARIABLE_HEADER) + VariableHeader->NameSize + ByteOffset, Width);
> > > >
> > > > - BitFieldVal = BitFieldRead32 (BufferValue, StartBit, EndBit);
> > > >
> > > > - CopyMem (ValueBuffer, &BitFieldVal, Width);
> > > >
> > > > - } else {
> > > >
> > > > - CopyMem (ValueBuffer, (UINT8 *)VariableHeader + sizeof
> > > > (VARIABLE_HEADER) + VariableHeader->NameSize +
> > > > IfrQuestionHdr->VarStoreInfo.VarOffset, Width);
> > > >
> > > > + //
> > > >
> > > > + // Copy the question value
> > > >
> > > > + //
> > > >
> > > > + if (ValueBuffer != NULL) {
> > > >
> > > > + if (BitFieldQuestion) {
> > > >
> > > > + CopyMem (&BufferValue, (UINT8 *)AuthVariableHeader +
> sizeof
> > > > (AUTHENTICATED_VARIABLE_HEADER) +
> AuthVariableHeader->NameSize +
> > > > ByteOffset, Width);
> > > >
> > > > + BitFieldVal = BitFieldRead32 (BufferValue, StartBit, EndBit);
> > > >
> > > > + CopyMem (ValueBuffer, &BitFieldVal, Width);
> > > >
> > > > + } else {
> > > >
> > > > + CopyMem (ValueBuffer, (UINT8 *)AuthVariableHeader +
> sizeof
> > > > (AUTHENTICATED_VARIABLE_HEADER) +
> AuthVariableHeader->NameSize +
> > > > IfrQuestionHdr->VarStoreInfo.VarOffset, Width);
> > > >
> > > > + }
> > > >
> > > > + }
> > > >
> > > > + } else {
> > > >
> > > > + if (VariableHeader->DataSize < ByteOffset + Width) {
> > > >
> > > > + return EFI_INVALID_PARAMETER;
> > > >
> > > > + }
> > > >
> > > > +
> > > >
> > > > + //
> > > >
> > > > + // Copy the question value
> > > >
> > > > + //
> > > >
> > > > + if (ValueBuffer != NULL) {
> > > >
> > > > + if (BitFieldQuestion) {
> > > >
> > > > + CopyMem (&BufferValue, (UINT8 *)VariableHeader + sizeof
> > > > (VARIABLE_HEADER) + VariableHeader->NameSize + ByteOffset, Width);
> > > >
> > > > + BitFieldVal = BitFieldRead32 (BufferValue, StartBit, EndBit);
> > > >
> > > > + CopyMem (ValueBuffer, &BitFieldVal, Width);
> > > >
> > > > + } else {
> > > >
> > > > + CopyMem (ValueBuffer, (UINT8 *)VariableHeader + sizeof
> > > > (VARIABLE_HEADER) + VariableHeader->NameSize +
> > > > IfrQuestionHdr->VarStoreInfo.VarOffset, Width);
> > > >
> > > > + }
> > > >
> > > > }
> > > >
> > > > }
> > > >
> > > >
> > > >
> > > > diff --git
> a/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
> > > > b/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
> > > > index 0116fb6ecb..dac4d614a8 100644
> > > > --- a/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
> > > > +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabaseDxe.inf
> > > > @@ -86,6 +86,9 @@
> > > > gEfiHiiImageDecoderNameJpegGuid
> > > > |gEfiMdeModulePkgTokenSpaceGuid.PcdSupportHiiImageProtocol ##
> > > > SOMETIMES_CONSUMES ## GUID
> > > >
> > > > gEfiHiiImageDecoderNamePngGuid
> > > > |gEfiMdeModulePkgTokenSpaceGuid.PcdSupportHiiImageProtocol ##
> > > > SOMETIMES_CONSUMES ## GUID
> > > >
> > > > gEdkiiIfrBitVarstoreGuid
> > > > ## SOMETIMES_CONSUMES ## GUID
> > > >
> > > > + gEfiAuthenticatedVariableGuid
> > > >
> > > > + gEfiVariableGuid
> > > >
> > > > + gEfiMdeModulePkgTokenSpaceGuid
> > > >
> > > >
> > > >
> > > > [Depex]
> > > >
> > > > TRUE
> > > >
> > > > --
> > > > 2.25.1
> > > >
> > > >
> > > >
> > > > -=-=-=-=-=-=
> > > > Groups.io Links: You receive all messages sent to this group.
> > > > View/Reply Online (#85447):
> > > > https://edk2.groups.io/g/devel/message/85447
> > > > Mute This Topic: https://groups.io/mt/88319448/4905953
> > > > Group Owner: devel+owner at edk2.groups.io
> > > > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > > > [gaoliming at byosoft.com.cn]
> > > > -=-=-=-=-=-=
> > > >
> > >
> > >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85891): https://edk2.groups.io/g/devel/message/85891
Mute This Topic: https://groups.io/mt/88577691/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list