Aaargh I forgot to rewrap the long lines. Resending verbatim, just with
the rewrapping. Apologies!

Laszlo


comments below


On 01/09/14 01:45, Jordan Justen wrote:
> From: Laszlo Ersek <[email protected]>
>
> The S3 suspend/resume infrastructure depends on the LockBox library class.
> The edk2 tree currently contains Null and SMM instances. The Null instance
> is useless, and the SMM instance would require SMM emulation by including
> the SMM core and adding several new drivers, which is deemed too complex.
>
> Hence add a simple LockBoxLib instance for OVMF.
>
> [email protected]:
>  * use PCDs instead of EmuNvramLib
>    - clear memory in PlatformPei on non S3 boots
>  * allocate NVS memory and store a pointer to that memory
>    - reduces memory use at fixed locations


> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 091f012..dd88e1d 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -24,6 +24,7 @@ Module Name:
>  //
>  // The Library classes this module consumes
>  //
> +#include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/HobLib.h>
>  #include <Library/IoLib.h>
> @@ -201,6 +202,22 @@ MemDetect (
>        EfiACPIMemoryNVS
>        );
>  #endif
> +
> +    //
> +    // Reserve the lock box storage area
> +    //
> +    // Since this memory range will be used on S3 resume, it must be
> +    // reserved as ACPI NVS.
> +    //
> +    ZeroMem (
> +      (VOID*)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
> +      (UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize)
> +      );

Correct, this is a bug in my v3 (EmuNvramLib). I missed the following
scenario:
- VM is started
- lockbox is populated before booting, in preparation for resume
- guest is rebooted inside same qemu process (guest RAM remains intact)
- lockbox still contains the old keys (GUIDs), so S3 setup fails
  (unfortunately with an ASSERT()!)

> +    BuildMemoryAllocationHob (
> +      (EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
> +      (UINT64)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize),
> +      EfiACPIMemoryNVS
> +      );
>    }
>
>    return LowerMemorySize;

(Same comment about the reservation not happening on Xen.)

> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf 
> b/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf

> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = LockBoxLib

Shouldn't this say "LockBoxBaseLib"?

> +  FILE_GUID                      = 17CA9B37-5BAB-492C-A09C-7121FBE34CE6
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = LockBoxLib

> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf 
> b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf

> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = LockBoxLib

Shouldn't this say "LockBoxDxeLib"?

> +  FILE_GUID                      = 17CA9B37-5BAB-492C-A09C-7121FBE34CE6

I think we should use different FILE_GUIDs for the two library
instances.

> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c 
> b/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c

> +/**
> +  Allocates a buffer of type EfiACPIMemoryNVS.
> +
> +  Allocates the number bytes specified by AllocationSize of type
> +  EfiACPIMemoryNVS and returns a pointer to the allocated buffer.
> +  If AllocationSize is 0, then a valid buffer of 0 size is
> +  returned.  If there is not enough memory remaining to satisfy
> +  the request, then NULL is returned.
> +
> +  @param  AllocationSize        The number of bytes to allocate.
> +
> +  @return A pointer to the allocated buffer or NULL if allocation fails.
> +
> +**/
> +VOID *
> +EFIAPI
> +AllocateAcpiNvsPool (
> +  IN UINTN  AllocationSize
> +  )
> +{
> +  EFI_STATUS  Status;
> +  VOID        *Memory;
> +
> +  Status = gBS->AllocatePool (EfiACPIMemoryNVS, AllocationSize, &Memory);
> +  if (EFI_ERROR (Status)) {
> +    Memory = NULL;
> +  }
> +  return Memory;
> +}

This allocation should be constrained below 4GB.

Consider a 64-bit DXE and a guest RAM size > 4GB. The allocation is
likely to return a pointer that can't be represented as UINT32. We'll
store it in the CopyAddress field just fine, but:

- A 32-bit PEI won't be able to convert CopyAddress to a (VOID *). We'll
catch it with RETURN_UNSUPPORTED, but lockbox restoration will fail
nonetheless.

- A 64-bit PEI will convert CopyAddress to a (VOID *) alright, but
dereferencing the pointer will cause a page fault, since the PEI page
tables (built by the X64 reset vector) cover only the first 4GB.

