Re: [PATCH 05/10] firewall: introduce stm32_firewall framework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Oleksii,

On 7/7/23 15:50, Oleksii Moisieiev wrote:

Gatien Chevallier <gatien.chevallier@xxxxxxxxxxx> writes:

Introduce a firewall framework that offers to firewall consumers different
firewall services such as the ability to check their access rights against
their firewall controller(s).

The firewall framework offers a generic API that is defined in firewall
controllers drivers to best fit the specificity of each firewall.

There are various types of firewalls:
-Peripheral firewalls that filter accesses to peripherals
-Memory firewalls that filter accesses to memories or memory regions
-Resource firewalls that filter accesses to internal resources such as
reset and clock controllers

A firewall controller must be probed at arch_initcall level and register
to the framework so that consumers can use their services.

Signed-off-by: Gatien Chevallier <gatien.chevallier@xxxxxxxxxxx>
---
  MAINTAINERS                               |   5 +
  arch/arm64/Kconfig.platforms              |   1 +
  drivers/bus/Kconfig                       |  10 +
  drivers/bus/Makefile                      |   1 +
  drivers/bus/stm32_firewall.c              | 252 ++++++++++++++++++++++
  drivers/bus/stm32_firewall.h              |  83 +++++++
  include/linux/bus/stm32_firewall_device.h | 134 ++++++++++++
  7 files changed, 486 insertions(+)
  create mode 100644 drivers/bus/stm32_firewall.c
  create mode 100644 drivers/bus/stm32_firewall.h
  create mode 100644 include/linux/bus/stm32_firewall_device.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 41385f01fa98..fabf95ba9b86 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20123,6 +20123,11 @@ T:	git git://linuxtv.org/media_tree.git
  F:	Documentation/devicetree/bindings/media/i2c/st,st-mipid02.yaml
  F:	drivers/media/i2c/st-mipid02.c
+ST STM32 FIREWALL
+M:	Gatien Chevallier <gatien.chevallier@xxxxxxxxxxx>
+S:	Maintained
+F:	drivers/bus/stm32_firewall.c
+
  ST STM32 I2C/SMBUS DRIVER
  M:	Pierre-Yves MORDRET <pierre-yves.mordret@xxxxxxxxxxx>
  M:	Alain Volmat <alain.volmat@xxxxxxxxxxx>
diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 6069120199bb..5a46e90f1e4e 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -293,6 +293,7 @@ config ARCH_STM32
  	select ARM_SMC_MBOX
  	select ARM_SCMI_PROTOCOL
  	select COMMON_CLK_SCMI
+	select STM32_FIREWALL
  	help
  	  This enables support for ARMv8 based STMicroelectronics
  	  STM32 family, including:
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index fcfa280df98a..4d54a7ea52b2 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -163,6 +163,16 @@ config QCOM_SSC_BLOCK_BUS
  	  i2c/spi/uart controllers, a hexagon core, and a clock controller
  	  which provides clocks for the above.
+config STM32_FIREWALL
+	bool "STM32 Firewall framework"
+	depends on ARCH_STM32
+	default MACH_STM32MP157 || MACH_STM32MP13 || MACH_STM32MP25
+	help
+	  Say y to enable firewall framework and its services. Firewall
+	  controllers will be able to register to the framework. Firewall
+	  controllers must be initialized and register to the firewall framework
+	  at arch_initcall level.
+
  config SUN50I_DE2_BUS
  	bool "Allwinner A64 DE2 Bus Driver"
  	  default ARM64
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index d90eed189a65..fc0511450ec2 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_OMAP_INTERCONNECT)	+= omap_l3_smx.o omap_l3_noc.o
  obj-$(CONFIG_OMAP_OCP2SCP)	+= omap-ocp2scp.o
  obj-$(CONFIG_QCOM_EBI2)		+= qcom-ebi2.o
  obj-$(CONFIG_QCOM_SSC_BLOCK_BUS)	+= qcom-ssc-block-bus.o
+obj-$(CONFIG_STM32_FIREWALL)	+= stm32_firewall.o
  obj-$(CONFIG_SUN50I_DE2_BUS)	+= sun50i-de2.o
  obj-$(CONFIG_SUNXI_RSB)		+= sunxi-rsb.o
  obj-$(CONFIG_OF)		+= simple-pm-bus.o
