Messages in this thread | | | Date | Fri, 7 Jul 2023 17:01:06 +0200 | Subject | Re: [PATCH 05/10] firewall: introduce stm32_firewall framework | From | Gatien CHEVALLIER <> |
| |
Hello Oleksii,
On 7/7/23 15:50, Oleksii Moisieiev wrote: > > Gatien Chevallier <gatien.chevallier@foss.st.com> 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@foss.st.com> >> --- >> 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@foss.st.com> >> +S: Maintained >> +F: drivers/bus/stm32_firewall.c >> + >> ST STM32 I2C/SMBUS DRIVER >> M: Pierre-Yves MORDRET <pierre-yves.mordret@foss.st.com> >> M: Alain Volmat <alain.volmat@foss.st.com> >> 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 */ > >
| |