Recover Add ipmi response buffer size check for all commands
Use the ipmi::Context channel to get the max transfer size to make sure
that we select the right size.
Also remove maximumReadSize since the max reply size is checked instead
We already check the max reply size so maximumReadSize is not needed
anymore. This will result in a bit of a different behavior when the size
exceed the max. It will not fail - instead, it will return up to the
max size.
Tested:
Work with ipmi config that doesn't support the default interface (13).
```
cat /usr/share/ipmi-providers/channel_config.json
{
"0": {
"name": "usb0",
"is_valid": true,
"active_sessions": 0,
"channel_info": {
"medium_type": "lan-802.3",
"protocol_type": "ipmb-1.0",
"session_supported": "multi-session",
"is_ipmi": true
}
},
"8": {
"name": "INTRABMC",
"is_valid": true,
"active_sessions": 0,
"channel_info": {
"medium_type": "oem",
"protocol_type": "oem",
"session_supported": "session-less",
"is_ipmi": true
}
},
"11": {
"name": "gbmcbr",
"is_valid": true,
"active_sessions": 0,
"channel_info": {
"medium_type": "lan-802.3",
"protocol_type": "ipmb-1.0",
"session_supported": "multi-session",
"is_ipmi": true
}
}
}
```
This read the blob count and always works now since it uses the right
interface to get the max transfer size.
```
ipmitool raw 46 128 207 194 0 0
cf c2 00 4a ac 0e 00 00 00
```
Change-Id: Ia65f80719a819e7d642eb7425463a56c179935ac
Signed-off-by: Willy Tu <wltu@google.com>
diff --git a/ipmi.cpp b/ipmi.cpp
index 9fd1491..5718e3c 100644
--- a/ipmi.cpp
+++ b/ipmi.cpp
@@ -256,9 +256,6 @@
}
std::memcpy(&request, data.data(), 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 d3179f3..cfa0de9 100644
--- a/main.cpp
+++ b/main.cpp
@@ -32,6 +32,7 @@
#include <memory>
#include <phosphor-logging/log.hpp>
#include <span>
+#include <user_channel/channel_layer.hpp>
namespace blobs
{
@@ -49,8 +50,12 @@
ipmi::registerOemHandler(
ipmi::prioOemBase, oem::obmcOemNumber, oem::Cmd::blobTransferCmd,
::ipmi::Privilege::User,
- [](ipmi::Context::ptr, uint8_t cmd, const std::vector<uint8_t>& data) {
- return handleBlobCommand(cmd, data);
+ [](ipmi::Context::ptr ctx, uint8_t cmd,
+ const std::vector<uint8_t>& data) {
+ // Get current IPMI channel and get the max transfer size
+ // (assuming that it does not change).
+ return handleBlobCommand(
+ cmd, data, ipmi::getChannelMaxTransferSize(ctx->channel));
});
/* Install handlers. */
diff --git a/manager.cpp b/manager.cpp
index 894e743..ba019d4 100644
--- a/manager.cpp
+++ b/manager.cpp
@@ -169,8 +169,7 @@
if (auto handler = getActionHandler(session, OpenFlags::read))
{
- return handler->read(session, offset,
- std::min(maximumReadSize, requestedSize));
+ return handler->read(session, offset, requestedSize);
}
return {};
}
diff --git a/manager.hpp b/manager.hpp
index 3dc09cd..31da10f 100644
--- a/manager.hpp
+++ b/manager.hpp
@@ -14,16 +14,6 @@
{
using namespace std::chrono_literals;
-
-/* The maximum read size.
- * NOTE: Once this can be dynamically determined, we'll switch to that method.
- * Having this in a header allows it to used cleanly for now.
- */
-const int crcSize = sizeof(uint16_t);
-const int btReplyHdrLen = 5;
-const int btTransportLength = 64;
-const uint32_t maximumReadSize =
- btTransportLength - (btReplyHdrLen + oem::groupMagicSize + crcSize);
constexpr auto defaultSessionTimeout = 10min;
struct SessionInfo
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 46ffea5..2cbecb1 100644
--- a/process.cpp
+++ b/process.cpp
@@ -123,7 +123,7 @@
}
Resp processBlobCommand(IpmiBlobHandler cmd, ManagerInterface* mgr,
- std::span<const uint8_t> data)
+ std::span<const uint8_t> data, size_t maxSize)
{
Resp result = cmd(mgr, data);
if (std::get<0>(result) != ipmi::ccSuccess)
@@ -150,6 +150,12 @@
return ipmi::responseUnspecifiedError();
}
+ /* Make sure the reply size fits the ipmi buffer */
+ if (replyLength > maxSize)
+ {
+ return ipmi::responseResponseError();
+ }
+
/* The command, whatever it was, replied, so let's set the CRC. */
std::span<const uint8_t> responseView = response;
responseView = responseView.subspan(sizeof(uint16_t));
@@ -166,11 +172,11 @@
return result;
}
-Resp handleBlobCommand(uint8_t cmd, std::vector<uint8_t> data)
+Resp handleBlobCommand(uint8_t cmd, std::vector<uint8_t> data, size_t maxSize)
{
/* on failure rc is set to the corresponding IPMI error. */
return processBlobCommand(validateBlobCommand(cmd, data), getBlobManager(),
- data);
+ data, maxSize);
}
} // namespace blobs
diff --git a/process.hpp b/process.hpp
index 6b89413..494df71 100644
--- a/process.hpp
+++ b/process.hpp
@@ -37,11 +37,11 @@
* @return the ipmi command result.
*/
Resp processBlobCommand(IpmiBlobHandler cmd, ManagerInterface* mgr,
- std::span<const uint8_t> data);
+ std::span<const uint8_t> data, size_t maxSize);
/**
* Given an IPMI command, request buffer, and reply buffer, validate the request
* and call processBlobCommand.
*/
-Resp handleBlobCommand(uint8_t cmd, std::vector<uint8_t> data);
+Resp handleBlobCommand(uint8_t cmd, std::vector<uint8_t> data, size_t maxSize);
} // namespace blobs
diff --git a/test/manager_read_unittest.cpp b/test/manager_read_unittest.cpp
index 73e198b..565eabf 100644
--- a/test/manager_read_unittest.cpp
+++ b/test/manager_read_unittest.cpp
@@ -76,34 +76,4 @@
EXPECT_EQ(result, data);
}
-TEST(ManagerReadTest, ReadTooManyBytesTruncates)
-{
- // For now, the hard-coded maximum transfer size is 64 bytes on read
- // commands, due to a hard-coded buffer in ipmid among other future
- // challenges.
-
- BlobManager mgr;
- std::unique_ptr<BlobMock> m1 = std::make_unique<BlobMock>();
- auto m1ptr = m1.get();
- EXPECT_TRUE(mgr.registerHandler(std::move(m1)));
-
- uint16_t sess = 1;
- uint32_t ofs = 0x54;
- uint32_t requested = 0x100;
- uint16_t flags = OpenFlags::read;
- std::string path = "/asdf/asdf";
- std::vector<uint8_t> data = {0x12, 0x14, 0x15, 0x16};
-
- EXPECT_CALL(*m1ptr, canHandleBlob(path)).WillOnce(Return(true));
- EXPECT_CALL(*m1ptr, open(_, flags, path)).WillOnce(Return(true));
- EXPECT_TRUE(mgr.open(flags, path, &sess));
-
- EXPECT_CALL(*m1ptr, read(sess, ofs, maximumReadSize))
- .WillOnce(Return(data));
-
- std::vector<uint8_t> result = mgr.read(sess, ofs, requested);
- EXPECT_EQ(data.size(), result.size());
- EXPECT_EQ(result, data);
-}
-
} // namespace blobs
diff --git a/test/process_unittest.cpp b/test/process_unittest.cpp
index 02014d4..1a0d6e4 100644
--- a/test/process_unittest.cpp
+++ b/test/process_unittest.cpp
@@ -168,7 +168,7 @@
};
EXPECT_EQ(ipmi::responseInvalidCommand(),
- processBlobCommand(h, &manager, request));
+ processBlobCommand(h, &manager, request, MAX_IPMI_BUFFER));
}
TEST_F(ProcessBlobCommandTest, CommandReturnsOkWithNoPayload)
@@ -184,7 +184,7 @@
};
EXPECT_EQ(ipmi::responseSuccess(std::vector<uint8_t>()),
- processBlobCommand(h, &manager, request));
+ processBlobCommand(h, &manager, request, MAX_IPMI_BUFFER));
}
TEST_F(ProcessBlobCommandTest, CommandReturnsOkWithInvalidPayloadLength)
@@ -200,7 +200,7 @@
};
EXPECT_EQ(ipmi::responseUnspecifiedError(),
- processBlobCommand(h, &manager, request));
+ processBlobCommand(h, &manager, request, MAX_IPMI_BUFFER));
}
TEST_F(ProcessBlobCommandTest, CommandReturnsOkWithValidPayloadLength)
@@ -221,10 +221,31 @@
EXPECT_CALL(crcMock, generateCrc(_)).WillOnce(Return(0x3412));
- auto result = validateReply(processBlobCommand(h, &manager, request));
+ auto result = validateReply(
+ processBlobCommand(h, &manager, request, MAX_IPMI_BUFFER));
EXPECT_EQ(result.size(), payloadLen);
EXPECT_THAT(result, ElementsAre(0x12, 0x34, 0x56));
}
+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;
+ std::vector<uint8_t> request(MAX_IPMI_BUFFER - 1);
+ uint32_t payloadLen = sizeof(uint16_t) + sizeof(uint8_t);
+
+ IpmiBlobHandler h = [payloadLen](ManagerInterface*,
+ std::span<const uint8_t>) {
+ std::vector<uint8_t> output(payloadLen, 0);
+ output[2] = 0x56;
+ return ipmi::responseSuccess(output);
+ };
+
+ EXPECT_EQ(ipmi::responseResponseError(),
+ processBlobCommand(h, &manager, request, 0));
+}
} // namespace blobs