[edk2-devel] [PATCH 1/2 v5] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
PierreGondois
pierre.gondois at arm.com
Fri Mar 5 17:58:49 UTC 2021
Hi Ilias,
Here is the rest of the review. Sorry to do it in 2 times.
Regards,
Pierre
>
> +/**
>
> + Fixup the Pcd values for variable storage
>
> +
>
> + Since the upper layers of EDK2 expect a memory mapped interface and
> we can't
>
> + offer that from an RPMB, the driver allocates memory on init and
> passes that
>
> + on the upper layers. Since the memory is dynamically allocated and
> we can't set the
>
> + PCD is StMM context, we need to patch it correctly on each access
>
> +
>
> + @retval EFI_SUCCESS Protocol was found and PCDs patched up
The error codes are missing.
>
> +
>
> + **/
>
> +EFI_STATUS
>
> +EFIAPI
>
> +FixPcdMemory (
>
> + VOID
>
> + )
>
> +{
>
> + EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *FvbProtocol;
>
> + MEM_INSTANCE *Instance;
>
> + EFI_STATUS Status;
>
> +
>
> + //
>
> + // Locate SmmFirmwareVolumeBlockProtocol
>
> + //
>
> + Status = gMmst->MmLocateProtocol (
>
> + &gEfiSmmFirmwareVolumeBlockProtocolGuid,
>
> + NULL,
>
> + (VOID **) &FvbProtocol
>
> + );
>
> + ASSERT_EFI_ERROR (Status);
>
> +
>
> + Instance = INSTANCE_FROM_FVB_THIS (FvbProtocol);
>
> + // The Pcd is user defined, so make sure we don't overflow
>
> + if (Instance->MemBaseAddress > MAX_UINT64 - PcdGet32
> (PcdFlashNvStorageVariableSize)) {
I think this can be removed since the next condition is more strict.
>
> + return EFI_INVALID_PARAMETER;
>
> + }
>
> +
>
>
[...]
> +STATIC
>
> +EFI_STATUS
>
> +ReadWriteRpmb (
>
> + UINTN SvcAct,
>
> + UINTN Addr,
>
> + UINTN NumBytes,
>
> + UINTN Offset
>
> + )
>
> +{
>
> + ARM_SVC_ARGS SvcArgs;
>
> + EFI_STATUS Status;
>
> +
>
> + ZeroMem (&SvcArgs, sizeof (SvcArgs));
>
> +
>
> + SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
If this is an FFA call, is it possible to:
- put a reference in the header to the spec (it should be similar to
the one at
edk2/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c)
- check the return status of the SVC call against the ones available
at edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
- if possible, remove the dependency to <IndustryStandard/ArmMmSvc.h>
>
> + SvcArgs.Arg1 = mStorageId;
>
> + SvcArgs.Arg2 = 0;
>
> + SvcArgs.Arg3 = SvcAct;
>
> + SvcArgs.Arg4 = Addr;
>
> + SvcArgs.Arg5 = NumBytes;
>
> + SvcArgs.Arg6 = Offset;
>
> +
>
> + ArmCallSvc (&SvcArgs);
>
> + if (SvcArgs.Arg3) {
>
> + DEBUG ((DEBUG_ERROR, "%a: Svc Call 0x%08x Addr: 0x%08x len: 0x%x
> Offset: 0x%x failed with 0x%x\n",
>
> + __func__, SvcAct, Addr, NumBytes, Offset, SvcArgs.Arg3));
>
> + }
>
> +
>
> + switch (SvcArgs.Arg3) {
>
> + case ARM_SVC_SPM_RET_SUCCESS:
>
> + Status = EFI_SUCCESS;
>
> + break;
>
> +
>
> + case ARM_SVC_SPM_RET_NOT_SUPPORTED:
>
> + Status = EFI_UNSUPPORTED;
>
> + break;
>
> +
>
> + case ARM_SVC_SPM_RET_INVALID_PARAMS:
>
> + Status = EFI_INVALID_PARAMETER;
>
> + break;
>
> +
>
> + case ARM_SVC_SPM_RET_DENIED:
>
> + Status = EFI_ACCESS_DENIED;
>
> + break;
>
> +
>
> + case ARM_SVC_SPM_RET_NO_MEMORY:
>
> + Status = EFI_OUT_OF_RESOURCES;
>
> + break;
>
> +
>
> + default:
>
> + Status = EFI_ACCESS_DENIED;
>
> + }
>
> +
>
> + return Status;
>
> +}
>
[...]
>
> +STATIC
>
> +EFI_STATUS
>
> +EFIAPI
>
> +ValidateFvHeader (
>
> + IN EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader
>
> + )
>
> +{
>
> + UINT16 Checksum;
>
> + VARIABLE_STORE_HEADER *VariableStoreHeader;
>
> + UINTN VariableStoreLength;
>
> + UINTN FvLength;
>
> +
>
> + FvLength = PcdGet32(PcdFlashNvStorageVariableSize) +
>
> + PcdGet32(PcdFlashNvStorageFtwWorkingSize) +
>
> + PcdGet32(PcdFlashNvStorageFtwSpareSize);
>
> +
>
> + // Verify the header revision, header signature, length
>
> + // Length of FvBlock cannot be 2**64-1
>
> + // HeaderLength cannot be an odd number
>
> + //
>
> + if ( (FwVolHeader->Revision != EFI_FVH_REVISION)
>
> + || (FwVolHeader->Signature != EFI_FVH_SIGNATURE)
>
> + || (FwVolHeader->FvLength != FvLength)
>
> + )
could be on the same line -> ') {'
>
> + {
>
> + DEBUG ((DEBUG_INFO, "%a: No Firmware Volume header present\n",
>
> + __FUNCTION__));
>
> + return EFI_NOT_FOUND;
>
> + }
>
> +
>
> + // Check the Firmware Volume Guid
>
> + if (!CompareGuid (&FwVolHeader->FileSystemGuid,
> &gEfiSystemNvDataFvGuid)) {
>
> + DEBUG ((DEBUG_INFO, "%a: Firmware Volume Guid non-compatible\n",
>
> + __FUNCTION__));
>
> + return EFI_VOLUME_CORRUPTED;
>
> + }
>
> +
>
> + // Verify the header checksum
>
> + Checksum = CalculateSum16((UINT16*)FwVolHeader,
> FwVolHeader->HeaderLength);
>
> + if (Checksum != 0) {
>
> + DEBUG ((DEBUG_INFO, "%a: FV checksum is invalid (Checksum:0x%X)\n",
>
> + __FUNCTION__, Checksum));
>
> + return EFI_VOLUME_CORRUPTED;
>
> + }
>
> +
>
> + VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)FwVolHeader +
>
> + FwVolHeader->HeaderLength);
>
> +
>
> + // Check the Variable Store Guid
>
> + if (!CompareGuid (&VariableStoreHeader->Signature,
> &gEfiVariableGuid) &&
>
> + !CompareGuid (&VariableStoreHeader->Signature,
> &gEfiAuthenticatedVariableGuid)) {
>
> + DEBUG ((DEBUG_INFO, "%a: Variable Store Guid non-compatible\n",
> __FUNCTION__));
>
> + return EFI_VOLUME_CORRUPTED;
>
> + }
>
> +
>
> + VariableStoreLength = PcdGet32 (PcdFlashNvStorageVariableSize) -
>
> + FwVolHeader->HeaderLength;
>
> + if (VariableStoreHeader->Size != VariableStoreLength) {
>
> + DEBUG ((DEBUG_INFO, "%a: Variable Store Length does not match\n",
>
> + __FUNCTION__));
>
> + return EFI_VOLUME_CORRUPTED;
>
> + }
>
> +
>
> + return EFI_SUCCESS;
>
empty line, could be removed
> +
>
> +}
>
> +
>
>
[...]
>
> +**/
>
> +STATIC
>
> +EFI_STATUS
>
> +EFIAPI
>
> +FvbInitialize (
>
> + MEM_INSTANCE *Instance
>
> + )
>
> +{
>
> + EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader;
>
> + EFI_STATUS Status;
>
> +
>
> + if (Instance->Initialized) {
>
> + return EFI_SUCCESS;
>
> + }
>
> +
>
> + // FirmwareVolumeHeader->FvLength is declared to have the Variable area
>
> + // AND the FTW working area AND the FTW Spare contiguous.
>
> + ASSERT (
>
> + (PcdGet64 (PcdFlashNvStorageVariableBase64) +
>
> + PcdGet32 (PcdFlashNvStorageVariableSize)) ==
>
> + PcdGet64 (PcdFlashNvStorageFtwWorkingBase64)
>
> + );
>
> + ASSERT (
>
> + (PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) +
>
> + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) ==
>
> + PcdGet64 (PcdFlashNvStorageFtwSpareBase64));
>
> +
>
> + // Check if the size of the area is at least one block size
>
> + ASSERT (
>
> + (PcdGet32 (PcdFlashNvStorageVariableSize) > 0) &&
I think the first check (Size > 0) is redundant with the second one
(Size > BlockSize).
>
> + (PcdGet32 (PcdFlashNvStorageVariableSize) / Instance->BlockSize > 0)
>
> + );
>
> + ASSERT (
>
> + (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > 0) &&
>
> + (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) / Instance->BlockSize
> > 0)
>
> + );
>
> + ASSERT (
>
> + (PcdGet32 (PcdFlashNvStorageFtwSpareSize) > 0) &&
>
> + (PcdGet32 (PcdFlashNvStorageFtwSpareSize) / Instance->BlockSize > 0)
>
> + );
>
> +
>
> + // Ensure the Variable areas are aligned on block size boundaries
>
> + ASSERT ((PcdGet64 (PcdFlashNvStorageVariableBase64) %
> Instance->BlockSize) == 0);
>
> + ASSERT ((PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) %
> Instance->BlockSize) == 0);
>
> + ASSERT ((PcdGet64 (PcdFlashNvStorageFtwSpareBase64) %
> Instance->BlockSize) == 0);
>
> +
>
> + // Read the file from disk and copy it to memory
>
> + ReadEntireFlash (Instance);
>
> +
>
> + FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->MemBaseAddress;
>
> + Status = ValidateFvHeader (FwVolHeader);
>
> + if (EFI_ERROR (Status)) {
>
> + // There is no valid header, so time to install one.
>
> + DEBUG ((DEBUG_INFO, "%a: The FVB Header is not valid.\n",
> __FUNCTION__));
>
> +
>
> + // Reset memory
>
> + SetMem64 ((VOID *)Instance->MemBaseAddress, Instance->NBlocks *
> Instance->BlockSize, ~0UL);
>
> + DEBUG ((DEBUG_INFO, "%a: Erasing Flash.\n", __FUNCTION__));
>
> + Status = ReadWriteRpmb (
>
> + SP_SVC_RPMB_WRITE,
>
> + Instance->MemBaseAddress,
>
> + PcdGet32(PcdFlashNvStorageVariableSize) +
>
> + PcdGet32(PcdFlashNvStorageFtwWorkingSize) +
>
> + PcdGet32(PcdFlashNvStorageFtwSpareSize),
>
> + 0
>
> + );
>
> + if (EFI_ERROR (Status)) {
>
> + return Status;
>
> + }
>
> + // Install all appropriate headers
>
> + DEBUG ((DEBUG_INFO, "%a: Installing a correct one for this
> volume.\n",
>
> + __FUNCTION__));
>
> + Status = InitializeFvAndVariableStoreHeaders (Instance);
>
> + if (EFI_ERROR (Status)) {
>
> + return Status;
>
> + }
>
> + } else {
>
> + DEBUG ((DEBUG_INFO, "%a: Found valid FVB Header.\n", __FUNCTION__));
>
> + }
>
> + Instance->Initialized = TRUE;
>
> +
>
> + return Status;
>
> +}
>
> +
>
> +/**
>
> + Since the RPMB is not byte addressable we need to allocate memory
>
> + and sync that on reads/writes. Initialize the memory and install the
>
> + Fvb protocol.
>
> +
>
> + @param ImageHandle The EFI image handle
>
> + @param SystemTable MM system table
>
> +
>
> + @retval EFI_SUCCESS Protocol installed
>
> +
>
> + @retval EFI_INVALID_PARAMETER Invalid Pcd values specified
>
> +
>
> + @retval EFI_OUT_OF_RESOURCES Can't allocate necessary memory
>
> +**/
>
> +EFI_STATUS
>
> +EFIAPI
>
> +OpTeeRpmbFvbInit (
>
> + IN EFI_HANDLE ImageHandle,
>
> + IN EFI_MM_SYSTEM_TABLE *SystemTable
>
> + )
>
> +{
>
> + EFI_STATUS Status;
>
> + VOID *Addr;
>
> + UINTN FvLength;
>
> + UINTN NBlocks;
>
> +
>
> + FvLength = PcdGet32(PcdFlashNvStorageVariableSize) +
>
> + PcdGet32(PcdFlashNvStorageFtwWorkingSize) +
>
> + PcdGet32(PcdFlashNvStorageFtwSpareSize);
>
> +
>
> + NBlocks = EFI_SIZE_TO_PAGES (ALIGN_VARIABLE (FvLength));
>
> + Addr = AllocatePages (NBlocks);
>
> + if (Addr == NULL) {
>
> + ASSERT (0);
>
> + return EFI_OUT_OF_RESOURCES;
>
> + }
>
> +
>
> + SetMem (&mInstance, sizeof (mInstance), 0);
NIT: you can use ZeroMem()
>
> +
>
> + mInstance.FvbProtocol.GetPhysicalAddress =
> OpTeeRpmbFvbGetPhysicalAddress;
>
> + mInstance.FvbProtocol.GetAttributes = OpTeeRpmbFvbGetAttributes;
>
> + mInstance.FvbProtocol.SetAttributes = OpTeeRpmbFvbSetAttributes;
>
> + mInstance.FvbProtocol.GetBlockSize = OpTeeRpmbFvbGetBlockSize;
>
> + mInstance.FvbProtocol.EraseBlocks = OpTeeRpmbFvbErase;
>
> + mInstance.FvbProtocol.Write = OpTeeRpmbFvbWrite;
>
> + mInstance.FvbProtocol.Read = OpTeeRpmbFvbRead;
>
> +
>
> + mInstance.MemBaseAddress = (EFI_PHYSICAL_ADDRESS)Addr;
>
> + mInstance.Signature = FLASH_SIGNATURE;
>
> + mInstance.Initialize = FvbInitialize;
>
> + mInstance.BlockSize = EFI_PAGE_SIZE;
>
> + mInstance.NBlocks = NBlocks;
>
> +
>
> + // The Pcd is user defined, so make sure we don't overflow
>
> + if (mInstance.MemBaseAddress > MAX_UINT64 - PcdGet32
> (PcdFlashNvStorageVariableSize)) {
>
I don't think this is necessary to do any check here since the memory
has just been allocated with the right size.
> + return EFI_INVALID_PARAMETER;
>
> + }
>
> +
>
> + if (mInstance.MemBaseAddress > MAX_UINT64 - PcdGet32
> (PcdFlashNvStorageVariableSize) -
>
> + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)) {
>
> + return EFI_INVALID_PARAMETER;
>
> + }
>
> +
>
> + // Update the defined PCDs related to Variable Storage
>
> + PatchPcdSet64 (PcdFlashNvStorageVariableBase64,
> mInstance.MemBaseAddress);
>
> + PatchPcdSet64 (
>
> + PcdFlashNvStorageFtwWorkingBase64,
>
> + mInstance.MemBaseAddress + PcdGet32 (PcdFlashNvStorageVariableSize)
>
> + );
>
> + PatchPcdSet64 (
>
> + PcdFlashNvStorageFtwSpareBase64,
>
> + mInstance.MemBaseAddress +
>
> + PcdGet32 (PcdFlashNvStorageVariableSize) +
>
> + PcdGet32 (PcdFlashNvStorageFtwWorkingSize)
>
> + );
Do we actually need to set these PCDs ? If the PCDs are persistent, we
should not need to patch them in FixupPcd.c. FvbInitialize() is using
them, but it is not called from OpTeeRpmbFvbInit().
>
> +
>
> + Status = gMmst->MmInstallProtocolInterface (
>
> + &mInstance.Handle,
>
> + &gEfiSmmFirmwareVolumeBlockProtocolGuid,
>
> + EFI_NATIVE_INTERFACE,
>
> + &mInstance.FvbProtocol
>
> + );
>
> + ASSERT_EFI_ERROR (Status);
>
> +
>
> + DEBUG ((DEBUG_INFO, "%a: Register OP-TEE RPMB Fvb\n", __FUNCTION__));
>
> + DEBUG ((DEBUG_INFO, "%a: Using NV store FV in-memory copy at 0x%lx\n",
>
> + __FUNCTION__, PatchPcdGet64 (PcdFlashNvStorageVariableBase64)));
>
> +
>
> + return Status;
>
> +}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72505): https://edk2.groups.io/g/devel/message/72505
Mute This Topic: https://groups.io/mt/80588994/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