vpnor: Hijack protocol rather than transport
By hijacking the transport the changes in behaviour were limited to the
mailbox interface. Now that we have a DBus interface as well this would
lead to inconsistent behaviour dependent on the transport.
Instead of hooking the transport, push the hook down to the protocol
level where we will achieve consistent behaviour across all transports.
Change-Id: I437866a6dbda107149336c15a00ee1aa058f5875
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
diff --git a/Makefile.am b/Makefile.am
index 102e7c4..3a7c3d4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -19,7 +19,8 @@
include vpnor/Makefile.am.include
else
mboxd_SOURCES += flash.c \
- lpc_reset.c
+ lpc_reset.c \
+ protocol_negotiate_version.c
endif
mboxctl_SOURCES = mboxctl.c
diff --git a/protocol.c b/protocol.c
index 293934f..2bd91ab 100644
--- a/protocol.c
+++ b/protocol.c
@@ -448,55 +448,9 @@
return 0;
}
-static const struct protocol_ops protocol_ops_v1 = {
- .reset = protocol_v1_reset,
- .get_info = protocol_v1_get_info,
- .get_flash_info = protocol_v1_get_flash_info,
- .create_window = protocol_v1_create_window,
- .mark_dirty = protocol_v1_mark_dirty,
- .erase = NULL,
- .flush = protocol_v1_flush,
- .close = protocol_v1_close,
- .ack = protocol_v1_ack,
-};
-
-static const struct protocol_ops protocol_ops_v2 = {
- .reset = protocol_v1_reset,
- .get_info = protocol_v2_get_info,
- .get_flash_info = protocol_v2_get_flash_info,
- .create_window = protocol_v2_create_window,
- .mark_dirty = protocol_v2_mark_dirty,
- .erase = protocol_v2_erase,
- .flush = protocol_v2_flush,
- .close = protocol_v2_close,
- .ack = protocol_v1_ack,
-};
-
-static const struct protocol_ops *protocol_ops_map[] = {
- [0] = NULL,
- [1] = &protocol_ops_v1,
- [2] = &protocol_ops_v2,
-};
-
-int protocol_negotiate_version(struct mbox_context *context,
- uint8_t requested)
-{
- /* Check we support the version requested */
- if (requested < API_MIN_VERSION)
- return -EINVAL;
-
- context->version = (requested > API_MAX_VERSION) ?
- API_MAX_VERSION : requested;
-
- context->protocol = protocol_ops_map[context->version];
-
- return context->version;
-}
-
int protocol_init(struct mbox_context *context)
{
- context->version = API_MAX_VERSION;
- context->protocol = protocol_ops_map[context->version];
+ protocol_negotiate_version(context, API_MAX_VERSION);
return 0;
}
diff --git a/protocol_negotiate_version.c b/protocol_negotiate_version.c
new file mode 100644
index 0000000..849363f
--- /dev/null
+++ b/protocol_negotiate_version.c
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright (C) 2018 IBM Corp.
+#include "config.h"
+
+#include <errno.h>
+
+#include "mbox.h"
+#include "protocol.h"
+
+static const struct protocol_ops protocol_ops_v1 = {
+ .reset = protocol_v1_reset,
+ .get_info = protocol_v1_get_info,
+ .get_flash_info = protocol_v1_get_flash_info,
+ .create_window = protocol_v1_create_window,
+ .mark_dirty = protocol_v1_mark_dirty,
+ .erase = NULL,
+ .flush = protocol_v1_flush,
+ .close = protocol_v1_close,
+ .ack = protocol_v1_ack,
+};
+
+static const struct protocol_ops protocol_ops_v2 = {
+ .reset = protocol_v1_reset,
+ .get_info = protocol_v2_get_info,
+ .get_flash_info = protocol_v2_get_flash_info,
+ .create_window = protocol_v2_create_window,
+ .mark_dirty = protocol_v2_mark_dirty,
+ .erase = protocol_v2_erase,
+ .flush = protocol_v2_flush,
+ .close = protocol_v2_close,
+ .ack = protocol_v1_ack,
+};
+
+static const struct protocol_ops *protocol_ops_map[] = {
+ [0] = NULL,
+ [1] = &protocol_ops_v1,
+ [2] = &protocol_ops_v2,
+};
+
+int protocol_negotiate_version(struct mbox_context *context,
+ uint8_t requested)
+{
+ /* Check we support the version requested */
+ if (requested < API_MIN_VERSION)
+ return -EINVAL;
+
+ context->version = (requested > API_MAX_VERSION) ?
+ API_MAX_VERSION : requested;
+
+ context->protocol = protocol_ops_map[context->version];
+
+ return context->version;
+}
diff --git a/test/Makefile.am.include b/test/Makefile.am.include
index 71fa851..fab9e14 100644
--- a/test/Makefile.am.include
+++ b/test/Makefile.am.include
@@ -25,7 +25,8 @@
lpc_reset.c \
common.c \
flash.c \
- protocol.c
+ protocol.c \
+ protocol_negotiate_version.c
TEST_MOCK_SRCS = %reldir%/tmpf.c %reldir%/mbox.c %reldir%/system.c
diff --git a/transport_mbox.c b/transport_mbox.c
index bc5e1e2..0d3d9b7 100644
--- a/transport_mbox.c
+++ b/transport_mbox.c
@@ -47,7 +47,7 @@
static const struct errno_map errno_map_v2[] = {
{ 0, MBOX_R_SUCCESS },
- { EACCES, MBOX_R_PARAM_ERROR },
+ { EACCES, MBOX_R_WINDOW_ERROR },
{ EBUSY, MBOX_R_BUSY },
{ EINVAL, MBOX_R_PARAM_ERROR },
{ EPERM, MBOX_R_WINDOW_ERROR },
diff --git a/vpnor/Makefile.am.include b/vpnor/Makefile.am.include
index 38fc631..75bd92e 100644
--- a/vpnor/Makefile.am.include
+++ b/vpnor/Makefile.am.include
@@ -3,7 +3,8 @@
%reldir%/flash.cpp \
%reldir%/pnor_partition.cpp \
%reldir%/lpc_reset.cpp \
- %reldir%/transport_mbox.cpp
+ %reldir%/protocol.cpp \
+ %reldir%/protocol_negotiate_version.cpp
mboxd_LDFLAGS += -lstdc++fs \
$(SDBUSPLUS_LIBS) \
diff --git a/vpnor/mboxd_pnor_partition_table.cpp b/vpnor/mboxd_pnor_partition_table.cpp
index cc0fccf..f787fc8 100644
--- a/vpnor/mboxd_pnor_partition_table.cpp
+++ b/vpnor/mboxd_pnor_partition_table.cpp
@@ -13,7 +13,6 @@
#include "xyz/openbmc_project/Common/error.hpp"
#include <phosphor-logging/elog-errors.hpp>
#include <experimental/filesystem>
-#include "vpnor/transport_mbox.hpp"
int init_vpnor(struct mbox_context *context)
{
@@ -48,8 +47,6 @@
if (context && !context->vpnor)
{
- context->handlers = vpnor_mbox_handlers;
-
try
{
context->vpnor = new vpnor_partition_table;
diff --git a/vpnor/protocol.cpp b/vpnor/protocol.cpp
new file mode 100644
index 0000000..57b35a7
--- /dev/null
+++ b/vpnor/protocol.cpp
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright (C) 2018 IBM Corp.
+#include "config.h"
+
+extern "C" {
+#include "mbox.h"
+#include "protocol.h"
+#include "vpnor/protocol.h"
+}
+
+#include "vpnor/pnor_partition_table.hpp"
+
+/* XXX: Maybe this should be a method on a class? */
+static bool vpnor_partition_is_readonly(const pnor_partition &part)
+{
+ return part.data.user.data[1] & PARTITION_READONLY;
+}
+
+typedef int (*create_window_fn)(struct mbox_context *context,
+ struct protocol_create_window *io);
+
+static int generic_vpnor_create_window(struct mbox_context *context,
+ struct protocol_create_window *io,
+ create_window_fn create_window)
+{
+ if (io->req.ro)
+ {
+ return create_window(context, io);
+ }
+
+ /* Only allow write windows on regions mapped by the ToC as writeable */
+ size_t offset = io->req.offset;
+ offset <<= context->block_size_shift;
+ try
+ {
+ const pnor_partition &part = context->vpnor->table->partition(offset);
+ if (vpnor_partition_is_readonly(part))
+ {
+ return -EPERM;
+ }
+ }
+ catch (const openpower::virtual_pnor::UnmappedOffset &e)
+ {
+ /*
+ * Writes to unmapped areas are not meaningful, so deny the request.
+ * This removes the ability for a compromised host to abuse unused
+ * space if any data was to be persisted (which it isn't).
+ */
+ return -EACCES;
+ }
+
+ return create_window(context, io);
+}
+
+int protocol_v1_vpnor_create_window(struct mbox_context *context,
+ struct protocol_create_window *io)
+{
+ return generic_vpnor_create_window(context, io, protocol_v1_create_window);
+}
+
+int protocol_v2_vpnor_create_window(struct mbox_context *context,
+ struct protocol_create_window *io)
+{
+ return generic_vpnor_create_window(context, io, protocol_v2_create_window);
+}
diff --git a/vpnor/protocol.h b/vpnor/protocol.h
new file mode 100644
index 0000000..8cebda6
--- /dev/null
+++ b/vpnor/protocol.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: Apache-2.0 */
+/* Copyright (C) 2018 IBM Corp. */
+#ifndef VPNOR_PROTOCOL_H
+#define VPNOR_PROTOCOL_H
+
+#include "protocol.h"
+
+/* Protocol v1 */
+int protocol_v1_vpnor_create_window(struct mbox_context *context,
+ struct protocol_create_window *io);
+
+/* Protocol v2 */
+int protocol_v2_vpnor_create_window(struct mbox_context *context,
+ struct protocol_create_window *io);
+
+#endif /* VPNOR_PROTOCOL_H */
diff --git a/vpnor/protocol_negotiate_version.cpp b/vpnor/protocol_negotiate_version.cpp
new file mode 100644
index 0000000..4aff8db
--- /dev/null
+++ b/vpnor/protocol_negotiate_version.cpp
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright (C) 2018 IBM Corp.
+#include "config.h"
+
+#include <errno.h>
+
+extern "C" {
+#include "mbox.h"
+#include "protocol.h"
+#include "vpnor/protocol.h"
+}
+
+/* We need to hijack create_window for vpnor */
+
+static const struct protocol_ops protocol_ops_v1 = {
+ .reset = protocol_v1_reset,
+ .get_info = protocol_v1_get_info,
+ .get_flash_info = protocol_v1_get_flash_info,
+ .create_window = protocol_v1_vpnor_create_window,
+ .mark_dirty = protocol_v1_mark_dirty,
+ .erase = NULL,
+ .flush = protocol_v1_flush,
+ .close = protocol_v1_close,
+ .ack = protocol_v1_ack,
+};
+
+static const struct protocol_ops protocol_ops_v2 = {
+ .reset = protocol_v1_reset,
+ .get_info = protocol_v2_get_info,
+ .get_flash_info = protocol_v2_get_flash_info,
+ .create_window = protocol_v2_vpnor_create_window,
+ .mark_dirty = protocol_v2_mark_dirty,
+ .erase = protocol_v2_erase,
+ .flush = protocol_v2_flush,
+ .close = protocol_v2_close,
+ .ack = protocol_v1_ack,
+};
+
+static const struct protocol_ops *protocol_ops_map[] = {
+ [0] = NULL,
+ [1] = &protocol_ops_v1,
+ [2] = &protocol_ops_v2,
+};
+
+int protocol_negotiate_version(struct mbox_context *context, uint8_t requested)
+{
+ /* Check we support the version requested */
+ if (requested < API_MIN_VERSION)
+ return -EINVAL;
+
+ uint8_t version =
+ (requested > API_MAX_VERSION) ? API_MAX_VERSION : requested;
+ context->version = static_cast<enum api_version>(version);
+
+ context->protocol = protocol_ops_map[context->version];
+
+ return context->version;
+}
diff --git a/vpnor/test/Makefile.am.include b/vpnor/test/Makefile.am.include
index 2444606..b029b37 100644
--- a/vpnor/test/Makefile.am.include
+++ b/vpnor/test/Makefile.am.include
@@ -12,9 +12,10 @@
vpnor/lpc_reset.cpp \
vpnor/mboxd_pnor_partition_table.cpp \
vpnor/flash.cpp \
- vpnor/transport_mbox.cpp \
vpnor/pnor_partition.cpp \
vpnor/pnor_partition_table.cpp \
+ vpnor/protocol.cpp \
+ vpnor/protocol_negotiate_version.cpp \
%reldir%/tmpd.cpp
VPNOR_LDADD = -lstdc++fs \
diff --git a/vpnor/transport_mbox.cpp b/vpnor/transport_mbox.cpp
deleted file mode 100644
index 620006e..0000000
--- a/vpnor/transport_mbox.cpp
+++ /dev/null
@@ -1,58 +0,0 @@
-#include "config.h"
-
-extern "C" {
-#include "mbox.h"
-#include "transport_mbox.h"
-};
-
-#include "vpnor/transport_mbox.hpp"
-#include "vpnor/pnor_partition_table.hpp"
-
-// clang-format off
-const mboxd_mbox_handler vpnor_mbox_handlers[NUM_MBOX_CMDS] =
-{
- mbox_handle_reset,
- mbox_handle_mbox_info,
- mbox_handle_flash_info,
- mbox_handle_read_window,
- mbox_handle_close_window,
- vpnor_handle_write_window,
- mbox_handle_dirty_window,
- mbox_handle_flush_window,
- mbox_handle_ack,
- mbox_handle_erase_window
-};
-// clang-format on
-
-/* XXX: Maybe this should be a method on a class? */
-static bool vpnor_partition_is_readonly(const pnor_partition& part)
-{
- return part.data.user.data[1] & PARTITION_READONLY;
-}
-
-int vpnor_handle_write_window(struct mbox_context* context,
- union mbox_regs* req, struct mbox_msg* resp)
-{
- size_t offset = get_u16(&req->msg.args[0]);
- offset <<= context->block_size_shift;
- try
- {
- const pnor_partition& part = context->vpnor->table->partition(offset);
- if (vpnor_partition_is_readonly(part))
- {
- return -MBOX_R_WINDOW_ERROR;
- }
- }
- catch (const openpower::virtual_pnor::UnmappedOffset& e)
- {
- /*
- * Writes to unmapped areas are not meaningful, so deny the request.
- * This removes the ability for a compromised host to abuse unused
- * space if any data was to be persisted (which it isn't).
- */
- return -MBOX_R_WINDOW_ERROR;
- }
-
- /* Defer to the default handler */
- return mbox_handle_write_window(context, req, resp);
-}
diff --git a/vpnor/transport_mbox.hpp b/vpnor/transport_mbox.hpp
deleted file mode 100644
index d9a05bb..0000000
--- a/vpnor/transport_mbox.hpp
+++ /dev/null
@@ -1,8 +0,0 @@
-extern "C" {
-#include "mbox.h"
-
-extern const mboxd_mbox_handler vpnor_mbox_handlers[NUM_MBOX_CMDS];
-
-int vpnor_handle_write_window(struct mbox_context *context,
- union mbox_regs *req, struct mbox_msg *resp);
-};