| Message ID | 20230811100731.108145-1-gatien.chevallier@foss.st.com |
|---|---|
| Headers | show |
| Series | Introduce STM32 Firewall framework | expand |
On 8/11/23 12:16, Greg KH wrote: > On Fri, Aug 11, 2023 at 12:07:21PM +0200, Gatien Chevallier wrote: >> From: Oleksii Moisieiev <Oleksii_Moisieiev@epam.com> >> >> Introducing of the common device controller bindings for the controller >> provider and consumer devices. Those bindings are intended to allow >> divided system on chip into multiple domains, that can be used to >> configure hardware permissions. >> >> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com> >> [Gatien: Fix typos and YAML error] >> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com> >> --- >> >> Changes in V4: >> Corrected typos and YAML errors > > Why are we supposed to ignore the first patch in this series, but pay > attention to the 10 after this that depend on it? > > totally confused, > > greg k-h Hello Greg, I'm sorry that this tag troubles your review. It was first suggested in [1]. The "IGNORE" means ignore review on this thread, as it is still under review in another thread (Link in the cover letter). It does not mean that the content should be ignored for the series. I will change this to something else as this is obviously confusing the review. @Oleksii, can we imagine integrating this patch to this series or do you prefer to keep it apart? Should I consider a resend with another tag if Oleksii prefers to keep this patch apart? [1] https://lore.kernel.org/all/1e498b93-d3bd-bd12-e991-e3f4bedf632d@linaro.org/ Best regards, Gatien
On Fri, Aug 11, 2023 at 12:07:25PM +0200, Gatien Chevallier wrote: ... > diff --git a/drivers/bus/stm32_firewall.c b/drivers/bus/stm32_firewall.c > new file mode 100644 > index 000000000000..900f3b052a66 > --- /dev/null > +++ b/drivers/bus/stm32_firewall.c > @@ -0,0 +1,293 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * 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 <linux/slab.h> > + > +#include "stm32_firewall.h" > + > +/* Corresponds to STM32_FIREWALL_MAX_EXTRA_ARGS + firewall ID */ > +#define STM32_FIREWALL_MAX_ARGS (STM32_FIREWALL_MAX_EXTRA_ARGS + 1) > + > +static LIST_HEAD(firewall_controller_list); > +static DEFINE_MUTEX(firewall_controller_list_lock); > + > +/* Firewall device API */ > +int stm32_firewall_get_firewall(struct device_node *np, struct stm32_firewall *firewall, > + unsigned int nb_firewall) > +{ > + struct stm32_firewall_controller *ctrl; > + struct of_phandle_iterator it; > + unsigned int i, j = 0; > + int err; > + > + if (!firewall || !nb_firewall) > + return -EINVAL; > + > + /* Parse property with phandle parsed out */ > + of_for_each_phandle(&it, err, np, "feature-domains", "#feature-domain-cells", 0) { > + struct of_phandle_args provider_args; > + struct device_node *provider = it.node; > + const char *fw_entry; > + bool match = false; > + > + if (err) { > + pr_err("Unable to get feature-domains property for node %s\n, err: %d", > + np->full_name, err); > + of_node_put(provider); > + return err; > + } > + > + if (j > nb_firewall) { > + pr_err("Too many firewall controllers"); > + of_node_put(provider); > + return -EINVAL; > + } > + > + provider_args.args_count = of_phandle_iterator_args(&it, provider_args.args, > + STM32_FIREWALL_MAX_ARGS); > + > + /* 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 == it.phandle) { > + match = true; > + firewall[j].firewall_ctrl = ctrl; > + break; > + } > + } > + mutex_unlock(&firewall_controller_list_lock); > + > + if (!match) { > + firewall[j].firewall_ctrl = NULL; > + pr_err("No firewall controller registered for %s\n", np->full_name); > + of_node_put(provider); > + return -ENODEV; > + } > + > + err = of_property_read_string_index(np, "feature-domain-names", j, &fw_entry); > + if (err == 0) > + firewall[j].entry = fw_entry; > + > + /* Handle the case when there are no arguments given along with the phandle */ > + if (provider_args.args_count < 0 || > + provider_args.args_count > STM32_FIREWALL_MAX_ARGS) { > + of_node_put(provider); > + return -EINVAL; > + } else if (provider_args.args_count == 0) { > + firewall[j].extra_args_size = 0; > + firewall[j].firewall_id = U32_MAX; > + j++; > + continue; > + } > + > + /* The firewall ID is always the first argument */ > + firewall[j].firewall_id = provider_args.args[0]; > + > + /* Extra args start at the third argument */ > + for (i = 0; i < provider_args.args_count; i++) > + firewall[j].extra_args[i] = provider_args.args[i + 1]; Hi Gatien, Above it is checked that the maximum value of provider_args.args_count is STM32_FIREWALL_MAX_ARGS. So here the maximum value of i is STM32_FIREWALL_MAX_ARGS - 1. STM32_FIREWALL_MAX_ARGS is defined as STM32_FIREWALL_MAX_EXTRA_ARGS + 1 And STM32_FIREWALL_MAX_EXTRA_ARGS is defined as 5. So the maximum value of i is (5 + 1 - 1) = 5. firewall[j] is of type struct stm32_firewall. And its args field has STM32_FIREWALL_MAX_EXTRA_ARGS (5) elements. Thus the maximum valid index is (5 - 1) = 4. But the line above may access index 5. Flagged by Smatch. > + > + /* Remove the firewall ID arg that is not an extra argument */ > + firewall[j].extra_args_size = provider_args.args_count - 1; > + > + j++; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(stm32_firewall_get_firewall); ... > diff --git a/include/linux/bus/stm32_firewall_device.h b/include/linux/bus/stm32_firewall_device.h > new file mode 100644 > index 000000000000..7b4450a8ec15 > --- /dev/null > +++ b/include/linux/bus/stm32_firewall_device.h > @@ -0,0 +1,141 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * 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 > + > +/* Opaque reference to stm32_firewall_controller */ > +struct stm32_firewall_controller; > + > +/** > + * struct 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 > + * @entry: Name of the firewall entry > + * @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]; > + const char *entry; > + size_t extra_args_size; > + u32 firewall_id; > +}; ...
On 8/12/23 16:09, Simon Horman wrote: > On Fri, Aug 11, 2023 at 12:07:25PM +0200, Gatien Chevallier wrote: > > ... > >> diff --git a/drivers/bus/stm32_firewall.c b/drivers/bus/stm32_firewall.c >> new file mode 100644 >> index 000000000000..900f3b052a66 >> --- /dev/null >> +++ b/drivers/bus/stm32_firewall.c >> @@ -0,0 +1,293 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * 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 <linux/slab.h> >> + >> +#include "stm32_firewall.h" >> + >> +/* Corresponds to STM32_FIREWALL_MAX_EXTRA_ARGS + firewall ID */ >> +#define STM32_FIREWALL_MAX_ARGS (STM32_FIREWALL_MAX_EXTRA_ARGS + 1) >> + >> +static LIST_HEAD(firewall_controller_list); >> +static DEFINE_MUTEX(firewall_controller_list_lock); >> + >> +/* Firewall device API */ >> +int stm32_firewall_get_firewall(struct device_node *np, struct stm32_firewall *firewall, >> + unsigned int nb_firewall) >> +{ >> + struct stm32_firewall_controller *ctrl; >> + struct of_phandle_iterator it; >> + unsigned int i, j = 0; >> + int err; >> + >> + if (!firewall || !nb_firewall) >> + return -EINVAL; >> + >> + /* Parse property with phandle parsed out */ >> + of_for_each_phandle(&it, err, np, "feature-domains", "#feature-domain-cells", 0) { >> + struct of_phandle_args provider_args; >> + struct device_node *provider = it.node; >> + const char *fw_entry; >> + bool match = false; >> + >> + if (err) { >> + pr_err("Unable to get feature-domains property for node %s\n, err: %d", >> + np->full_name, err); >> + of_node_put(provider); >> + return err; >> + } >> + >> + if (j > nb_firewall) { >> + pr_err("Too many firewall controllers"); >> + of_node_put(provider); >> + return -EINVAL; >> + } >> + >> + provider_args.args_count = of_phandle_iterator_args(&it, provider_args.args, >> + STM32_FIREWALL_MAX_ARGS); >> + >> + /* 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 == it.phandle) { >> + match = true; >> + firewall[j].firewall_ctrl = ctrl; >> + break; >> + } >> + } >> + mutex_unlock(&firewall_controller_list_lock); >> + >> + if (!match) { >> + firewall[j].firewall_ctrl = NULL; >> + pr_err("No firewall controller registered for %s\n", np->full_name); >> + of_node_put(provider); >> + return -ENODEV; >> + } >> + >> + err = of_property_read_string_index(np, "feature-domain-names", j, &fw_entry); >> + if (err == 0) >> + firewall[j].entry = fw_entry; >> + >> + /* Handle the case when there are no arguments given along with the phandle */ >> + if (provider_args.args_count < 0 || >> + provider_args.args_count > STM32_FIREWALL_MAX_ARGS) { >> + of_node_put(provider); >> + return -EINVAL; >> + } else if (provider_args.args_count == 0) { >> + firewall[j].extra_args_size = 0; >> + firewall[j].firewall_id = U32_MAX; >> + j++; >> + continue; >> + } >> + >> + /* The firewall ID is always the first argument */ >> + firewall[j].firewall_id = provider_args.args[0]; >> + >> + /* Extra args start at the third argument */ >> + for (i = 0; i < provider_args.args_count; i++) >> + firewall[j].extra_args[i] = provider_args.args[i + 1]; > > Hi Gatien, > > Above it is checked that the maximum value of provider_args.args_count is > STM32_FIREWALL_MAX_ARGS. > So here the maximum value of i is STM32_FIREWALL_MAX_ARGS - 1. > > STM32_FIREWALL_MAX_ARGS is defined as STM32_FIREWALL_MAX_EXTRA_ARGS + 1 > And STM32_FIREWALL_MAX_EXTRA_ARGS is defined as 5. > So the maximum value of i is (5 + 1 - 1) = 5. > > firewall[j] is of type struct stm32_firewall. > And its args field has STM32_FIREWALL_MAX_EXTRA_ARGS (5) elements. > Thus the maximum valid index is (5 - 1) = 4. > > But the line above may access index 5. > > Flagged by Smatch. > Hi Simon, Thank you for pointing this out. I'll correct it for V5. Best regards, Gatien >> + >> + /* Remove the firewall ID arg that is not an extra argument */ >> + firewall[j].extra_args_size = provider_args.args_count - 1; >> + >> + j++; >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(stm32_firewall_get_firewall); > > ... > >> diff --git a/include/linux/bus/stm32_firewall_device.h b/include/linux/bus/stm32_firewall_device.h >> new file mode 100644 >> index 000000000000..7b4450a8ec15 >> --- /dev/null >> +++ b/include/linux/bus/stm32_firewall_device.h >> @@ -0,0 +1,141 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * 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 >> + >> +/* Opaque reference to stm32_firewall_controller */ >> +struct stm32_firewall_controller; >> + >> +/** >> + * struct 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 >> + * @entry: Name of the firewall entry >> + * @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]; >> + const char *entry; >> + size_t extra_args_size; >> + u32 firewall_id; >> +}; > > ...