Earlier I made an effort to track down all the Reserved and AcpiNVS
memory allocations performed internally by the DXE drivers that are
related to S3 (bootscript etc) and saved to lockbox. They are *all*
constrained below 4GB. For example:

- IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c:
  uses a wrapper called AllocateMemoryBelow4G(),

- MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c:
  various gBS->AllocatePages() calls, with AllocateMaxAddress and 4GB-1,

- MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c:
  same

So we don't have to worry about the OrigAddress values when restoring
in-place, even the last byte of each such a restoration-target range
fits under 4GB. (I mean, we should keep the checks, but we can expect
them never to fire.)

Furthermore, when restoring to an auto variable, that's below 4GB too
(it's on the temporary PEI stack, or on the migrated (permanent) PEI
stack, both of which we control). See

http://article.gmane.org/gmane.comp.bios.tianocore.devel/5749/raw

which I captured and cross-referenced for an 8 GB guest (X64/X64).

So, we don't have to worry about OrigAddress (nor
RestoreLockBox():Buffer) failing or checks, but we must ensure
(CopyAddress + AllocationSize <= 4GB).

> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxLib.c 
> b/OvmfPkg/Library/LockBoxLib/LockBoxLib.c
> new file mode 100644
> index 0000000..6146389
> --- /dev/null
> +++ b/OvmfPkg/Library/LockBoxLib/LockBoxLib.c
> @@ -0,0 +1,358 @@
> +/** @file
> +
> +  Library implementing the LockBox interface for OVMF
> +
> +  Copyright (C) 2013, Red Hat, Inc.
> +  Copyright (c) 2010 - 2014, Intel Corporation. All rights reserved.<BR>
> +
> +  This program and the accompanying materials are licensed and made available
> +  under the terms and conditions of the BSD License which accompanies this
> +  distribution. The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
> WITHOUT
> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <Uefi.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/LockBoxLib.h>
> +#include <Library/PcdLib.h>
> +#include <LockBoxLib.h>
> +
> +#pragma pack(1)
> +typedef struct {
> +  EFI_GUID             Guid;
> +  EFI_PHYSICAL_ADDRESS OrigAddress;
> +  EFI_PHYSICAL_ADDRESS CopyAddress;
> +  UINT32               Size;
> +  UINT64               Attributes;
> +} LOCK_BOX_HEADER;
> +#pragma pack()
> +
> +

OK

> +/**
> +  Find LockBox entry based on GUID.
> +
> +  @param[in] Guid  The GUID to search for.
> +
> +  @return  Address of the LOCK_BOX_HEADER found. If LOCK_BOX_HEADER.Size is
> +           greater than sizeof(LOCK_BOX_HEADER), then the returned header
> +           contains the Guid of interest. Otherwise, the Guid was not found 
> and
> +           the terminator header is returned (whose size is exactly
> +           sizeof(LOCK_BOX_HEADER)).
> +**/
> +STATIC
> +LOCK_BOX_HEADER *
> +EFIAPI
> +FindHeaderByGuid (
> +  IN CONST EFI_GUID *Guid
> +  )
> +{
> +  LOCK_BOX_HEADER *Start, *Header, *End;

Heh, isn't this a *grave* violation of the coding style? :)

> +
> +  Start = (LOCK_BOX_HEADER *)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase);
> +  End = Start + (PcdGet32 (PcdOvmfLockBoxStorageBase) / sizeof (*Start));

Typo, the PCD should be xxxSize rather than xxxBase.

> +  for (Header = Start; Header < End; Header++) {
> +    if (Header->Size == 0 || CompareGuid (Guid, &Header->Guid)) {
> +      return Header;
> +    }
> +  }
> +
> +  return NULL;
> +}

The meaning of LOCK_BOX_HEADER.Size has changed, relative to v3, which
is fine per se. The leading comment has not been updated however:
- the terminator header now has Size==0,
- a NULL retval (impossible in v3) means "not found and we're also out
  of space". In this case we don't have a terminator header apparently
  (also impossible in v3). I think it should be documented.

