process: add IPMI error return mechanism and update errors

The IPMI packet validation code must return specific IPMI errors
corresponding to what error has occurred instead of the invalid command
error.

Update all IPMI handler pieces to return more specific errors.

Change-Id: I8d21e92015d84cc0880e3b83991aed7288e19eab
Signed-off-by: Patrick Venture <venture@google.com>

14 files changed, 79 insertions, 40 deletions

diff --git a/ipmi.cpp b/ipmi.cpp
index fb5f120..e2e0fef 100644
--- a/ipmi.cpp
+++ b/ipmi.cpp
@@ -105,7 +105,7 @@ ipmi_ret_t enumerateBlob(ManagerInterface* mgr, const uint8_t* reqBuf,
     std::string blobId = mgr->getBlobId(request.blobIdx);
     if (blobId == "")
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_INVALID_FIELD_REQUEST;
     }
 
     /* TODO(venture): Need to do a hard-code check against the maximum
@@ -130,13 +130,13 @@ ipmi_ret_t openBlob(ManagerInterface* mgr, const uint8_t* reqBuf,
         request->blobId, (requestLen - sizeof(struct BmcBlobOpenTx)));
     if (path.empty())
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_REQ_DATA_LEN_INVALID;
     }
 
     /* Attempt to open. */
     if (!mgr->open(request->flags, path, &session))
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_UNSPECIFIED_ERROR;
     }
 
     struct BmcBlobOpenRx reply;
@@ -158,7 +158,7 @@ ipmi_ret_t closeBlob(ManagerInterface* mgr, const uint8_t* reqBuf,
     /* Attempt to close. */
     if (!mgr->close(request.sessionId))
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_UNSPECIFIED_ERROR;
     }
 
     (*dataLen) = 0;
@@ -175,13 +175,13 @@ ipmi_ret_t deleteBlob(ManagerInterface* mgr, const uint8_t* reqBuf,
         request->blobId, (requestLen - sizeof(struct BmcBlobDeleteTx)));
     if (path.empty())
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_REQ_DATA_LEN_INVALID;
     }
 
     /* Attempt to delete. */
     if (!mgr->deleteBlob(path))
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_UNSPECIFIED_ERROR;
     }
 
     (*dataLen) = 0;
@@ -220,14 +220,14 @@ ipmi_ret_t statBlob(ManagerInterface* mgr, const uint8_t* reqBuf,
         request->blobId, (requestLen - sizeof(struct BmcBlobStatTx)));
     if (path.empty())
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_REQ_DATA_LEN_INVALID;
     }
 
     /* Attempt to stat. */
     struct BlobMeta meta;
     if (!mgr->stat(path, &meta))
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_UNSPECIFIED_ERROR;
     }
 
     return returnStatBlob(&meta, replyCmdBuf, dataLen);
@@ -244,7 +244,7 @@ ipmi_ret_t sessionStatBlob(ManagerInterface* mgr, const uint8_t* reqBuf,
 
     if (!mgr->stat(request.sessionId, &meta))
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_UNSPECIFIED_ERROR;
     }
 
     return returnStatBlob(&meta, replyCmdBuf, dataLen);
@@ -259,7 +259,7 @@ ipmi_ret_t commitBlob(ManagerInterface* mgr, const uint8_t* reqBuf,
     /* Sanity check the commitDataLen */
     if (request->commitDataLen > (requestLen - sizeof(struct BmcBlobCommitTx)))
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_REQ_DATA_LEN_INVALID;
     }
 
     std::vector<uint8_t> data(request->commitDataLen);
@@ -267,7 +267,7 @@ ipmi_ret_t commitBlob(ManagerInterface* mgr, const uint8_t* reqBuf,
 
     if (!mgr->commit(request->sessionId, data))
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_UNSPECIFIED_ERROR;
     }
 
     (*dataLen) = 0;
@@ -317,7 +317,7 @@ ipmi_ret_t writeBlob(ManagerInterface* mgr, const uint8_t* reqBuf,
     /* Attempt to write the bytes. */
     if (!mgr->write(request->sessionId, request->offset, data))
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_UNSPECIFIED_ERROR;
     }
 
     return IPMI_CC_OK;
@@ -342,7 +342,7 @@ ipmi_ret_t writeMeta(ManagerInterface* mgr, const uint8_t* reqBuf,
     /* Attempt to write the bytes. */
     if (!mgr->writeMeta(request.sessionId, request.offset, data))
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_UNSPECIFIED_ERROR;
     }
 
     return IPMI_CC_OK;
