Add ipmi response buffer size check for all commands
Add a response size check for all Blob command using the max transfer
buffer size for the default ipmi channel.
The max size is fetched at the beginning and assumed that it won't
change. This is done instead of fetching it for all request to limit the
amount of refactoring needed to make it work with existing codebase.
Change-Id: I66c506cab9c92a120e9fc75da4b6b74d5677120b
Signed-off-by: Willy Tu <wltu@google.com>
diff --git a/ipmi.cpp b/ipmi.cpp
index 07f6cd8..d5954f2 100644
--- a/ipmi.cpp
+++ b/ipmi.cpp
@@ -109,8 +109,6 @@
return IPMI_CC_INVALID_FIELD_REQUEST;
}
- /* TODO(venture): Need to do a hard-code check against the maximum
- * reply buffer size. */
reply->crc = 0;
/* Explicilty copies the NUL-terminator. */
std::memcpy(reply + 1, blobId.c_str(), blobId.length() + 1);
@@ -287,9 +285,6 @@
struct BmcBlobReadTx request;
std::memcpy(&request, reqBuf, sizeof(request));
- /* TODO(venture): Verify requestedSize can fit in a returned IPMI packet.
- */
-
std::vector<uint8_t> result =
mgr->read(request.sessionId, request.offset, request.requestedSize);
diff --git a/main.cpp b/main.cpp
index e90969b..63701c8 100644
--- a/main.cpp
+++ b/main.cpp
@@ -24,11 +24,13 @@
#include <ipmid/api.h>
#include <cstdio>
+#include <functional>
#include <ipmid/iana.hpp>
#include <ipmid/oemopenbmc.hpp>
#include <ipmid/oemrouter.hpp>
#include <memory>
#include <phosphor-logging/log.hpp>
+#include <user_channel/channel_layer.hpp>
namespace blobs
{
@@ -44,8 +46,13 @@
"Registering OEM:[%#08X], Cmd:[%#04X] for Blob Commands\n",
oem::obmcOemNumber, oem::Cmd::blobTransferCmd);
- oemRouter->registerHandler(oem::obmcOemNumber, oem::Cmd::blobTransferCmd,
- handleBlobCommand);
+ oemRouter->registerHandler(
+ oem::obmcOemNumber, oem::Cmd::blobTransferCmd,
+ std::bind_front(handleBlobCommand,
+ // Get current IPMI channel and get the max transfer
+ // size (assuming that it
+ // does not change).
+ ipmi::getChannelMaxTransferSize(ipmi::currentChNum)));
/* Install handlers. */
try
diff --git a/meson.build b/meson.build
index 1c5808d..21d113c 100644
--- a/meson.build
+++ b/meson.build
@@ -21,7 +21,11 @@
description: 'Phosphor Blob Transfer Interface',
version: meson.project_version())
+cpp = meson.get_compiler('cpp')
+
phosphor_logging_dep = dependency('phosphor-logging')
+ipmid_dep = dependency('libipmid')
+channellayer_dep = cpp.find_library('channellayer', required: true)
blob_manager_pre = declare_dependency(
dependencies: [
@@ -60,7 +64,8 @@
implicit_include_directories: false,
dependencies: [
blob_manager_dep,
- dependency('libipmid'),
+ ipmid_dep,
+ channellayer_dep,
],
install: true,
install_dir: get_option('libdir') / 'ipmid-providers')
diff --git a/process.cpp b/process.cpp
index 2481fa2..cdf73fa 100644
--- a/process.cpp
+++ b/process.cpp
@@ -110,7 +110,7 @@
ipmi_ret_t processBlobCommand(IpmiBlobHandler cmd, ManagerInterface* mgr,
const uint8_t* reqBuf, uint8_t* replyCmdBuf,
- size_t* dataLen)
+ size_t* dataLen, size_t maxSize)
{
ipmi_ret_t result = cmd(mgr, reqBuf, replyCmdBuf, dataLen);
if (result != IPMI_CC_OK)
@@ -134,6 +134,12 @@
return IPMI_CC_UNSPECIFIED_ERROR;
}
+ /* Make sure the reply size fits the ipmi buffer */
+ if (replyLength > maxSize)
+ {
+ return IPMI_CC_RESPONSE_ERROR;
+ }
+
/* The command, whatever it was, replied, so let's set the CRC. */
std::vector<std::uint8_t> crcBuffer(replyCmdBuf + sizeof(uint16_t),
replyCmdBuf + replyLength);
@@ -144,7 +150,7 @@
return result;
}
-ipmi_ret_t handleBlobCommand(ipmi_cmd_t, const uint8_t* reqBuf,
+ipmi_ret_t handleBlobCommand(size_t maxSize, ipmi_cmd_t, const uint8_t* reqBuf,
uint8_t* replyCmdBuf, size_t* dataLen)
{
/* It's holding at least a sub-command. The OEN is trimmed from the bytes
@@ -166,7 +172,7 @@
}
return processBlobCommand(command, getBlobManager(), reqBuf, replyCmdBuf,
- dataLen);
+ dataLen, maxSize);
}
} // namespace blobs
diff --git a/process.hpp b/process.hpp
index da0031c..438f828 100644
--- a/process.hpp
+++ b/process.hpp
@@ -36,16 +36,18 @@
* @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.
+ * @param[in,out] maxSize - Maximum ipmi reply size
* @return the ipmi command result.
*/
ipmi_ret_t processBlobCommand(IpmiBlobHandler cmd, ManagerInterface* mgr,
const uint8_t* reqBuf, uint8_t* replyCmdBuf,
- size_t* dataLen);
+ size_t* dataLen, size_t maxSize);
/**
* Given an IPMI command, request buffer, and reply buffer, validate the request
* and call processBlobCommand.
*/
-ipmi_ret_t handleBlobCommand(ipmi_cmd_t cmd, const uint8_t* reqBuf,
- uint8_t* replyCmdBuf, size_t* dataLen);
+ipmi_ret_t handleBlobCommand(size_t maxSize, ipmi_cmd_t cmd,
+ const uint8_t* reqBuf, uint8_t* replyCmdBuf,
+ size_t* dataLen);
} // namespace blobs
diff --git a/test/process_unittest.cpp b/test/process_unittest.cpp
index b72fe9a..dce8042 100644
--- a/test/process_unittest.cpp
+++ b/test/process_unittest.cpp
@@ -200,8 +200,8 @@
dataLen = sizeof(request);
- EXPECT_EQ(IPMI_CC_INVALID,
- processBlobCommand(h, &manager, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_INVALID, processBlobCommand(h, &manager, request, reply,
+ &dataLen, MAX_IPMI_BUFFER));
}
TEST_F(ProcessBlobCommandTest, CommandReturnsOkWithNoPayload)
@@ -222,8 +222,8 @@
dataLen = sizeof(request);
- EXPECT_EQ(IPMI_CC_OK,
- processBlobCommand(h, &manager, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_OK, processBlobCommand(h, &manager, request, reply,
+ &dataLen, MAX_IPMI_BUFFER));
}
TEST_F(ProcessBlobCommandTest, CommandReturnsOkWithInvalidPayloadLength)
@@ -245,7 +245,8 @@
dataLen = sizeof(request);
EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
- processBlobCommand(h, &manager, request, reply, &dataLen));
+ processBlobCommand(h, &manager, request, reply, &dataLen,
+ MAX_IPMI_BUFFER));
}
TEST_F(ProcessBlobCommandTest, CommandReturnsOkWithValidPayloadLength)
@@ -270,11 +271,36 @@
EXPECT_CALL(crcMock, generateCrc(_)).WillOnce(Return(0x3412));
- EXPECT_EQ(IPMI_CC_OK,
- processBlobCommand(h, &manager, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_OK, processBlobCommand(h, &manager, request, reply,
+ &dataLen, MAX_IPMI_BUFFER));
EXPECT_EQ(dataLen, payloadLen);
uint8_t expectedBytes[3] = {0x12, 0x34, 0x56};
EXPECT_EQ(0, std::memcmp(expectedBytes, reply, sizeof(expectedBytes)));
}
+
+TEST_F(ProcessBlobCommandTest,
+ CommandReturnsErrorWithReplyExceededMaxTransferSize)
+{
+ // There is a minimum payload length of 3 bytes, this command returns a
+ // payload of 3 bytes and the crc code is called to process the payload.
+
+ StrictMock<ManagerMock> manager;
+ size_t dataLen;
+ uint8_t request[MAX_IPMI_BUFFER] = {0};
+ uint8_t reply[MAX_IPMI_BUFFER] = {0};
+ uint32_t payloadLen = sizeof(uint16_t) + sizeof(uint8_t);
+
+ IpmiBlobHandler h = [payloadLen](ManagerInterface*, const uint8_t*,
+ uint8_t* replyCmdBuf, size_t* dataLen) {
+ (*dataLen) = payloadLen;
+ replyCmdBuf[2] = 0x56;
+ return IPMI_CC_OK;
+ };
+
+ dataLen = sizeof(request);
+
+ EXPECT_EQ(IPMI_CC_RESPONSE_ERROR,
+ processBlobCommand(h, &manager, request, reply, &dataLen, 0));
+}
} // namespace blobs