Anyway, just so I understand the rest of the code:
- we only store fixed size headers,
- the full size of the store is known,
- the in-use part of the store is terminated with a header where
  Size==0,
- Size==0 is invalid for normal headers,
- if there's no Size==0 header, then the store is completely in-use.


> +
> +
> +/**
> +  This function will save confidential information to lockbox.
> +
> +  @param Guid       the guid to identify the confidential information
> +  @param Buffer     the address of the confidential information
> +  @param Length     the length of the confidential information
> +
> +  @retval RETURN_SUCCESS            the information is saved successfully.
> +  @retval RETURN_INVALID_PARAMETER  the Guid is NULL, or Buffer is NULL, or
> +                                    Length is 0
> +  @retval RETURN_ALREADY_STARTED    the requested GUID already exist.
> +  @retval RETURN_OUT_OF_RESOURCES   no enough resource to save the 
> information.
> +  @retval RETURN_ACCESS_DENIED      it is too late to invoke this interface
> +  @retval RETURN_NOT_STARTED        it is too early to invoke this interface
> +  @retval RETURN_UNSUPPORTED        the service is not supported by
> +                                    implementaion.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SaveLockBox (
> +  IN  GUID                        *Guid,
> +  IN  VOID                        *Buffer,
> +  IN  UINTN                       Length
> +  )
> +{
> +  LOCK_BOX_HEADER *Header;
> +  VOID            *CopyBuffer;
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: Guid=%g Buffer=%p Length=0x%x\n", __FUNCTION__,
> +    Guid, Buffer, (UINT32) Length));
> +
> +  if (Guid == NULL || Buffer == NULL || Length == 0) {
> +    return RETURN_INVALID_PARAMETER;
> +  }
> +
> +  if (Length > 0xFFFFFFFF - sizeof *Header) {
> +    return RETURN_OUT_OF_RESOURCES;
> +  }

This check should be updated (to compare Length against 0xFFFFFFFF
only), because LOCK_BOX_HEADER.Size doesn't include the header itself
any longer.

> +
> +  Header = FindHeaderByGuid (Guid);
> +  if (Header == NULL) {
> +    return RETURN_OUT_OF_RESOURCES;
> +  }

OK. Otherwise we found either the GUID or the terminator.

> +
> +  if (Header->Size > 0) {
> +    return RETURN_ALREADY_STARTED;
> +  }

OK.

> +
> +  CopyBuffer = AllocateAcpiNvsPool (Length);
> +  if (CopyBuffer == NULL) {
> +    return RETURN_OUT_OF_RESOURCES;
> +  }
> +
> +  //
> +  // overwrite the current terminator header with new metadata
> +  //
> +  CopyGuid (&Header->Guid, Guid);
> +  Header->OrigAddress = (UINTN) Buffer;
> +  Header->CopyAddress = (UINTN) CopyBuffer;
> +  Header->Size        = Length;
> +  Header->Attributes  = 0;
> +
> +  //
> +  // copy contents
> +  //
> +  CopyMem (CopyBuffer, Buffer, Length);

OK, and we don't add the new terminator header explicitly, because:
- the lockbox store can only grow, and
- we have zero-initialized the entire storage (setting all possible Size
  fields to zero).

> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: Guid=%g Buffer=%p Length=0x%x\n", __FUNCTION__,
> +    Guid, Buffer, (UINT32) Length));

I think we should keep the debug message either at front or at rear, or
at least make them say different things.

> +  return RETURN_SUCCESS;
> +}
> +
> +
> +/**
> +  This function will set lockbox attributes.
> +
> +  @param Guid       the guid to identify the confidential information
> +  @param Attributes the attributes of the lockbox
> +
> +  @retval RETURN_SUCCESS            the information is saved successfully.
> +  @retval RETURN_INVALID_PARAMETER  attributes is invalid.
> +  @retval RETURN_NOT_FOUND          the requested GUID not found.
> +  @retval RETURN_ACCESS_DENIED      it is too late to invoke this interface
> +  @retval RETURN_NOT_STARTED        it is too early to invoke this interface
> +  @retval RETURN_UNSUPPORTED        the service is not supported by
> +                                    implementaion.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SetLockBoxAttributes (
> +  IN  GUID                        *Guid,
> +  IN  UINT64                      Attributes
> +  )
> +{
> +  LOCK_BOX_HEADER *Header;
> +
> +  if (Guid == NULL) {
> +    return RETURN_INVALID_PARAMETER;
> +  }
> +  if (PcdGet32 (PcdOvmfLockBoxStorageSize) == 0) {
> +    return RETURN_UNSUPPORTED;
> +  }

Not sure if we need to check for this explicitly any more.
FindHeaderByGuid() handles undersized stores just fine (resulting in
out-of-resources when saving, and not-found below).

Admittedly, the interface contract requires RETURN_UNSUPPORTED, but then
we should return that code in SaveLockBox() too for an undersized
storage, rather than RETURN_OUT_OF_RESOURCES.

> +
> +  Header = FindHeaderByGuid (Guid);
> +  if (!Header || Header->Size == 0) {
> +    return RETURN_NOT_FOUND;
> +  }

Right, "not found and out of space" or "not found and there's some space
left".

> +  Header->Attributes = Attributes;
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: Guid=%g Attributes=0x%Lx\n", __FUNCTION__, 
> Guid,
> +    Attributes));
> +  return RETURN_SUCCESS;
> +}
> +
> +
> +/**
> +  This function will update confidential information to lockbox.
> +
> +  @param Guid   the guid to identify the original confidential information
> +  @param Offset the offset of the original confidential information
> +  @param Buffer the address of the updated confidential information
> +  @param Length the length of the updated confidential information
> +
> +  @retval RETURN_SUCCESS            the information is saved successfully.
> +  @retval RETURN_INVALID_PARAMETER  the Guid is NULL, or Buffer is NULL, or
> +                                    Length is 0.
> +  @retval RETURN_NOT_FOUND          the requested GUID not found.
> +  @retval RETURN_BUFFER_TOO_SMALL   the original buffer to too small to hold
> +                                    new information.
> +  @retval RETURN_ACCESS_DENIED      it is too late to invoke this interface
> +  @retval RETURN_NOT_STARTED        it is too early to invoke this interface
> +  @retval RETURN_UNSUPPORTED        the service is not supported by
> +                                    implementaion.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +UpdateLockBox (
> +  IN  GUID                        *Guid,
> +  IN  UINTN                       Offset,
> +  IN  VOID                        *Buffer,
> +  IN  UINTN                       Length
> +  )
> +{
> +  LOCK_BOX_HEADER *Header;
> +
> +  if (Guid == NULL || Buffer == NULL || Length == 0) {
> +    return RETURN_INVALID_PARAMETER;
> +  }
> +  if (PcdGet32 (PcdOvmfLockBoxStorageSize) == 0) {
> +    return RETURN_UNSUPPORTED;
> +  }

again, handled by FindHeaderByGuid() in one fell swoop (if we don't care
about RETURN_UNSUPPORTED specifically)

> +
> +  Header = FindHeaderByGuid (Guid);
> +  if (!Header || Header->Size == 0) {
> +    return RETURN_NOT_FOUND;
> +  }

OK

> +
> +  if (Header->Size < Offset ||
> +      Offset + Length > Header->Size) {
> +    return RETURN_BUFFER_TOO_SMALL;
> +  }

The first condition is OK.

The second condition is wrong in C (it is correct mathematically). It
should be rewritten as (by subtracting Offset from both sides):

  Length > Header->Size - Offset

Because:
(a) It is mathematically equivalent.

(b) We know -- by not taking the first branch -- that
    (Header->Size >= Offset). Hence the (Header->Size - Offset)
    subtraction won't underflow, we're safe.

(c) In a 32-bit DXE, the addition would be evaluated in UINT32 (both
    UINT32 and UINTN would mean "unsigned int"), and it could overflow.
    Consider the following values:

    - Header->Size == 100u (stored in our storage)
    - Offset == 80u (input param)
    - Length == 0xFFFFFFFFu (input param)

  Header->Size < Offset || Offset + Length      > Header->Size
  100u         < 80u    || 80u    + 0xFFFFFFFFu > 100u
  0                     || 79u                  > 100u
  0                     || 0
  0

and the following CopyMem() would do Very Bad Things (TM). Whereas the
proposed expression


  Header->Size < Offset || Length      > Header->Size - Offset
  100u         < 80u    || 0xFFFFFFFFu > 100u         - 80u
  0                     || 0xFFFFFFFFu > 20u
  0                     || 1
  1

prevents it. (This "strategy" works with all integer sizes, hence it is
preferable over trying to do the addition in a wider type.)


NB the *mathematical* condition that we're checking (whichever way we
write it) would let the following proceed:

  (Header->Size == Offset && Length == 0)

(ie. write zero bytes right after the end of the buffer), and that is
allowed by CopyMem(). But we filter out (Length == 0) at the top, so we
don't even need to care about CopyMem()'s support for it.

> +  CopyMem ((UINT8 *)(UINTN) (Header->CopyAddress) + Offset, Buffer, Length);
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: Guid=%g Offset=0x%x Length=0x%x\n", 
> __FUNCTION__,
> +    Guid, (UINT32) Offset, (UINT32) Length));
> +  return RETURN_SUCCESS;
> +}
> +
> +
> +/**
> +  This function will restore confidential information from lockbox.
> +
> +  @param Guid   the guid to identify the confidential information
> +  @param Buffer the address of the restored confidential information
> +                NULL means restored to original address, Length MUST be NULL 
> at
> +                same time.
> +  @param Length the length of the restored confidential information
> +
> +  @retval RETURN_SUCCESS            the information is restored successfully.
> +  @retval RETURN_INVALID_PARAMETER  the Guid is NULL, or one of Buffer and
> +                                    Length is NULL.
> +  @retval RETURN_WRITE_PROTECTED    Buffer and Length are NULL, but the 
> LockBox
> +                                    has no 
> LOCK_BOX_ATTRIBUTE_RESTORE_IN_PLACE
> +                                    attribute.
> +  @retval RETURN_BUFFER_TOO_SMALL   the Length is too small to hold the
> +                                    confidential information.
> +  @retval RETURN_NOT_FOUND          the requested GUID not found.
> +  @retval RETURN_NOT_STARTED        it is too early to invoke this interface
> +  @retval RETURN_ACCESS_DENIED      not allow to restore to the address
> +  @retval RETURN_UNSUPPORTED        the service is not supported by
> +                                    implementaion.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +RestoreLockBox (
> +  IN  GUID                        *Guid,
> +  IN  VOID                        *Buffer, OPTIONAL
> +  IN  OUT UINTN                   *Length  OPTIONAL
> +  )
> +{
> +  LOCK_BOX_HEADER *Header;
> +
> +  if ((Guid == NULL) ||
> +      ((Buffer == NULL) && (Length != NULL)) ||
> +      ((Buffer != NULL) && (Length == NULL))) {
> +    return EFI_INVALID_PARAMETER;
> +  }

I can see you're not a fan of XOR :)

> +
> +  if (PcdGet32 (PcdOvmfLockBoxStorageSize) == 0) {
> +    return RETURN_UNSUPPORTED;
> +  }

(same comment re RETURN_UNSUPPORTED as above)

> +
> +  Header = FindHeaderByGuid (Guid);
> +  if (!Header || Header->Size == 0) {
> +    return RETURN_NOT_FOUND;
> +  }
> +
> +  if (Buffer == NULL) {
> +    if (!(Header->Attributes & LOCK_BOX_ATTRIBUTE_RESTORE_IN_PLACE)) {
> +      return RETURN_WRITE_PROTECTED;
> +    }
> +    if (Header->OrigAddress + Header->Size > MAX_ADDRESS) {
> +      return RETURN_UNSUPPORTED;
> +    }

Ia32:

  Header->OrigAddress + Header->Size > MAX_ADDRESS
  UINT64                UINT32         0xFFFFFFFF
  unsigned long long    unsigned int   unsigned int

Header->Size is converted to "unsigned long long" for the addition.
MAX_ADDRESS is converted to "unsigned long long" for the comparison. The
addition can't overflow "unsigned long long", by virtue of having saved
the same contiguous range earlier, with CopyMem().

The condition ensures that the entire range can be pointed to with
(VOID*).

X64:

  Header->OrigAddress + Header->Size > MAX_ADDRESS
  UINT64                UINT32         0xFFFFFFFFFFFFFFFFull
  unsigned long long    unsigned int   unsigned long long

Header->Size is converted to "unsigned long long" for the addition. The
addition can't overflow "unsigned long long", by virtue of having saved
the same contiguous range earlier, with CopyMem().

The condition always evaluates to 0, which is correct because (VOID *)
has the same range on X64 as EFI_PHYSICAL_ADDRESS.


Almost OK (both bitnesses), except we should write

  Header->OrigAddress + (Header->Size - 1) > MAX_ADDRESS

Because
(a) Header->Size is positive here, and
(b) MAX_ADDRESS is an inclusive limit (not exclusive); we must compare
    the last written offset against it.


> +    Buffer = (VOID *)(UINTN) Header->OrigAddress;
> +  } else {
> +    if (Header->Size > *Length) {
> +      return RETURN_BUFFER_TOO_SMALL;

I believe in this case we must also set *Length to Header->Size. I'm not
sure where exactly I got the idea from, but this is what usually
accompanies RETURN_BUFFER_TOO_SMALL when Length is IN OUT (which it is
in this function).

> +    }
> +  }
> +
> +  if (Header->CopyAddress + Header->Size > MAX_ADDRESS) {
> +    return RETURN_UNSUPPORTED;
> +  }

You won't need this after forcing AllocateAcpiNvsPool() to stay under
4GB.

> +
> +  CopyMem (Buffer, (VOID*)(UINTN) Header->CopyAddress, Header->Size);
> +  if (Length != NULL) {
> +    *Length = Header->Size;
> +  }

OK, but once you store the needed size near RETURN_BUFFER_TOO_SMALL as
well, you can make that code handle both cases ("too small" and "big
enough") uniformly, so this could go away. If you agree.

> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: Guid=%g Buffer=%p\n", __FUNCTION__, Guid,
> +    Buffer));
> +  return RETURN_SUCCESS;
> +}
> +
> +
> +/**
> +  This function will restore confidential information from all lockbox which
> +  have RestoreInPlace attribute.
> +
> +  @retval RETURN_SUCCESS            the information is restored successfully.
> +  @retval RETURN_NOT_STARTED        it is too early to invoke this interface
> +  @retval RETURN_UNSUPPORTED        the service is not supported by
> +                                    implementaion.
> +**/
> +RETURN_STATUS
> +EFIAPI
> +RestoreAllLockBoxInPlace (
> +  VOID
> +  )
> +{
> +  LOCK_BOX_HEADER *Start, *Header, *End;
> +
> +  Start = (LOCK_BOX_HEADER *)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase);
> +  End = Start + (PcdGet32 (PcdOvmfLockBoxStorageBase) / sizeof (*Start));