diff --git a/drivers/bus/stm32_firewall.c b/drivers/bus/stm32_firewall.c
new file mode 100644
index 000000000000..510db5bc6eaf
--- /dev/null
+++ b/drivers/bus/stm32_firewall.c
@@ -0,0 +1,252 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023, STMicroelectronics - All Rights Reserved
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/bus/stm32_firewall_device.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#include "stm32_firewall.h"
+
+/* Corresponds to STM32_FIREWALL_MAX_EXTRA_ARGS + firewall controller reference + firewall ID */
+#define STM32_FIREWALL_MAX_ARGS		(STM32_FIREWALL_MAX_EXTRA_ARGS + 2)
+
+static LIST_HEAD(firewall_controller_list);
+static DEFINE_MUTEX(firewall_controller_list_lock);
+
+static int stm32_firewall_get_id(struct device_node *np, u32 *id)
+{
+	u32 feature_domain_cell[2];
+
+	/* Get property from device node */
+	if (of_property_read_u32_array(np, "feature-domains",
+				       feature_domain_cell,
+				       ARRAY_SIZE(feature_domain_cell))) {
+		pr_err("Unable to find get firewall ID property\n");
+		return -ENODEV;
+	}
+
+	*id = feature_domain_cell[1];
+
+	return 0;
+}
+
+/* Firewall device API */
+
+int stm32_firewall_get_firewall(struct device_node *np,
+				struct stm32_firewall *firewall)
+{
+	struct stm32_firewall_controller *ctrl;
+	struct of_phandle_args args;
+	u32 controller_phandle;
+	bool match = false;
+	size_t i;
+	int err;
+
+	if (!firewall)
+		return -EINVAL;
+
+	/* The controller phandle is always the first argument of the feature-domains property. */
+	err = of_property_read_u32(np, "feature-domains", &controller_phandle);
+	if (err) {
+		pr_err("Unable to get feature-domains property for node %s\n", np->full_name);
+		return err;
+	}
+
+	/* Parse property with phandle parsed out */
+	err = of_parse_phandle_with_args(np, "feature-domains", "#feature-domain-cells", 0, &args);
+	if (err) {
+		pr_err("Unable to read feature-domains arguments for node %s\n", np->full_name);
+		return err;
+	}
+
+	/* The phandle is parsed out */
+	if (args.args_count > STM32_FIREWALL_MAX_ARGS - 1)
+		return -EINVAL;
+
+	of_node_put(np);
+
+	/* Check if the parsed phandle corresponds to a registered firewall controller */
+	mutex_lock(&firewall_controller_list_lock);
+	list_for_each_entry(ctrl, &firewall_controller_list, entry) {
+		if (ctrl->dev->of_node->phandle == controller_phandle) {
+			match = true;
+			firewall->firewall_ctrl = ctrl;
+			break;
+		}
+	}
+	mutex_unlock(&firewall_controller_list_lock);
+	if (!match) {
+		firewall->firewall_ctrl = NULL;
+		pr_err("No firewall controller registered for %s\n", np->full_name);
+		return -ENODEV;
+	}
+
+	/*
+	 * The firewall ID is always the second argument of the feature-domains property.
+	 * The first argument is already parsed out, so args.args[0] is the second argument.
+	 */
+	firewall->firewall_id = args.args[0];
+
+	/* Extra args start at the third argument */
+	for (i = 0; i < args.args_count; i++)
+		firewall->extra_args[i] = args.args[i + 1];
+
+	/* Remove the firewall ID arg that is not an extra argument */
+	if (args.args_count >= 1)
+		firewall->extra_args_size = args.args_count - 1;

As I can see you support some extra args, that can be provided in the
feature-domain property. But in the binding description I see maxItems:
2. I beliewe this should be highlighted in bindings description.


Good point, maybe it would be better to define a high maxItem value in
the binding description that is aligned with STM32_FIREWALL_MAX_ARGS.

+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(stm32_firewall_get_firewall);
+
+int stm32_firewall_grant_access(struct stm32_firewall *firewall)
+{
+	struct stm32_firewall_controller *firewall_controller;
+
+	if (!firewall || firewall->firewall_id == U32_MAX)
+		return -EINVAL;
+
+	firewall_controller = firewall->firewall_ctrl;
+
+	if (!firewall_controller)
+		return -ENODEV;
+
+	return firewall_controller->grant_access(firewall_controller, firewall->firewall_id);
+}
+EXPORT_SYMBOL_GPL(stm32_firewall_grant_access);
+
+int stm32_firewall_grant_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id)
+{
+	struct stm32_firewall_controller *firewall_controller;
+
+	if (!firewall || subsystem_id == U32_MAX || firewall->firewall_id == U32_MAX)
+		return -EINVAL;
+
+	firewall_controller = firewall->firewall_ctrl;
+
+	if (!firewall_controller)
+		return -ENODEV;
+
+	return firewall_controller->grant_access(firewall_controller, subsystem_id);
+}
+EXPORT_SYMBOL_GPL(stm32_firewall_grant_access_by_id);
+
+void stm32_firewall_release_access(struct stm32_firewall *firewall)
+{
+	struct stm32_firewall_controller *firewall_controller;
+
+	if (!firewall || firewall->firewall_id == U32_MAX) {
+		pr_err("Incorrect arguments when releasing a firewall access");
+		return;
+	}
+
+	firewall_controller = firewall->firewall_ctrl;
+
+	if (!firewall_controller) {
+		pr_debug("No firewall controller to release");
+		return;
+	}
+
+	firewall_controller->release_access(firewall_controller, firewall->firewall_id);
+}
+EXPORT_SYMBOL_GPL(stm32_firewall_release_access);
+
+void stm32_firewall_release_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id)
+{
+	struct stm32_firewall_controller *firewall_controller;
+
+	if (!firewall || subsystem_id == U32_MAX || firewall->firewall_id == U32_MAX) {
+		pr_err("Incorrect arguments when releasing a firewall access");
+		return;
+	}
+
+	firewall_controller = firewall->firewall_ctrl;
+
+	if (!firewall_controller) {
+		pr_debug("No firewall controller to release");
+		return;
+	}
+
+	firewall_controller->release_access(firewall_controller, subsystem_id);
+}
+EXPORT_SYMBOL_GPL(stm32_firewall_release_access_by_id);
+
+/* Firewall controller API */
+
+int stm32_firewall_controller_register(struct stm32_firewall_controller *firewall_controller)
+{
+	pr_info("Registering firewall controller %s", dev_name(firewall_controller->dev));
+
+	if (!firewall_controller)
+		return -ENODEV;
+
+	mutex_lock(&firewall_controller_list_lock);
+	list_add_tail(&firewall_controller->entry, &firewall_controller_list);
+	mutex_unlock(&firewall_controller_list_lock);
+
+	return 0;
+
+}
+
+void stm32_firewall_controller_unregister(struct stm32_firewall_controller *firewall_controller)
+{
+	struct stm32_firewall_controller *ctrl, *tmp;
+	bool controller_removed = false;
+
+	if (!firewall_controller) {
+		pr_debug("Null reference while unregistering firewall controller");
+		return;
+	}
+
+	mutex_lock(&firewall_controller_list_lock);
+	list_for_each_entry_safe(ctrl, tmp, &firewall_controller_list, entry) {
+		if (ctrl == firewall_controller) {
+			controller_removed = true;
+			list_del_init(&ctrl->entry);
+			break;
+		}
+	}
IIUC list_for_each_entry_safe protects you from removing node during
loop. But all list operations are done under
firewall_controller_list_lock mutex. I beliewe there is no need for
_safe call under the mutex because removing node during loop is
impossible. I think it worth investigation if it's safe to use
list_for_each_entry_safe without lock.

AFAICT, list_for_each_entry_safe() does not protect against concurrent
accesses, so I'll keep the mutex lock.

Deleting while iterating will cause trouble if not using the safe
version. I think it is fine as it is.

+	mutex_unlock(&firewall_controller_list_lock);
+
+	if (!controller_removed)
+		pr_debug("There was no firewall controller named %s to unregister",
+			 dev_name(firewall_controller->dev));
+}
+
+void stm32_firewall_populate_bus(struct stm32_firewall_controller *firewall_controller)
+{
+	struct device_node *child;
+	struct device *parent;
+	u32 firewall_id;
+	int err;
+
+	parent = firewall_controller->dev;
+
+	dev_dbg(parent, "Populating %s system bus\n", dev_name(firewall_controller->dev));
+
+	for_each_available_child_of_node(dev_of_node(parent), child) {
+		err = stm32_firewall_get_id(child, &firewall_id);
+		if (err < 0 ||
+		    firewall_controller->grant_access(firewall_controller, firewall_id)) {
+			/*
+			 * Peripheral access not allowed or not defined.
+			 * Mark the node as populated so platform bus won't probe it
+			 */
+			of_node_set_flag(child, OF_POPULATED);
+			dev_err(parent, "%s: Device driver will not be probed\n",
+				child->full_name);
+		}
+	}
+}
diff --git a/drivers/bus/stm32_firewall.h b/drivers/bus/stm32_firewall.h
new file mode 100644
index 000000000000..8d92e8c1ab77
--- /dev/null
+++ b/drivers/bus/stm32_firewall.h
@@ -0,0 +1,83 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2023, STMicroelectronics - All Rights Reserved
+ */
+
+#ifndef _STM32_FIREWALL_H
+#define _STM32_FIREWALL_H
+
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+/**
+ * STM32_PERIPHERAL_FIREWALL:		This type of firewall protects peripherals

Intendation

+ * STM32_MEMORY_FIREWALL:		This type of firewall protects memories/subsets of memory
+ *					zones
+ * STM32_RESOURCE_FIREWALL:		This type of firewall protects internal resources
+ * STM32_NOTYPE_FIREWALL:		Undefined firewall type
+ */
+
+#define STM32_PERIPHERAL_FIREWALL	BIT(1)

