Re: [EXTERNAL] [edk2-devel] [PATCH v1 1/1] SecurityPkg/Library: Add Tpm2NvUndefineSpaceSpecial to Tpm2CommandLib

["Yao, Jiewen" <jiewen.yao@intel.com>]

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

Hi Bret
Since it took much long time to get ECC feedback than I expected, I would give feedback on code while we are waiting.

1) Please confirm how you test the code, such as Microsoft platform ?
2) Please remove “+} // Tpm2NvUndefineSpaceSpecial()” at the end of the function. We do not use that style in other code.
3) Please copy the definition from TPM spec “This command allows removal of a platform-created NV Index that has TPMA_NV_POLICY_DELETE SET” to the function header description. The current one “This command removes an index from the TPM.” is for TPM2_NV_UndefineSpace instead of TPM2_NV_UndefineSpaceSpecial.

Since above comment does not impact any function, I would like to give RB.

With about change, reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

Thank you
Yao Jiewen


From: Bret Barkelew via groups.io<mailto:bret.barkelew=microsoft.com@groups.io>
Sent: Friday, July 30, 2021 11:08 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; bret@corthon.com<mailto:bret@corthon.com>
Cc: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Jian J Wang<mailto:jian.j.wang@intel.com>; Qi Zhang<mailto:qi1.zhang@intel.com>; Rahul Kumar<mailto:rahul1.kumar@intel.com>
Subject: Re: [EXTERNAL] [edk2-devel] [PATCH v1 1/1] SecurityPkg/Library: Add Tpm2NvUndefineSpaceSpecial to Tpm2CommandLib



Note, even though this keeps with the style of the rest of the file, it breaks ECC:

SecurityPkg/Library: Add Tpm2NvUndefineSpaceSpecial to Tpm2CommandLib by corthon · Pull Request #1848 · tianocore/edk2 (github.com)<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Fpull%2F1848&data=04%7C01%7CBret.Barkelew%40microsoft.com%7Ce90f066a5a8c4feae73108d95dee3ec3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637644100117879377%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=7uX%2FX1sJDbWsxeqYtqfQIFXBbDRVnii7kcJ1nri65T4%3D&reserved=0>



PROGRESS - --Running SecurityPkg: EccCheck Test NO-TARGET --

ERROR -

ERROR -

ERROR - EFI coding style error

ERROR - *Error code: 8001

ERROR - *Only capital letters are allowed to be used for #define declarations

ERROR - *file: //home/vsts/work/1/s/SecurityPkg/Library/Tpm2CommandLib/Tpm2NVStorage.c

ERROR - *Line number: 27

ERROR - *The #define name [RC_NV_UndefineSpaceSpecial_nvIndex] does no



Thoughts?



- Bret



From: Bret Barkelew via groups.io<mailto:bret=corthon.com@groups.io>
Sent: Friday, July 30, 2021 10:55 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Jian J Wang<mailto:jian.j.wang@intel.com>; Qi Zhang<mailto:qi1.zhang@intel.com>; Rahul Kumar<mailto:rahul1.kumar@intel.com>
Subject: [EXTERNAL] [edk2-devel] [PATCH v1 1/1] SecurityPkg/Library: Add Tpm2NvUndefineSpaceSpecial to Tpm2CommandLib



Used to provision and maintain certain HW-defined NV spaces.

REF: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2994&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7Cb7ae3c62047c48fc85d908d953833ca0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632645397602953%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=s96M3RvxMOY831Vfr1nt%2Fz1h3cyb6jU9eFzvjKO7Dtc%3D&amp;reserved=0<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2994&data=04%7C01%7CBret.Barkelew%40microsoft.com%7Ce90f066a5a8c4feae73108d95dee3ec3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637644100117889332%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=padkn1C%2BUQzKQSPo5gyurkMYW5ihwyf2Wm2mp2lrRKg%3D&reserved=0>

