flash: Introduce flash_validate()

Clean up the protocol_negotiate_version() mess. The existing approach
came about due to viewing the vpnor implementation as an edge case in
its own right. The code becomes much neater if we consider all backends
as equal and afford them the callbacks necessary for correct behaviour.

Change-Id: Ifaeee9da459818cf22b2f137ddc5b8d0356b9be9
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

Makefile.am

@@ -20,8 +20,7 @@ if VIRTUAL_PNOR_ENABLED
 include vpnor/Makefile.am.include
 else
 mboxd_SOURCES += flash.c \
-	lpc_reset.c \
-	protocol_negotiate_version.c
+	lpc_reset.c
 endif
 
 mboxctl_SOURCES = mboxctl.c

flash.c

@@ -105,6 +105,13 @@ void flash_dev_free(struct mbox_context *context)
 
 /* Flash Functions */
 
+int flash_validate(struct mbox_context *context, uint32_t offset,
+		   uint32_t size, bool ro)
+{
+	/* Default behaviour is all accesses are valid */
+	return 0;
+}
+
 /*
  * flash_is_erased() - Check if an offset into flash is erased
  * @context:	The mbox context pointer

flash.h

@@ -4,6 +4,7 @@
 #ifndef FLASH_H
 #define FLASH_H
 
+#include <stdbool.h>
 #include <stdint.h>
 
 #define FLASH_DIRTY	0x00
@@ -16,6 +17,8 @@ struct mbox_context;
 
 int flash_dev_init(struct mbox_context *context);
 void flash_dev_free(struct mbox_context *context);
+int flash_validate(struct mbox_context *context, uint32_t offset,
+		   uint32_t size, bool ro);
 int64_t flash_copy(struct mbox_context *context, uint32_t offset, void *mem,
 		   uint32_t size);
 int flash_set_bytemap(struct mbox_context *context, uint32_t offset,

protocol.c

@@ -7,8 +7,9 @@
 
 #include "common.h"
 #include "flash.h"
-#include "mboxd.h"
 #include "lpc.h"
+#include "mboxd.h"
+#include "protocol.h"
 #include "windows.h"
 
 #define BLOCK_SIZE_SHIFT_V1		12 /* 4K */
@@ -76,14 +77,17 @@ int protocol_events_clear(struct mbox_context *context, uint8_t bmc_event)
 	return context->transport->clear_events(context, bmc_event, mask);
 }
 
-int protocol_v1_reset(struct mbox_context *context)
+static int protocol_v1_reset(struct mbox_context *context)
 {
 	/* Host requested it -> No BMC Event */
 	windows_reset_all(context);
 	return lpc_reset(context);
 }
 