Intendation


I'm sorry but I don't understand. I indent with tabs, maybe this is why
it appears strange here. The indentation is fine, same for others below.

+#define STM32_MEMORY_FIREWALL		BIT(2)
+#define STM32_RESOURCE_FIREWALL		BIT(3)
+#define STM32_NOTYPE_FIREWALL		BIT(4)
+
+/**
+ * struct stm32_firewall_controller - Information on firewall controller supplying services
+ *
+ * @name			Name of the firewall controller
+ * @dev				Device reference of the firewall controller

Intendation

+ * @mmio			Base address of the firewall controller
+ * @entry			List entry of the firewall controller list
+ * @type			Type of firewall
+ * @max_entries			Number of entries covered by the firewall
Intendation

+ * @grant_access		Callback used to grant access for a device access against a
+ *				firewall controller
+ * @release_access		Callback used to release resources taken by a device when access was
+ *				granted
+ * @grant_memory_range_access	Callback used to grant access for a device to a given memory region
+ */
+struct stm32_firewall_controller {
+	const char *name;
+	struct device *dev;
+	void __iomem *mmio;
+	struct list_head entry;
+	unsigned int type;
+	unsigned int max_entries;
+
+	int (*grant_access)(struct stm32_firewall_controller *ctrl, u32 id);
+	void (*release_access)(struct stm32_firewall_controller *ctrl, u32 id);
+	int (*grant_memory_range_access)(struct stm32_firewall_controller *ctrl, phys_addr_t paddr,
+					 size_t size);
+};
+
+/**
+ * int stm32_firewall_controller_register - Register a firewall controller to the STM32 firewall
+ *					    framework
+ * @firewall_controller		Firewall controller to register
+ *
+ * Returns 0 in case of success or -ENODEV if no controller was given.
+ */
+int stm32_firewall_controller_register(struct stm32_firewall_controller *firewall_controller);
+
+/**
+ * int stm32_firewall_controller_unregister - Unregister a firewall controller from the STM32
+ *					      firewall framework
+ * @firewall_controller		Firewall controller to unregister
+ */
+void stm32_firewall_controller_unregister(struct stm32_firewall_controller *firewall_controller);
+
+/**
+ * stm32_firewall_populate_bus - Populate device tree nodes that have a correct firewall
+ *				 configuration. This is used at boot-time only, as a sanity check
+ *				 between device tree and firewalls hardware configurations to
+ *				 prevent a kernel crash when a device driver is not granted access
+ *
+ * @firewall_controller		Firewall controller which nodes will be populated or not
+ */
+void stm32_firewall_populate_bus(struct stm32_firewall_controller *firewall_controller);
+
+#endif /* _STM32_FIREWALL_H */
diff --git a/include/linux/bus/stm32_firewall_device.h b/include/linux/bus/stm32_firewall_device.h
new file mode 100644
index 000000000000..ccaecea7fc6c
--- /dev/null
+++ b/include/linux/bus/stm32_firewall_device.h
@@ -0,0 +1,134 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2023, STMicroelectronics - All Rights Reserved
+ */
+
+#ifndef STM32_FIREWALL_DEVICE_H
+#define STM32_FIREWALL_DEVICE_H
+
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#define STM32_FIREWALL_MAX_EXTRA_ARGS		5