diff --git a/main.cpp b/main.cpp
index dda9dec..4328567 100644
--- a/main.cpp
+++ b/main.cpp
@@ -47,15 +47,17 @@ static ipmi_ret_t handleBlobCommand(ipmi_cmd_t cmd, const uint8_t* reqBuf,
      */
     if ((*dataLen) < 1)
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_REQ_DATA_LEN_INVALID;
     }
 
+    /* on failure rc is set to the corresponding IPMI error. */
+    ipmi_ret_t rc = IPMI_CC_OK;
     Crc16 crc;
     IpmiBlobHandler command =
-        validateBlobCommand(&crc, reqBuf, replyCmdBuf, dataLen);
+        validateBlobCommand(&crc, reqBuf, replyCmdBuf, dataLen, &rc);
     if (command == nullptr)
     {
-        return IPMI_CC_INVALID;
+        return rc;
     }
 
     return processBlobCommand(command, getBlobManager(), &crc, reqBuf,
diff --git a/process.cpp b/process.cpp
index 245da69..7e6410d 100644
--- a/process.cpp
+++ b/process.cpp
@@ -48,7 +48,8 @@ static const std::unordered_map<BlobOEMCommands, IpmiBlobHandler> handlers = {
 };
 
 IpmiBlobHandler validateBlobCommand(CrcInterface* crc, const uint8_t* reqBuf,
-                                    uint8_t* replyCmdBuf, size_t* dataLen)
+                                    uint8_t* replyCmdBuf, size_t* dataLen,
+                                    ipmi_ret_t* code)
 {
     size_t requestLength = (*dataLen);
     /* We know dataLen is at least 1 already */
@@ -57,6 +58,7 @@ IpmiBlobHandler validateBlobCommand(CrcInterface* crc, const uint8_t* reqBuf,
     /* Validate it's at least well-formed. */
     if (!validateRequestLength(command, requestLength))
     {
+        *code = IPMI_CC_REQ_DATA_LEN_INVALID;
         return nullptr;
     }
 
@@ -66,6 +68,7 @@ IpmiBlobHandler validateBlobCommand(CrcInterface* crc, const uint8_t* reqBuf,
         /* Verify the request includes: command, crc16, data */
         if (requestLength < sizeof(struct BmcRx))
         {
+            *code = IPMI_CC_REQ_DATA_LEN_INVALID;
             return nullptr;
         }
 
@@ -91,6 +94,7 @@ IpmiBlobHandler validateBlobCommand(CrcInterface* crc, const uint8_t* reqBuf,
         /* Crc expected but didn't match. */
         if (crcValue != crc->get())
         {
+            *code = IPMI_CC_UNSPECIFIED_ERROR;
             return nullptr;
         }
     }
@@ -99,6 +103,7 @@ IpmiBlobHandler validateBlobCommand(CrcInterface* crc, const uint8_t* reqBuf,
     auto found = handlers.find(command);
     if (found == handlers.end())
     {
+        *code = IPMI_CC_INVALID_FIELD_REQUEST;
         return nullptr;
     }
 
@@ -126,7 +131,7 @@ ipmi_ret_t processBlobCommand(IpmiBlobHandler cmd, ManagerInterface* mgr,
     /* The response, if it has one byte, has three, to include the crc16. */
     if (replyLength < (sizeof(uint16_t) + 1))
     {
-        return IPMI_CC_INVALID;
+        return IPMI_CC_UNSPECIFIED_ERROR;
     }
 
     /* The command, whatever it was, replied, so let's set the CRC. */
diff --git a/process.hpp b/process.hpp
index 8f874dc..0e377bc 100644
--- a/process.hpp
+++ b/process.hpp
@@ -22,10 +22,12 @@ using IpmiBlobHandler =
  * @param[in,out] replyCmdBuf - a pointer to the ipmi reply packet buffer.
  * @param[in,out] dataLen - initially the request length, set to reply length
  *                          on return.
- * @return the ipmi command handler.
+ * @param[out] code - set to the IPMI error on failure, otherwise unset.
+ * @return the ipmi command handler, or nullptr on failure.
  */
 IpmiBlobHandler validateBlobCommand(CrcInterface* crc, const uint8_t* reqBuf,
-                                    uint8_t* replyCmdBuf, size_t* dataLen);
+                                    uint8_t* replyCmdBuf, size_t* dataLen,
+                                    ipmi_ret_t* code);
 
 /**
  * Call the IPMI command and process the result, including running the CRC
diff --git a/test/ipmi_close_unittest.cpp b/test/ipmi_close_unittest.cpp
index f315ea0..edfddca 100644
--- a/test/ipmi_close_unittest.cpp
+++ b/test/ipmi_close_unittest.cpp
@@ -38,7 +38,8 @@ TEST(BlobCloseTest, ManagerRejectsCloseReturnsFailure)
     std::memcpy(request, &req, sizeof(req));
 
     EXPECT_CALL(mgr, close(sessionId)).WillOnce(Return(false));
-    EXPECT_EQ(IPMI_CC_INVALID, closeBlob(&mgr, request, reply, &dataLen));
+    EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+              closeBlob(&mgr, request, reply, &dataLen));
 }
 
 TEST(BlobCloseTest, BlobClosedReturnsSuccess)
diff --git a/test/ipmi_commit_unittest.cpp b/test/ipmi_commit_unittest.cpp
index 34ff10d..30f78bf 100644
--- a/test/ipmi_commit_unittest.cpp
+++ b/test/ipmi_commit_unittest.cpp
@@ -35,7 +35,8 @@ TEST(BlobCommitTest, InvalidCommitDataLengthReturnsFailure)
 
     dataLen = sizeof(struct BmcBlobCommitTx);
 
-    EXPECT_EQ(IPMI_CC_INVALID, commitBlob(&mgr, request, reply, &dataLen));
+    EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID,
+              commitBlob(&mgr, request, reply, &dataLen));
 }
 
 TEST(BlobCommitTest, ValidCommitNoDataHandlerRejectsReturnsFailure)
@@ -57,7 +58,8 @@ TEST(BlobCommitTest, ValidCommitNoDataHandlerRejectsReturnsFailure)
 
     EXPECT_CALL(mgr, commit(req->sessionId, _)).WillOnce(Return(false));
 
-    EXPECT_EQ(IPMI_CC_INVALID, commitBlob(&mgr, request, reply, &dataLen));
+    EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+              commitBlob(&mgr, request, reply, &dataLen));
 }
 
 TEST(BlobCommitTest, ValidCommitNoDataHandlerAcceptsReturnsSuccess)
diff --git a/test/ipmi_delete_unittest.cpp b/test/ipmi_delete_unittest.cpp
index 409635d..6985eb1 100644
--- a/test/ipmi_delete_unittest.cpp
+++ b/test/ipmi_delete_unittest.cpp
@@ -36,7 +36,8 @@ TEST(BlobDeleteTest, InvalidRequestLengthReturnsFailure)
 
     dataLen = sizeof(struct BmcBlobDeleteTx) + blobId.length();
 
-    EXPECT_EQ(IPMI_CC_INVALID, deleteBlob(&mgr, request, reply, &dataLen));
+    EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID,
+              deleteBlob(&mgr, request, reply, &dataLen));
 }
 
 TEST(BlobDeleteTest, RequestRejectedReturnsFailure)
@@ -60,7 +61,8 @@ TEST(BlobDeleteTest, RequestRejectedReturnsFailure)
 
     EXPECT_CALL(mgr, deleteBlob(StrEq(blobId))).WillOnce(Return(false));
 
-    EXPECT_EQ(IPMI_CC_INVALID, deleteBlob(&mgr, request, reply, &dataLen));
+    EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+              deleteBlob(&mgr, request, reply, &dataLen));
 }
 
 TEST(BlobDeleteTest, BlobDeleteReturnsOk)
diff --git a/test/ipmi_enumerate_unittest.cpp b/test/ipmi_enumerate_unittest.cpp
index 041a8cb..184c686 100644
--- a/test/ipmi_enumerate_unittest.cpp
+++ b/test/ipmi_enumerate_unittest.cpp
@@ -31,7 +31,8 @@ TEST(BlobEnumerateTest, VerifyIfRequestByIdInvalidReturnsFailure)
 
     EXPECT_CALL(mgr, getBlobId(req.blobIdx)).WillOnce(Return(""));
 
-    EXPECT_EQ(IPMI_CC_INVALID, enumerateBlob(&mgr, request, reply, &dataLen));
+    EXPECT_EQ(IPMI_CC_INVALID_FIELD_REQUEST,
+              enumerateBlob(&mgr, request, reply, &dataLen));
 }
 
 TEST(BlobEnumerateTest, BoringRequestByIdAndReceive)
diff --git a/test/ipmi_open_unittest.cpp b/test/ipmi_open_unittest.cpp
index 523025d..704c144 100644
--- a/test/ipmi_open_unittest.cpp
+++ b/test/ipmi_open_unittest.cpp
@@ -39,7 +39,8 @@ TEST(BlobOpenTest, InvalidRequestLengthReturnsFailure)
 
     dataLen = sizeof(struct BmcBlobOpenTx) + blobId.length();
 
-    EXPECT_EQ(IPMI_CC_INVALID, openBlob(&mgr, request, reply, &dataLen));
+    EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID,
+              openBlob(&mgr, request, reply, &dataLen));
 }
 
 TEST(BlobOpenTest, RequestRejectedReturnsFailure)
@@ -65,7 +66,8 @@ TEST(BlobOpenTest, RequestRejectedReturnsFailure)
     EXPECT_CALL(mgr, open(req->flags, StrEq(blobId), _))
         .WillOnce(Return(false));
 
-    EXPECT_EQ(IPMI_CC_INVALID, openBlob(&mgr, request, reply, &dataLen));
+    EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+              openBlob(&mgr, request, reply, &dataLen));
 }
 
 TEST(BlobOpenTest, BlobOpenReturnsOk)
diff --git a/test/ipmi_sessionstat_unittest.cpp b/test/ipmi_sessionstat_unittest.cpp
index 70c36ec..a71252c 100644
--- a/test/ipmi_sessionstat_unittest.cpp
+++ b/test/ipmi_sessionstat_unittest.cpp
@@ -37,7 +37,8 @@ TEST(BlobSessionStatTest, RequestRejectedByManagerReturnsFailure)
                           Matcher<struct BlobMeta*>(_)))
         .WillOnce(Return(false));
 
-    EXPECT_EQ(IPMI_CC_INVALID, sessionStatBlob(&mgr, request, reply, &dataLen));
+    EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+              sessionStatBlob(&mgr, request, reply, &dataLen));
 }
 
 TEST(BlobSessionStatTest, RequestSucceedsNoMetadata)
diff --git a/test/ipmi_stat_unittest.cpp b/test/ipmi_stat_unittest.cpp
index 75f90c4..e7ff277 100644
--- a/test/ipmi_stat_unittest.cpp
+++ b/test/ipmi_stat_unittest.cpp
@@ -39,7 +39,8 @@ TEST(BlobStatTest, InvalidRequestLengthReturnsFailure)
 
     dataLen = sizeof(struct BmcBlobStatTx) + blobId.length();
 
-    EXPECT_EQ(IPMI_CC_INVALID, statBlob(&mgr, request, reply, &dataLen));
+    EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID,
+              statBlob(&mgr, request, reply, &dataLen));
 }
 
 TEST(BlobStatTest, RequestRejectedReturnsFailure)
@@ -65,7 +66,8 @@ TEST(BlobStatTest, RequestRejectedReturnsFailure)
                           Matcher<struct BlobMeta*>(_)))
         .WillOnce(Return(false));
 
-    EXPECT_EQ(IPMI_CC_INVALID, statBlob(&mgr, request, reply, &dataLen));
+    EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+              statBlob(&mgr, request, reply, &dataLen));
 }
 
 TEST(BlobStatTest, RequestSucceedsNoMetadata)
diff --git a/test/ipmi_write_unittest.cpp b/test/ipmi_write_unittest.cpp
index 7b5e45f..99615af 100644
--- a/test/ipmi_write_unittest.cpp
+++ b/test/ipmi_write_unittest.cpp
@@ -40,7 +40,8 @@ TEST(BlobWriteTest, ManagerReturnsFailureReturnsFailure)
                       ElementsAreArray(expectedBytes, sizeof(expectedBytes))))
         .WillOnce(Return(false));
 
-    EXPECT_EQ(IPMI_CC_INVALID, writeBlob(&mgr, request, reply, &dataLen));
+    EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+              writeBlob(&mgr, request, reply, &dataLen));
 }
 
 TEST(BlobWriteTest, ManagerReturnsTrueWriteSucceeds)
diff --git a/test/ipmi_writemeta_unittest.cpp b/test/ipmi_writemeta_unittest.cpp
index 2dfbe59..2d63452 100644
--- a/test/ipmi_writemeta_unittest.cpp
+++ b/test/ipmi_writemeta_unittest.cpp
@@ -39,7 +39,8 @@ TEST(BlobWriteMetaTest, ManagerReturnsFailureReturnsFailure)
                        ElementsAreArray(expectedBytes, sizeof(expectedBytes))))
         .WillOnce(Return(false));
 
-    EXPECT_EQ(IPMI_CC_INVALID, writeMeta(&mgr, request, reply, &dataLen));
+    EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+              writeMeta(&mgr, request, reply, &dataLen));
 }
 
 TEST(BlobWriteMetaTest, ManagerReturnsTrueWriteSucceeds)
diff --git a/test/process_unittest.cpp b/test/process_unittest.cpp
index 63e6f5e..4f8c9a6 100644
--- a/test/process_unittest.cpp
+++ b/test/process_unittest.cpp
@@ -57,8 +57,11 @@ TEST(ValidateBlobCommandTest, InvalidCommandReturnsFailure)
 
     request[0] = 0xff;         // There is no command 0xff.
     dataLen = sizeof(uint8_t); // There is no payload for CRC.
+    ipmi_ret_t rc;
 
-    EXPECT_EQ(nullptr, validateBlobCommand(&crc, request, reply, &dataLen));
+    EXPECT_EQ(nullptr,
+              validateBlobCommand(&crc, request, reply, &dataLen, &rc));
+    EXPECT_EQ(IPMI_CC_INVALID_FIELD_REQUEST, rc);
 }
 
 TEST(ValidateBlobCommandTest, ValidCommandWithoutPayload)
@@ -72,8 +75,10 @@ TEST(ValidateBlobCommandTest, ValidCommandWithoutPayload)
 
     request[0] = BlobOEMCommands::bmcBlobGetCount;
     dataLen = sizeof(uint8_t); // There is no payload for CRC.
+    ipmi_ret_t rc;
 
-    IpmiBlobHandler res = validateBlobCommand(&crc, request, reply, &dataLen);
+    IpmiBlobHandler res =
+        validateBlobCommand(&crc, request, reply, &dataLen, &rc);
     EXPECT_FALSE(res == nullptr);
     EqualFunctions(getBlobCount, res);
 }
@@ -91,8 +96,11 @@ TEST(ValidateBlobCommandTest, WithPayloadMinimumLengthIs3VerifyChecks)
     request[0] = BlobOEMCommands::bmcBlobGetCount;
     dataLen = sizeof(uint8_t) + sizeof(uint16_t);
     // There is a payload, but there are insufficient bytes.
+    ipmi_ret_t rc;
 
-    EXPECT_EQ(nullptr, validateBlobCommand(&crc, request, reply, &dataLen));
+    EXPECT_EQ(nullptr,
+              validateBlobCommand(&crc, request, reply, &dataLen, &rc));
+    EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID, rc);
 }
 
 TEST(ValidateBlobCommandTest, WithPayloadAndInvalidCrc)
@@ -125,7 +133,11 @@ TEST(ValidateBlobCommandTest, WithPayloadAndInvalidCrc)
         }));
     EXPECT_CALL(crc, get()).WillOnce(Return(0x1234));
 
-    EXPECT_EQ(nullptr, validateBlobCommand(&crc, request, reply, &dataLen));
+    ipmi_ret_t rc;
+
+    EXPECT_EQ(nullptr,
+              validateBlobCommand(&crc, request, reply, &dataLen, &rc));
+    EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR, rc);
 }
 
 TEST(ValidateBlobCommandTest, WithPayloadAndValidCrc)
@@ -158,7 +170,10 @@ TEST(ValidateBlobCommandTest, WithPayloadAndValidCrc)
         }));
     EXPECT_CALL(crc, get()).WillOnce(Return(0x3412));
 
-    IpmiBlobHandler res = validateBlobCommand(&crc, request, reply, &dataLen);
+    ipmi_ret_t rc;
+
+    IpmiBlobHandler res =
+        validateBlobCommand(&crc, request, reply, &dataLen, &rc);
     EXPECT_FALSE(res == nullptr);
     EqualFunctions(writeBlob, res);
 }
@@ -181,8 +196,10 @@ TEST(ValidateBlobCommandTest, InputIntegrationTest)
     uint8_t reply[MAX_IPMI_BUFFER] = {0};
 
     dataLen = sizeof(request);
+    ipmi_ret_t rc;
 
-    IpmiBlobHandler res = validateBlobCommand(&crc, request, reply, &dataLen);
+    IpmiBlobHandler res =
+        validateBlobCommand(&crc, request, reply, &dataLen, &rc);
     EXPECT_FALSE(res == nullptr);
     EqualFunctions(openBlob, res);
 }
@@ -250,7 +267,7 @@ TEST(ProcessBlobCommandTest, CommandReturnsOkWithInvalidPayloadLength)
 
     dataLen = sizeof(request);
 
-    EXPECT_EQ(IPMI_CC_INVALID,
+    EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
               processBlobCommand(h, &manager, &crc, request, reply, &dataLen));
 }