Signed-off-by: Bret Barkelew <bret.barkelew@microsoft.com<mailto:bret.barkelew@microsoft.com>>
Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
Cc: Qi Zhang <qi1.zhang@intel.com<mailto:qi1.zhang@intel.com>>
Cc: Rahul Kumar <rahul1.kumar@intel.com<mailto:rahul1.kumar@intel.com>>
---
 SecurityPkg/Library/Tpm2CommandLib/Tpm2NVStorage.c | 122 ++++++++++++++++++++
 SecurityPkg/Include/Library/Tpm2CommandLib.h       |  22 ++++
 2 files changed, 144 insertions(+)

diff --git a/SecurityPkg/Library/Tpm2CommandLib/Tpm2NVStorage.c b/SecurityPkg/Library/Tpm2CommandLib/Tpm2NVStorage.c
index 87572de20164..7931fade9190 100644
--- a/SecurityPkg/Library/Tpm2CommandLib/Tpm2NVStorage.c
+++ b/SecurityPkg/Library/Tpm2CommandLib/Tpm2NVStorage.c
@@ -24,6 +24,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #define RC_NV_UndefineSpace_authHandle      (TPM_RC_H + TPM_RC_1)

 #define RC_NV_UndefineSpace_nvIndex         (TPM_RC_H + TPM_RC_2)



+#define RC_NV_UndefineSpaceSpecial_nvIndex  (TPM_RC_H + TPM_RC_1)

+

 #define RC_NV_Read_authHandle               (TPM_RC_H + TPM_RC_1)

 #define RC_NV_Read_nvIndex                  (TPM_RC_H + TPM_RC_2)

 #define RC_NV_Read_size                     (TPM_RC_P + TPM_RC_1)

@@ -74,6 +76,20 @@ typedef struct {
   TPMS_AUTH_RESPONSE         AuthSession;

 } TPM2_NV_UNDEFINESPACE_RESPONSE;



+typedef struct {

+  TPM2_COMMAND_HEADER       Header;

+  TPMI_RH_NV_INDEX          NvIndex;

+  TPMI_RH_PLATFORM          Platform;

+  UINT32                    AuthSessionSize;

+  TPMS_AUTH_COMMAND         AuthSession;

+} TPM2_NV_UNDEFINESPACESPECIAL_COMMAND;

+

+typedef struct {

+  TPM2_RESPONSE_HEADER       Header;

+  UINT32                     AuthSessionSize;

+  TPMS_AUTH_RESPONSE         AuthSession;

+} TPM2_NV_UNDEFINESPACESPECIAL_RESPONSE;

+

 typedef struct {

   TPM2_COMMAND_HEADER       Header;

   TPMI_RH_NV_AUTH           AuthHandle;

@@ -506,6 +522,112 @@ Done:
   return Status;

 }



+/**

+  This command removes an index from the TPM.

+

+  @param[in]  NvIndex             The NV Index.

+  @param[in]  IndexAuthSession    Auth session context for the Index auth/policy

+  @param[in]  PlatAuthSession     Auth session context for the Platform auth/policy

+

+  @retval EFI_SUCCESS             Operation completed successfully.

+  @retval EFI_NOT_FOUND           The command was returned successfully, but NvIndex is not found.

+  @retval EFI_UNSUPPORTED         Selected NvIndex does not support deletion through this call.

+  @retval EFI_SECURITY_VIOLATION  Deletion is not authorized by current policy session.

+  @retval EFI_INVALID_PARAMETER   The command was unsuccessful.

+  @retval EFI_DEVICE_ERROR        The command was unsuccessful.

+**/

+EFI_STATUS

+EFIAPI

+Tpm2NvUndefineSpaceSpecial (

+  IN      TPMI_RH_NV_INDEX          NvIndex,

+  IN      TPMS_AUTH_COMMAND         *IndexAuthSession OPTIONAL,

+  IN      TPMS_AUTH_COMMAND         *PlatAuthSession OPTIONAL

+  )