It's not clear to me why it's 5. Comment above sais this is
implementation defined. Maybe this parameter can be configurable?


Maybe it should be part of compat_data of firewall controllers :)

+
+/* Opaque reference to stm32_firewall_controller */
+struct stm32_firewall_controller;
+
+/**
+ * stm32_firewall - Information on a device's firewall. Each device can have more than one firewall.
+ *
+ * @firewall_ctrl		Pointer referencing a firewall controller of the device. It is
+ *				opaque so a device cannot manipulate the controller's ops or access
+ *				the controller's data
+ * @extra_args			Extra arguments that are implementation dependent
+ * @extra_args_size		Number of extra arguments
+ * @firewall_id			Firewall ID associated the device for this firewall controller
+ */
+struct stm32_firewall {
+	struct stm32_firewall_controller *firewall_ctrl;
+	u32 extra_args[STM32_FIREWALL_MAX_EXTRA_ARGS];
+	size_t extra_args_size;
+	u32 firewall_id;
+};
+
+#if IS_ENABLED(CONFIG_STM32_FIREWALL)
+/**
+ * stm32_firewall_get_firewall - Get the firewall(s) associated to given device.
+ *				 The firewall controller reference is always the first argument
+ *				 of the feature-domains property.
+ *				 The firewall ID is always the second argument of the
+ *				 feature-domains property.
+ *
+ * @np				Device node to parse

Intendation

+ * @firewall			Resulting firewall reference(s)
+ *
+ * Returns 0 on success, -ENODEV if there's no match with a firewall controller or appropriate errno
+ * code if error occurred.
+ */
+int stm32_firewall_get_firewall(struct device_node *np, struct stm32_firewall *firewall);
+
+/**
+ * stm32_firewall_grant_access - Request firewall access rights and grant access.
+ *
+ * @firewall			Firewall reference containing the ID to check against its firewall
+ *				controller
+ *
+ * Returns 0 if access is granted, -EACCES if access is denied, -ENODEV if firewall is null or
+ * appropriate errno code if error occurred
+ */
+int stm32_firewall_grant_access(struct stm32_firewall *firewall);
+
+/**
+ * stm32_firewall_release_access - Release access granted from a call to
+ *				   stm32_firewall_grant_access().
+ *
+ * @firewall			Firewall reference containing the ID to check against its firewall
+ *				controller
+ */
+void stm32_firewall_release_access(struct stm32_firewall *firewall);
+
+/**
+ * stm32_firewall_grant_access_by_id - Request firewall access rights of a given device
+ *				       based on a specific firewall ID
+ *
+ * Warnings:
+ * There is no way to ensure that the given ID will correspond to the firewall referenced in the
+ * device node if the ID did not come from stm32_firewall_get_firewall(). In that case, this
+ * function must be used with caution.
+ * This function should be used for subsystem resources that do not have the same firewall ID
+ * as their parent.
+ * U32_MAX is an invalid ID.
+ *
+ * @firewall			Firewall reference containing the firewall controller
+ * @subsystem_id		Firewall ID of the subsystem resource
+ *
+ * Returns 0 if access is granted, -EACCES if access is denied, -ENODEV if firewall is null or
+ * appropriate errno code if error occurred
+ */
+int stm32_firewall_grant_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id);

