Add NULL Check to allow IpmiSendCommandInternal flow through avoindin…

…g assert as routine does not consider NULL para… (#243) ## Description IpmiSendCommandInternal did not allow NULL parameter for ResponseData and ResponseDataSize Parameters. Expected Behavior: NULL can be passed as parameter for some specific IPMI commands. FIX APPLIED: Allow IpmiSendCommandInternal to allow a NULL parameter as long as ResponseDataSize is zero. - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested Tested with Oem Specific Ipmi Command. Prior to change, cpu exception occurred. After fix, system was able to continue. ## Integration Instructions N/A --------- Co-authored-by: Aaron <[email protected]> Co-authored-by: Aaron Pop <[email protected]>
microsoft · Jul 26, 2024 · 253124f · 253124f
1 parent 0c52422
commit 253124f
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 29 deletions.
diff --git a/IpmiFeaturePkg/GenericIpmi/Common/GenericIpmi.c b/IpmiFeaturePkg/GenericIpmi/Common/GenericIpmi.c
@@ -110,8 +110,8 @@ IpmiPrintCommand (
   @param[in]      Command           IPMI command to send.
   @param[in]      CommandData       Pointer to command data buffer, if needed.
   @param[in]      CommandDataSize   Size of command data buffer.
-  @param[in,out]  ResponseData      Pointer to response data buffer.
-  @param[in,out]  ResponseDataSize  Pointer to response data buffer size.
+  @param[in,out]  ResponseData      Pointer to response data buffer. Optional depending on command being sent.
+  @param[in,out]  ResponseDataSize  Pointer to response data buffer size. Optional depending on command being sent.
 
   @retval   EFI_INVALID_PARAMETER   One of the input values is bad.
   @retval   EFI_DEVICE_ERROR        IPMI command failed.
@@ -127,8 +127,8 @@ IpmiSendCommandInternal (
   IN      UINT8           Command,
   IN      UINT8           *CommandData,
   IN      UINT8           CommandDataSize,
-  IN OUT  UINT8           *ResponseData,
-  IN OUT  UINT8           *ResponseDataSize
+  IN OUT  UINT8           *ResponseData OPTIONAL,
+  IN OUT  UINT8           *ResponseDataSize OPTIONAL
   )
 {
   IPMI_BMC_INSTANCE_DATA  *IpmiInstance;
@@ -143,6 +143,15 @@ IpmiSendCommandInternal (
     return EFI_INVALID_PARAMETER;
   }
 
+  //
+  // ResponseData and ResponseDataSize cannot be mismatched.
+  // Either both NULL are NULL or both are Non-Null, depending
+  // on the command being sent.
+  //
+  if ((ResponseData != NULL) != (ResponseDataSize != NULL)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   RetryCnt     = PcdGet8 (PcdIpmiCommandMaxReties);
   IpmiInstance = INSTANCE_FROM_SM_IPMI_BMC_THIS (This);
 
@@ -277,7 +286,7 @@ IpmiSendCommandInternal (
     //
     // Verify the response data buffer passed in is big enough.
     //
-    if ((DataSize - IPMI_RESPONSE_HEADER_SIZE) > *ResponseDataSize) {
+    if ((ResponseDataSize != NULL) && ((DataSize - IPMI_RESPONSE_HEADER_SIZE) > *ResponseDataSize)) {
       //
       // Verify the response data matched with the cmd sent.
       //
@@ -301,25 +310,27 @@ IpmiSendCommandInternal (
     break;
   }
 
-  //
-  // Copy data over to the response data buffer.
-  //
-  *ResponseDataSize = DataSize - IPMI_RESPONSE_HEADER_SIZE;
-  CopyMem (
-    ResponseData,
-    IpmiResponse->ResponseData,
-    *ResponseDataSize
-    );
+  if ((ResponseData != NULL) && (ResponseDataSize != NULL)) {
+    //
+    // Copy data over to the response data buffer.
+    //
+    *ResponseDataSize = DataSize - IPMI_RESPONSE_HEADER_SIZE;
+    CopyMem (
+      ResponseData,
+      IpmiResponse->ResponseData,
+      *ResponseDataSize
+      );
 
-  //
-  // Add completion code in response data to meet the requirement of IPMI spec 2.0
-  //
-  *ResponseDataSize += 1; // Add one byte for Completion Code
-  for (Index = 1; Index < *ResponseDataSize; Index++) {
-    ResponseData[*ResponseDataSize - Index] = ResponseData[*ResponseDataSize - (Index + 1)];
-  }
+    //
+    // Add completion code in response data to meet the requirement of IPMI spec 2.0
+    //
+    *ResponseDataSize += 1;   // Add one byte for Completion Code
+    for (Index = 1; Index < *ResponseDataSize; Index++) {
+      ResponseData[*ResponseDataSize - Index] = ResponseData[*ResponseDataSize - (Index + 1)];
+    }
 
-  ResponseData[0] = IpmiResponse->CompletionCode;
+    ResponseData[0] = IpmiResponse->CompletionCode;
+  }
 
   IpmiInstance->BmcStatus = BMC_OK;
   return EFI_SUCCESS;

diff --git a/IpmiFeaturePkg/GenericIpmi/Common/GenericIpmi.h b/IpmiFeaturePkg/GenericIpmi/Common/GenericIpmi.h
@@ -111,8 +111,8 @@ IpmiInitializeBmc (
   @param[in]      Command           IPMI command to send.
   @param[in]      CommandData       Pointer to command data buffer, if needed.
   @param[in]      CommandDataSize   Size of command data buffer.
-  @param[in,out]  ResponseData      Pointer to response data buffer.
-  @param[in,out]  ResponseDataSize  Pointer to response data buffer size.
+  @param[in,out]  ResponseData      Pointer to response data buffer. Optional depending on command being sent.
+  @param[in,out]  ResponseDataSize  Pointer to response data buffer size. Optional depending on command being sent.
 
   @retval   EFI_INVALID_PARAMETER   One of the input values is bad.
   @retval   EFI_DEVICE_ERROR        IPMI command failed.
@@ -128,8 +128,8 @@ IpmiSendCommandInternal (
   IN      UINT8           Command,
   IN      UINT8           *CommandData,
   IN      UINT8           CommandDataSize,
-  IN OUT  UINT8           *ResponseData,
-  IN OUT  UINT8           *ResponseDataSize
+  IN OUT  UINT8           *ResponseData OPTIONAL,
+  IN OUT  UINT8           *ResponseDataSize OPTIONAL
   );
 
 /**

diff --git a/IpmiFeaturePkg/GenericIpmi/Test/GenericIpmiUnitTest.c b/IpmiFeaturePkg/GenericIpmi/Test/GenericIpmiUnitTest.c
@@ -164,10 +164,11 @@ TestIpmiBufferTooSmall (
   UINT32                       DataSize;
 
   //
-  // Test a 0 length buffer.
+  // Test a 0 length buffer while passing a Non-Null Response Buffer
   //
 
   DataSize = 0;
+  ZeroMem (&Response, sizeof (Response));
 
   Status = mIpmiInstance.IpmiTransport.IpmiSubmitCommand (
                                          &mIpmiInstance.IpmiTransport,
@@ -176,12 +177,30 @@ TestIpmiBufferTooSmall (
                                          IPMI_APP_GET_DEVICE_ID,
                                          NULL,
                                          0,
-                                         NULL,
+                                         (UINT8 *)&Response,
                                          &DataSize
                                          );
 
   UT_ASSERT_STATUS_EQUAL (Status, EFI_BUFFER_TOO_SMALL);
-  UT_ASSERT_EQUAL (DataSize, sizeof (IPMI_GET_DEVICE_ID_RESPONSE));
+
+  //
+  // Test a 4 byte length buffer while passing NULL
+  //
+
+  DataSize = 4;
+
+  Status = mIpmiInstance.IpmiTransport.IpmiSubmitCommand (
+                                         &mIpmiInstance.IpmiTransport,
+                                         IPMI_NETFN_APP,
+                                         0,
+                                         IPMI_APP_GET_DEVICE_ID,
+                                         NULL,
+                                         0,
+                                         NULL,
+                                         &DataSize
+                                         );
+
+  UT_ASSERT_STATUS_EQUAL (Status, EFI_INVALID_PARAMETER);
 
   //
   // Test an inadequate buffer size.