+{

+  EFI_STATUS                              Status;

+  TPM2_NV_UNDEFINESPACESPECIAL_COMMAND    SendBuffer;

+  TPM2_NV_UNDEFINESPACESPECIAL_RESPONSE   RecvBuffer;

+  UINT32                                  SendBufferSize;

+  UINT32                                  RecvBufferSize;

+  UINT8                                   *Buffer;

+  UINT32                                  IndexAuthSize, PlatAuthSize;

+  TPM_RC                                  ResponseCode;

+

+  //

+  // Construct command

+  //

+  SendBuffer.Header.tag = SwapBytes16(TPM_ST_SESSIONS);

+  SendBuffer.Header.commandCode = SwapBytes32(TPM_CC_NV_UndefineSpaceSpecial);

+

+  SendBuffer.NvIndex = SwapBytes32 (NvIndex);

+  SendBuffer.Platform = SwapBytes32 (TPM_RH_PLATFORM);

+

+  //

+  // Marshall the Auth Sessions for the two handles.

+  Buffer = (UINT8 *)&SendBuffer.AuthSession;

+  // IndexAuthSession

+  IndexAuthSize = CopyAuthSessionCommand (IndexAuthSession, Buffer);

+  Buffer += IndexAuthSize;

+  // PlatAuthSession

+  PlatAuthSize = CopyAuthSessionCommand (PlatAuthSession, Buffer);

+  Buffer += PlatAuthSize;

+  // AuthSessionSize

+  SendBuffer.AuthSessionSize = SwapBytes32(IndexAuthSize + PlatAuthSize);

+

+  // Update total command size.

+  SendBufferSize = (UINT32)(Buffer - (UINT8 *)&SendBuffer);

+  SendBuffer.Header.paramSize = SwapBytes32 (SendBufferSize);

+

+  //

+  // send Tpm command

+  //

+  RecvBufferSize = sizeof (RecvBuffer);

+  Status = Tpm2SubmitCommand (SendBufferSize, (UINT8 *)&SendBuffer, &RecvBufferSize, (UINT8 *)&RecvBuffer);

+  if (EFI_ERROR (Status)) {

+    goto Done;

+  }

+

+  if (RecvBufferSize < sizeof (TPM2_RESPONSE_HEADER)) {

+    DEBUG ((EFI_D_ERROR, "Tpm2NvUndefineSpaceSpecial - RecvBufferSize Error - %x\n", RecvBufferSize));

+    Status = EFI_DEVICE_ERROR;

+    goto Done;

+  }

+

+  ResponseCode = SwapBytes32(RecvBuffer.Header.responseCode);

+  if (ResponseCode != TPM_RC_SUCCESS) {

+    DEBUG ((EFI_D_ERROR, "Tpm2NvUndefineSpaceSpecial - responseCode - %x\n", SwapBytes32(RecvBuffer.Header.responseCode)));

+  }

+  switch (ResponseCode) {

+  case TPM_RC_SUCCESS:

+    // return data

+    break;

+  case TPM_RC_ATTRIBUTES:

+  case TPM_RC_ATTRIBUTES + RC_NV_UndefineSpaceSpecial_nvIndex:

+    Status = EFI_UNSUPPORTED;

+    break;

+  case TPM_RC_NV_AUTHORIZATION:

+    Status = EFI_SECURITY_VIOLATION;

+    break;

+  case TPM_RC_HANDLE + RC_NV_UndefineSpaceSpecial_nvIndex: // TPM_RC_NV_DEFINED:

+    Status = EFI_NOT_FOUND;

+    break;

+  case TPM_RC_VALUE + RC_NV_UndefineSpace_nvIndex:

+    Status = EFI_INVALID_PARAMETER;

+    break;

+  default:

+    Status = EFI_DEVICE_ERROR;

+    break;

+  }

+

+Done:

+  //

+  // Clear AuthSession Content

+  //

+  ZeroMem (&SendBuffer, sizeof(SendBuffer));

+  ZeroMem (&RecvBuffer, sizeof(RecvBuffer));

+  return Status;

+} // Tpm2NvUndefineSpaceSpecial()