-int protocol_v1_get_info(struct mbox_context *context,
+static int protocol_negotiate_version(struct mbox_context *context,
+				      uint8_t requested);
+
+static int protocol_v1_get_info(struct mbox_context *context,
 				struct protocol_get_info *io)
 {
 	uint8_t old_version = context->version;
@@ -120,7 +124,7 @@ int protocol_v1_get_info(struct mbox_context *context,
 	return lpc_map_memory(context);
 }
 
-int protocol_v1_get_flash_info(struct mbox_context *context,
+static int protocol_v1_get_flash_info(struct mbox_context *context,
 			       struct protocol_get_flash_info *io)
 {
 	io->resp.v1.flash_size = context->flash_size;
@@ -149,11 +153,18 @@ static inline uint16_t get_lpc_addr_shifted(struct mbox_context *context)
 	return lpc_addr >> context->block_size_shift;
 }
 
-int protocol_v1_create_window(struct mbox_context *context,
+static int protocol_v1_create_window(struct mbox_context *context,
 				     struct protocol_create_window *io)
 {
-	int rc;
 	uint32_t offset = io->req.offset << context->block_size_shift;
+	uint32_t size = io->req.size << context->block_size_shift;
+	int rc;
+
+	rc = flash_validate(context, offset, size, io->req.ro);
+	if (rc < 0) {
+		/* Backend does not allow window to be created. */
+		return rc;
+	}
 
 	/* Close the current window if there is one */
 	if (context->current) {
@@ -202,7 +213,7 @@ int protocol_v1_create_window(struct mbox_context *context,
 	return 0;
 }
 
-int protocol_v1_mark_dirty(struct mbox_context *context,
+static int protocol_v1_mark_dirty(struct mbox_context *context,
 				  struct protocol_mark_dirty *io)
 {
 	uint32_t offset = io->req.v1.offset;
@@ -305,7 +316,8 @@ static int generic_flush(struct mbox_context *context)
 				  WINDOW_CLEAN);
 }
 
-int protocol_v1_flush(struct mbox_context *context, struct protocol_flush *io)
+static int protocol_v1_flush(struct mbox_context *context,
+			     struct protocol_flush *io)
 {
 	int rc;
 
@@ -331,7 +343,8 @@ int protocol_v1_flush(struct mbox_context *context, struct protocol_flush *io)
 	return generic_flush(context);
 }
 
-int protocol_v1_close(struct mbox_context *context, struct protocol_close *io)
+static int protocol_v1_close(struct mbox_context *context,
+			     struct protocol_close *io)
 {
 	int rc;
 
@@ -355,7 +368,8 @@ int protocol_v1_close(struct mbox_context *context, struct protocol_close *io)
 	return 0;
 }
 
-int protocol_v1_ack(struct mbox_context *context, struct protocol_ack *io)
+static int protocol_v1_ack(struct mbox_context *context,
+			   struct protocol_ack *io)
 {
 	return protocol_events_clear(context,
 				     (io->req.flags & BMC_EVENT_ACK_MASK));
@@ -380,7 +394,7 @@ static uint16_t get_suggested_timeout(struct mbox_context *context)
 	return ret;
 }
 
-int protocol_v2_get_info(struct mbox_context *context,
+static int protocol_v2_get_info(struct mbox_context *context,
 				struct protocol_get_info *io)
 {
 	uint8_t old_version = context->version;
@@ -415,7 +429,7 @@ int protocol_v2_get_info(struct mbox_context *context,
 	return lpc_map_memory(context);
 }
 
-int protocol_v2_get_flash_info(struct mbox_context *context,
+static int protocol_v2_get_flash_info(struct mbox_context *context,
 				      struct protocol_get_flash_info *io)
 {
 	io->resp.v2.flash_size =
@@ -426,7 +440,7 @@ int protocol_v2_get_flash_info(struct mbox_context *context,
 	return 0;
 }
 
-int protocol_v2_create_window(struct mbox_context *context,
+static int protocol_v2_create_window(struct mbox_context *context,
 				     struct protocol_create_window *io)
 {
 	int rc;
@@ -442,7 +456,7 @@ int protocol_v2_create_window(struct mbox_context *context,
 	return 0;
 }
 
-int protocol_v2_mark_dirty(struct mbox_context *context,
+static int protocol_v2_mark_dirty(struct mbox_context *context,
 				  struct protocol_mark_dirty *io)
 {
 	if (!(context->current && context->current_is_write)) {
@@ -458,7 +472,7 @@ int protocol_v2_mark_dirty(struct mbox_context *context,
 				  io->req.v2.size, WINDOW_DIRTY);
 }
 
-int protocol_v2_erase(struct mbox_context *context,
+static int protocol_v2_erase(struct mbox_context *context,
 			     struct protocol_erase *io)
 {
 	size_t start, len;
@@ -487,7 +501,8 @@ int protocol_v2_erase(struct mbox_context *context,
 	return 0;
 }
 
-int protocol_v2_flush(struct mbox_context *context, struct protocol_flush *io)
+static int protocol_v2_flush(struct mbox_context *context,
+			     struct protocol_flush *io)
 {
 	if (!(context->current && context->current_is_write)) {
 		MSG_ERR("Tried to call flush without open write window\n");
@@ -497,7 +512,8 @@ int protocol_v2_flush(struct mbox_context *context, struct protocol_flush *io)
 	return generic_flush(context);
 }
 
-int protocol_v2_close(struct mbox_context *context, struct protocol_close *io)
+static int protocol_v2_close(struct mbox_context *context,
+			     struct protocol_close *io)
 {
 	int rc;
 
@@ -521,6 +537,51 @@ int protocol_v2_close(struct mbox_context *context, struct protocol_close *io)
 	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,
+};
+
+static 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)
 {
 	protocol_negotiate_version(context, API_MAX_VERSION);

protocol.h

@@ -119,8 +119,6 @@ struct protocol_ops {
 int protocol_init(struct mbox_context *context);
 void protocol_free(struct mbox_context *context);
 
-int protocol_negotiate_version(struct mbox_context *context, uint8_t requested);
-
 /* Sneaky reset: Don't tell the host */
 int __protocol_reset(struct mbox_context *context);
 
@@ -132,32 +130,4 @@ int protocol_events_put(struct mbox_context *context,
 int protocol_events_set(struct mbox_context *context, uint8_t bmc_event);
 int protocol_events_clear(struct mbox_context *context, uint8_t bmc_event);
 
-/* Protocol v1 */
-int protocol_v1_reset(struct mbox_context *context);
-int protocol_v1_get_info(struct mbox_context *context,
-			 struct protocol_get_info *io);
-int protocol_v1_get_flash_info(struct mbox_context *context,
-			       struct protocol_get_flash_info *io);
-int protocol_v1_create_window(struct mbox_context *context,
-			      struct protocol_create_window *io);
-int protocol_v1_mark_dirty(struct mbox_context *context,
-			   struct protocol_mark_dirty *io);
-int protocol_v1_flush(struct mbox_context *context, struct protocol_flush *io);
-int protocol_v1_close(struct mbox_context *context, struct protocol_close *io);
-int protocol_v1_ack(struct mbox_context *context, struct protocol_ack *io);
-
-/* Protocol v2 */
-int protocol_v2_get_info(struct mbox_context *context,
-			 struct protocol_get_info *io);
-int protocol_v2_get_flash_info(struct mbox_context *context,
-			       struct protocol_get_flash_info *io);
-int protocol_v2_create_window(struct mbox_context *context,
-			      struct protocol_create_window *io);
-int protocol_v2_mark_dirty(struct mbox_context *context,
-			   struct protocol_mark_dirty *io);
-int protocol_v2_erase(struct mbox_context *context,
-		      struct protocol_erase *io);
-int protocol_v2_flush(struct mbox_context *context, struct protocol_flush *io);
-int protocol_v2_close(struct mbox_context *context, struct protocol_close *io);
-
 #endif /* PROTOCOL_H */

protocol_negotiate_version.c

-// SPDX-License-Identifier: Apache-2.0
-// Copyright (C) 2018 IBM Corp.
-#include "config.h"
-
-#include <errno.h>
-
-#include "mboxd.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;
-}

test/Makefile.am.include

@@ -26,8 +26,7 @@ TEST_MBOX_SRCS = \
 	lpc_reset.c \
 	common.c \
 	flash.c \
-	protocol.c \
-	protocol_negotiate_version.c
+	protocol.c
 
 TEST_MOCK_SRCS = %reldir%/tmpf.c %reldir%/mbox.c %reldir%/system.c
 

vpnor/Makefile.am.include

@@ -2,9 +2,7 @@ mboxd_SOURCES += %reldir%/pnor_partition_table.cpp \
 	%reldir%/mboxd_pnor_partition_table.cpp \
 	%reldir%/flash.cpp \
 	%reldir%/pnor_partition.cpp \
-	%reldir%/lpc_reset.cpp \
-	%reldir%/protocol.cpp \
-	%reldir%/protocol_negotiate_version.cpp
+	%reldir%/lpc_reset.cpp
 
 mboxd_LDFLAGS += -lstdc++fs \
 	$(SDBUSPLUS_LIBS) \

vpnor/flash.cpp

@@ -237,3 +237,39 @@ int flash_write(struct mbox_context* context, uint32_t offset, void* buf,
     }
     return 0;
 }
+
+static bool vpnor_partition_is_readonly(const pnor_partition& part)
+{
+    return part.data.user.data[1] & PARTITION_READONLY;
+}
+
+int flash_validate(struct mbox_context* context, uint32_t offset,
+                   uint32_t size __attribute__((unused)), bool ro)
+{
+    /* All reads are allowed */
+    if (ro)
+    {
+        return 0;
+    }
+
+    /* Only allow write windows on regions mapped by the ToC as writeable */
+    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 0;
+}

vpnor/protocol.cpp

-// SPDX-License-Identifier: Apache-2.0
-// Copyright (C) 2018 IBM Corp.
-#include "config.h"
-
-extern "C" {
-#include "mboxd.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);
-}

vpnor/protocol_negotiate_version.cpp

-// SPDX-License-Identifier: Apache-2.0
-// Copyright (C) 2018 IBM Corp.
-#include "config.h"
-
-#include <errno.h>
-
-extern "C" {
-#include "mboxd.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;
-}

vpnor/test/Makefile.am.include

@@ -14,8 +14,6 @@ TEST_MBOX_VPNOR_INTEG_SRCS = \
 	vpnor/flash.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 \