Can we store all registered IDs that were found by populate_bus
functio, or is it expected when this function was called before
populate_bus call?


This function is intended to be used by devices that needs to check
specific ID. Maybe IDs of peripherals that do not have a driver in
Linux / driver isn't probed / ...

About storing all IDs found when populating the bus, do you have some
use in mind? :) Else I'd prefer to save some memory.

Best regards,
Gatien

+
+/**
+ * stm32_firewall_release_access_by_id - Release access granted from a call to
+ *					 stm32_firewall_grant_access_by_id().
+ *
+ * Warnings:
+ * There is no way to ensure that the given ID will correspond to the firewall referenced in the
+ * device node if the ID did not come from stm32_firewall_get_firewall(). In that case, this
+ * function must be used with caution.
+ * This function should be used for subsystem resources that do not have the same firewall ID
+ * as their parent.
+ * U32_MAX is an invalid ID.
+ *
+ * @firewall			Firewall reference containing the firewall controller
+ * @subsystem_id		Firewall ID of the subsystem resource
+ */
+void stm32_firewall_release_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id);
+
+#else /* CONFIG_STM32_FIREWALL */
+
+int stm32_firewall_get_firewall(struct device_node *np, struct stm32_firewall *firewall)
+{
+	return -ENODEV;
+}
+
+int stm32_firewall_grant_access(struct stm32_firewall *firewall)
+{
+	return -ENODEV;
+}
+
+void stm32_firewall_release_access(struct stm32_firewall *firewall)
+{
+}
+
+int stm32_firewall_grant_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id)
+{
+	return -ENODEV;
+}
+
+void stm32_firewall_release_access_by_id(struct stm32_firewall *firewall, u32 subsystem_id)
+{
+}
+
+#endif /* CONFIG_STM32_FIREWALL */
+#endif /* STM32_FIREWALL_DEVICE_H */





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux