| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 26 Jul 2023 12:38:00 +0200
From: Gatien CHEVALLIER <gatien.chevallier@...s.st.com>
To: Simon Horman <simon.horman@...igine.com>
CC: <Oleksii_Moisieiev@...m.com>, <gregkh@...uxfoundation.org>,
<herbert@...dor.apana.org.au>, <davem@...emloft.net>,
<robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
<conor+dt@...nel.org>, <alexandre.torgue@...s.st.com>,
<vkoul@...nel.org>, <jic23@...nel.org>, <olivier.moysan@...s.st.com>,
<arnaud.pouliquen@...s.st.com>, <mchehab@...nel.org>,
<fabrice.gasnier@...s.st.com>, <andi.shyti@...nel.org>,
<ulf.hansson@...aro.org>, <edumazet@...gle.com>, <kuba@...nel.org>,
<pabeni@...hat.com>, <hugues.fruchet@...s.st.com>, <lee@...nel.org>,
<will@...nel.org>, <catalin.marinas@....com>, <arnd@...nel.org>,
<richardcochran@...il.com>, Frank Rowand <frowand.list@...il.com>,
<linux-crypto@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<dmaengine@...r.kernel.org>, <linux-i2c@...r.kernel.org>,
<linux-iio@...r.kernel.org>, <alsa-devel@...a-project.org>,
<linux-media@...r.kernel.org>, <linux-mmc@...r.kernel.org>,
<netdev@...r.kernel.org>, <linux-phy@...ts.infradead.org>,
<linux-serial@...r.kernel.org>, <linux-spi@...r.kernel.org>,
<linux-usb@...r.kernel.org>
Subject: Re: [PATCH v2 05/11] firewall: introduce stm32_firewall framework
On 7/26/23 12:19, Simon Horman wrote:
> On Tue, Jul 25, 2023 at 06:40:58PM +0200, Gatien Chevallier wrote:
>> Introduce a STM32 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 STM32 firewall framework offers a generic API for STM32 firewall
>> controllers that is defined in their 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
>> -No type for undefined type of firewall
>>
>> Signed-off-by: Gatien Chevallier <gatien.chevallier@...s.st.com>
>
> ...
>
>> diff --git a/drivers/bus/stm32_firewall.c b/drivers/bus/stm32_firewall.c
>
> ...
>
>> +int stm32_firewall_populate_bus(struct stm32_firewall_controller *firewall_controller)
>> +{
>> + struct stm32_firewall *firewalls;
>> + struct device_node *child;
>> + struct device *parent;
>> + unsigned int i;
>> + int len;
>> + 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) {
>> + /* The feature-domains property is mandatory for firewall bus devices */
>> + len = of_count_phandle_with_args(child, "feature-domains", "#feature-domain-cells");
>> + if (len <= 0)
>
> Coccinelle says that, due to breaking out of a
> for_each_available_child_of_node() loop, a call to of_node_put()
> is needed here
>
Hi Simon,
Thank you, I already sent a V3 correcting the patch ordering issue. I
will implement this for V4.
>> + return -EINVAL;
>> +
>> + firewalls = kcalloc(len, sizeof(*firewalls), GFP_KERNEL);
>> + if (!firewalls)
>
> And here.
>
ditto
>> + return -ENOMEM;
>> +
>> + err = stm32_firewall_get_firewall(child, firewalls, (unsigned int)len);
>> + if (err) {
>> + kfree(firewalls);
>
> And here.
>
ditto
>> + return err;
>> + }
>> +
>> + for (i = 0; i < len; i++) {
>> + if (firewall_controller->grant_access(firewall_controller,
>> + firewalls[i].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);
>> + }
>> + }
>> +
>> + kfree(firewalls);
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(stm32_firewall_populate_bus);
>
>> diff --git a/drivers/bus/stm32_firewall.h b/drivers/bus/stm32_firewall.h
>
> ...
>
>> +/**
>> + * struct stm32_firewall_controller - Information on firewall controller supplying services
>> + *
>> + * @name Name of the firewall controller
>
> kernel-doc complains that name and the other fields of
> struct stm32_firewall_controller are not documented.
> I believe this is because a ':' is needed after the name of
> the parameter (in this case 'name').
>
> * @name: Name of the firewall controller
>
> Likewise, elsewhere.
>
I will implement it in V4, thank you.
>> + * @dev Device reference of the firewall controller
>> + * @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
>> + * @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
>
> kernel-doc seems unhappy about the presence of 'int' on this line.
>
> * stm32_firewall_controller_register - Register a firewall controller to the STM32 firewall
>
> Likewise, elsewhere.
>
Yes, I will remove the type in V4.
>> + * 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);
>
> ...
>
>> diff --git a/include/linux/bus/stm32_firewall_device.h b/include/linux/bus/stm32_firewall_device.h
>> new file mode 100644
>> index 000000000000..9bdc4060154c
>> --- /dev/null
>> +++ b/include/linux/bus/stm32_firewall_device.h
>> @@ -0,0 +1,140 @@
>> +/* 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;
>> +
>> +/**
>> + * stm32_firewall - Information on a device's firewall. Each device can have more than one firewall.
>
> kernel-doc seems unhappy about the absence of struct on this line.
>
> * struct stm32_firewall - Information on a device's firewall. Each device can have more than one firewall.
>
Yes, I will add the struct in V4.
>> + *
>> + * @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;
>> +};
>
> ...
>
Best regards,
Gatien
Powered by blists - more mailing lists