Same typo as in FindHeaderByGuid().

Also we don't seem to care about RETURN_UNSUPPORTED here (just like in
SaveLockBox()), so I propose to drop it in all other functions too.

> +
> +  for (Header = Start; Header < End && Header->Size > 0; Header++) {
> +    if (Header->Attributes & LOCK_BOX_ATTRIBUTE_RESTORE_IN_PLACE) {
> +      VOID *Buffer;
> +
> +      if ((UINTN) Header->OrigAddress != Header->OrigAddress) {
> +        return RETURN_UNSUPPORTED;
> +      }

You've kept this from v3 verbatim, but I like your check against
MAX_ADDRESS in RestoreLockBox() better (with the suggested off-by-one
fix):

  Header->OrigAddress + (Header->Size - 1) > MAX_ADDRESS

I missed the case when the base was representable in 32-bit but the end
part wasn't.

> +      Buffer = (VOID *)(UINTN) Header->OrigAddress;
> +      CopyMem (Buffer, (VOID*)(UINTN)Header->CopyAddress, Header->Size);
> +      DEBUG ((DEBUG_VERBOSE, "%a: Guid=%g Buffer=%p\n", __FUNCTION__,
> +        Header->Guid, Buffer));
> +    }
> +  }
> +  return RETURN_SUCCESS;
> +}

Thanks,
Laszlo

------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today. 
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Reply via email to