+

 /**

   This command reads a value from an area in NV memory previously defined by TPM2_NV_DefineSpace().



diff --git a/SecurityPkg/Include/Library/Tpm2CommandLib.h b/SecurityPkg/Include/Library/Tpm2CommandLib.h
index ee8eb622951c..8d7b4998d98d 100644
--- a/SecurityPkg/Include/Library/Tpm2CommandLib.h
+++ b/SecurityPkg/Include/Library/Tpm2CommandLib.h
@@ -364,6 +364,28 @@ Tpm2NvUndefineSpace (
   IN      TPMS_AUTH_COMMAND         *AuthSession OPTIONAL

   );



+/**

+  This command removes an index from the TPM.

+

+  @param[in]  NvIndex             The NV Index.

+  @param[in]  IndexAuthSession    Auth session context for the Index auth/policy

+  @param[in]  PlatAuthSession     Auth session context for the Platform auth/policy

+

+  @retval EFI_SUCCESS             Operation completed successfully.

+  @retval EFI_NOT_FOUND           The command was returned successfully, but NvIndex is not found.

+  @retval EFI_UNSUPPORTED         Selected NvIndex does not support deletion through this call.

+  @retval EFI_SECURITY_VIOLATION  Deletion is not authorized by current policy session.

+  @retval EFI_INVALID_PARAMETER   The command was unsuccessful.

+  @retval EFI_DEVICE_ERROR        The command was unsuccessful.

+**/

+EFI_STATUS

+EFIAPI

+Tpm2NvUndefineSpaceSpecial (

+  IN      TPMI_RH_NV_INDEX          NvIndex,

+  IN      TPMS_AUTH_COMMAND         *IndexAuthSession OPTIONAL,

+  IN      TPMS_AUTH_COMMAND         *PlatAuthSession OPTIONAL

+  );

+

 /**

   This command reads a value from an area in NV memory previously defined by TPM2_NV_DefineSpace().



--
2.31.1.windows.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78450): https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F78450&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7Cb7ae3c62047c48fc85d908d953833ca0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632645397612922%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=CWxLwgp73z2XQEa%2FN77gsCwRF73xha0RZCKwcFTlrRE%3D&amp;reserved=0<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F78450&data=04%7C01%7CBret.Barkelew%40microsoft.com%7Ce90f066a5a8c4feae73108d95dee3ec3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637644100117889332%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5xF1G1wVYzxVg8d6jArEFdAZnDdNqdWIAOvw8FJV07M%3D&reserved=0>
Mute This Topic: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F84555713%2F1822150&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7Cb7ae3c62047c48fc85d908d953833ca0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632645397612922%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=IWQ6E4yP0ECt3oYLYQa%2BnddGfcQEDMgfASlcxRuda%2BQ%3D&amp;reserved=0<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F84555713%2F1822150&data=04%7C01%7CBret.Barkelew%40microsoft.com%7Ce90f066a5a8c4feae73108d95dee3ec3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637644100117899289%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=2FYy4OplAG2hC8gFxfV3zySEX4SPryroOsIJ9BpQ8v0%3D&reserved=0>
Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io>
Unsubscribe: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7Cb7ae3c62047c48fc85d908d953833ca0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637632645397612922%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=qor4Y5FZEH8ch0AEmWDbe97FIQk4V1qx7IURcTHzjAU%3D&amp;reserved=0<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&data=04%7C01%7CBret.Barkelew%40microsoft.com%7Ce90f066a5a8c4feae73108d95dee3ec3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637644100117909246%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=uaOZ7%2FL3FmNlZ%2Fj0jKkSVLWQElFkWKNORPvvZW%2Bu8AM%3D&reserved=0> [brbarkel@microsoft.com]
-=-=-=-=-=-=







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