Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: stm32_gpio: register firewall controllers for GPIO access rights #7102

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

etienne-lms
Copy link
Contributor

This P-R makes stm32mp platforms to OP-TEE secure DTB to manage STM32 GPIO access rights. It tried to nicely split changes but it now spreads on almost 20 commits.

  • The series start with 7 patches in stm32mp platform DT source files to explicitly describe GPIOs and pins used by OP-TEE but accessible from non-secure world.
    "dt-bindings: gpio: stm32mp: flags for non-secure GPIOs"
    "dt-bindings: pinctrl: stm32mp: flags for non-secure pins"
    "dts: stm32: ..."
  • Follows 5 patches to implement firewall support in stm32_gpio driver, for stm32mp1 and stm32mp2 platforms.
    "drivers: stm32_gpio: check GPIO is not already consumed"
    "drivers: stm32_gpio: factorize apply_rif_config()"
    "drivers: stm32_gpio: register to firewall framework"
    "drivers: stm32_gpio: check secure state of consumed GPIOs"
    "drivers: stm32_gpio: check secure state of pinctrl states"
  • The series ends with 7 patches that removes deprecated shared_resource driver implementation related to STM32 pin muxing.
    "drivers: stm32_xxx: remove use of xxx() ..."
    "plat-stm32mp1: remove use of xxx() ..."

@etienne-lms
Copy link
Contributor Author

Rebased to fix merge conflicts.

CI / Code Style checkpatch reports are all false positive:
trace message exceeding 80char/line + BIT() not used in DT bindings header file.

@GseoC
Copy link
Contributor

GseoC commented Nov 7, 2024

7 first patches (device tree):

  • For dts: stm32: define SoC GPIO banks that are firewall controllers, in commit message: register to the firewall framework as a firewall controller
  • Some comments to address

@etienne-lms
Copy link
Contributor Author

Comments on the 7 first commits addressed.

CI / Code style reports false positive (trace message impl. exceeding 80char/line + use BIT() in DT binding header file).

CI / make check (QEMUv8, Clang) failed on optee_rust_examples_ext-1.0 build:

2024-11-12T10:29:03.1227175Z error: failed to get `uuid` as a dependency of package `ta v0.2.0 (/__w/optee_os/optee_repo_qemu_v8/out-br/build/optee_rust_examples_ext-1.0/examples/digest-rs/ta)`
(...)
2024-11-12T10:29:03.1232776Z Caused by:
2024-11-12T10:29:03.1233923Z   [7] Couldn't connect to server (Failed to connect to index.crates.io port 443 after 0 ms: Couldn't connect to server)

@@ -753,7 +754,7 @@ static TEE_Result apply_rif_config(struct stm32_gpio_bank *bank)
panic();

for (i = 0; i < bank->ngpios; i++) {
if (!(BIT(i) & bank->rif_cfg->access_mask[0]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this removed? So that we allow reconfiguration of pins that we don't list in the DT RIF config at first place? It's not what we have at downstream

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits set in gpios_mask are also set in bank->rif_cfg->access_mask[0] (see caller functions) hence testing gpios_mask is enough IMHO. Maybe I should add an assertion at function entry?
assert(gpios_mask & bank->rif_cfg->access_mask[0] == gpios_mask);

core/drivers/stm32_gpio.c Show resolved Hide resolved
core/drivers/stm32_gpio.c Outdated Show resolved Hide resolved
* Non RIF GPIO banks use TZPROT() bit mask (bits 0 to 15)
* and GPIO_STM32_NSEC bit flag in the DT bindings.
*/
gpios_mask = firewall->args[0] & GENMASK_32(15, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the non-RIF case, we allow multiple pin reconfiguration. We forbid that for RIF-capable banks. I think the function should have the same use, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that STM32MP1 DT bindings allow a single firewall config/query to confgure several GPIO pins whereas STM32 RIF DT bindings (STM32MP2 GPIO) require 1 config/query per GPIO pin?
If so, I intentionally made it that way to better conform with STM32 DT binding st,protreg that defines the default configuration of the GPIOs secure hardening. For STM32 RIF protected GPIOS, st,proret requires 1 DT 32bit cell per GPIO whereas in STM32MP1 protected GPIO, the DT binding allows to configure several GPIOs with a single 32bit DT cell.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is what I meant. I think this should be mentionned in a comment somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think the inline comment above (lines 1025/1026) is not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have rephrased the inline comment.

core/drivers/stm32_gpio.c Outdated Show resolved Hide resolved
fdt_get_name(fdt, consumer_node, NULL),
bank + 'A', pin);
do_panic = true;
} else if (!pin_is_accessible(bank_ref, pin)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pin_is_accessible() should also be checked when pinctrl is applied IMO. Same story as the secure state, it can be reconfigured in the meanswhile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I'll fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem right. If the GPIO(s) are in semaphore, the semaphores will be taken whilst we don't apply the pinctrl, therefore don't consume it. I would remove this part. As I've said in my previous comment, if the CPU is not TDCID, we have no clue if the RIF configuration has been changed between getting the pinctrl and applying it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've added acquire_rif_semaphore_if_needed() local helper functions for that. See the appended fixup commit.

for (n = 0; n < pin_count; n++) {
struct stm32_gpio_bank *bank = stm32_gpio_get_bank(p[n].bank);

if (p[n].cfg.nsec == !pin_is_secure(bank, p[n].pin))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think:

		if (pin_is_secure(bank, p[n].pin) && p[n].cfg.nsec)
			continue;

is clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 cases to consider:

		if ((pin_is_secure(bank, p[n].pin) && !p[n].cfg.nsec) ||
		    (!pin_is_secure(bank, p[n].pin) && p[n].cfg.nsec))

I though it would be simpler on a single line.

core/drivers/stm32_gpio.c Outdated Show resolved Hide resolved
@@ -113,19 +113,13 @@ static void register_secure_uart(struct stm32_uart_pdata *pd __maybe_unused)
{
#ifndef CFG_STM32MP25
stm32mp_register_secure_periph_iomem(pd->base.pa);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at our static mapping, I don't think we need this as well, don't you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't, but let's address that is a specific series/P-R if you don't mind.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@GseoC
Copy link
Contributor

GseoC commented Nov 12, 2024

For commit core: stm32_gpio: register firewall controllers for GPIO access rights
s/STM32 GPIO driver verifies/ STM32 GPIO driver now verifies

ditto for following pinctrl commit

For commits:
drivers: stm32_i2c: remove use of stm32_pinctrl_set_secure_cfg()
drivers: stm32_uart: remove use of stm32_pinctrl_set_secure_cfg()
In the commit message, it's not the ETZPC driver, it's rather the GPIO one

For commit: drivers: stm32_uart: remove use of shared_resource for pincltr
typo in commit title

Copy link
Contributor Author

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review comments addressed or answered.
I'll rebase the series to resolve some rebase conflicts.

@@ -753,7 +754,7 @@ static TEE_Result apply_rif_config(struct stm32_gpio_bank *bank)
panic();

for (i = 0; i < bank->ngpios; i++) {
if (!(BIT(i) & bank->rif_cfg->access_mask[0]))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits set in gpios_mask are also set in bank->rif_cfg->access_mask[0] (see caller functions) hence testing gpios_mask is enough IMHO. Maybe I should add an assertion at function entry?
assert(gpios_mask & bank->rif_cfg->access_mask[0] == gpios_mask);

core/drivers/stm32_gpio.c Show resolved Hide resolved
core/drivers/stm32_gpio.c Outdated Show resolved Hide resolved
* Non RIF GPIO banks use TZPROT() bit mask (bits 0 to 15)
* and GPIO_STM32_NSEC bit flag in the DT bindings.
*/
gpios_mask = firewall->args[0] & GENMASK_32(15, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that STM32MP1 DT bindings allow a single firewall config/query to confgure several GPIO pins whereas STM32 RIF DT bindings (STM32MP2 GPIO) require 1 config/query per GPIO pin?
If so, I intentionally made it that way to better conform with STM32 DT binding st,protreg that defines the default configuration of the GPIOs secure hardening. For STM32 RIF protected GPIOS, st,proret requires 1 DT 32bit cell per GPIO whereas in STM32MP1 protected GPIO, the DT binding allows to configure several GPIOs with a single 32bit DT cell.

core/drivers/stm32_gpio.c Outdated Show resolved Hide resolved
fdt_get_name(fdt, consumer_node, NULL),
bank + 'A', pin);
do_panic = true;
} else if (!pin_is_accessible(bank_ref, pin)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I'll fix.

for (n = 0; n < pin_count; n++) {
struct stm32_gpio_bank *bank = stm32_gpio_get_bank(p[n].bank);

if (p[n].cfg.nsec == !pin_is_secure(bank, p[n].pin))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 cases to consider:

		if ((pin_is_secure(bank, p[n].pin) && !p[n].cfg.nsec) ||
		    (!pin_is_secure(bank, p[n].pin) && p[n].cfg.nsec))

I though it would be simpler on a single line.

core/drivers/stm32_gpio.c Outdated Show resolved Hide resolved
@@ -113,19 +113,13 @@ static void register_secure_uart(struct stm32_uart_pdata *pd __maybe_unused)
{
#ifndef CFG_STM32MP25
stm32mp_register_secure_periph_iomem(pd->base.pa);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't, but let's address that is a specific series/P-R if you don't mind.

@etienne-lms
Copy link
Contributor Author

Rebased

@etienne-lms
Copy link
Contributor Author

@GseoC, may I squash the fixup commits and fix the commit messages as you suggested?

core/drivers/stm32_gpio.c Outdated Show resolved Hide resolved
@GseoC
Copy link
Contributor

GseoC commented Nov 15, 2024

@GseoC, may I squash the fixup commits and fix the commit messages as you suggested?

Please do when addressing the comment above so that I can make another round, thanks!

@etienne-lms
Copy link
Contributor Author

Fixup commits squashed and commits messages updated as per #7102 (comment) and #7102 (comment) review comments.

Define STM32 GPIO DT bindings bit flags for GPIOs that are to be used
in non-secure state.

Signed-off-by: Etienne Carriere <[email protected]>
Define stm32 pinctrl DT bindings bit flags for pins that are
expected to be used in non-secure state.

Signed-off-by: Etienne Carriere <[email protected]>
Add property #access-controller-cells to GPIO banks that register
to the firewall framework.

Signed-off-by: Etienne Carriere <[email protected]>
On STM32MP15 based devices, UART2/3/4/5/6/7/8 cannot be secured.
Explicitly state that in the pinctrl nodes. This change ease the use
of a non-secure UART for OP-TEE output console on STM32MP15 based boards.

Signed-off-by: Etienne Carriere <[email protected]>
Explicitly state that legacy pinctrl phandles i2c4_pins_a and
i2c4_sleep_pins_a refer to non-secure I2C4 pin muxing on STM32MP15
based platforms.

Define secure I2C4 bus pinctrl states for boards that use the I2C4 bus
in secure state on STM32MP15 SoCs.

Signed-off-by: Etienne Carriere <[email protected]>
Explicitly state that legacy pinctrl phandles usart4_pins_a
refer to non-secure USART4 pin muxing, used in STM32MP13 based
boards for OP-TEE console using a non-secure UART bus.

Signed-off-by: Etienne Carriere <[email protected]>
Explicitly state that legacy pinctrl phandles usart2_pins_a
refer to non-secure USART2 pin muxing, used in STM32MP23 and
STM32MP25 based boards for OP-TEE console using a non-secure UART bus.

Define secure USART2 bus pinctrl states for board that needs
to use the USART2 bus in secure state.

Signed-off-by: Etienne Carriere <[email protected]>
Check that a GPIO requested by a consumer is not already consumed by
another device.

Signed-off-by: Etienne Carriere <[email protected]>
Change apply_rif_config() to be able to call it for a subset of pins
in a GPIO bank.

Signed-off-by: Etienne Carriere <[email protected]>
Register secure aware STM32 GPIO banks to the firewall framework
as a firewall controller to allow GPIO and pinctrl consumer devices
to load alternate configurations for pins.

Signed-off-by: Etienne Carriere <[email protected]>
STM32 GPIO driver now verifies that any GPIO consumed by OP-TEE can
be accessed and has the expected secure hardening configuration.

If a driver attempts to consume a GPIO that cannot be accessed
by OP-TEE, core panics.

Use newly added GPIO_STM32_NSEC bindings macro in STM32 GPIO
driver DT bindings header file as hint on whether GPIO is expected
secure or shared with non-secure world.

When a GPIO is used with an inappropriate configuration state,
STM32 GPIO driver panics or prints an info level message, depending
on CFG_INSECURE.

Signed-off-by: Etienne Carriere <[email protected]>
STM32 GPIO driver now verifies that any pinctrl consumed by OP-TEE can
be accessed and has the expected secure hardening configuration.

If a driver attempts to consume a pinctrl with pins that cannot be
accessed by OP-TEE, core panics.

Non-secure pins must have the STM32_PIN_NSEC bit set in the pin
handler argument unless what the pin is expected to be secure. The
driver returns an error when the expected secure state of a pin does
not match its effective secure state, unless CFG_INSECURE is enabled
in which case the driver only prints an info level trace message.

Signed-off-by: Etienne Carriere <[email protected]>
Remove management of GPIO and pinctrl secure state since this is
now handled from STM32 ETZPC driver based through the firewall framework.

Signed-off-by: Etienne Carriere <[email protected]>
Remove use of stm32_pinctrl_set_secure_cfg() to set the secure state
of the pins of a pinctrl state since this is now handled from STM32
GPIO driver based on the firewall framework.

Signed-off-by: Etienne Carriere <[email protected]>
Remove use of stm32_pinctrl_set_secure_cfg() to set the secure state
of the pins of a pinctrl state since this is now handled from STM32
GPIO driver based on the firewall framework.

Signed-off-by: Etienne Carriere <[email protected]>
Remove use of shared_resources platform driver to manage the secure
state of the pins of a pinctrl state since this is now managed using
the firewall framework.

Signed-off-by: Etienne Carriere <[email protected]>
…ctrl

Remove use of shared_resources platform driver in STM32MP15 PMIC driver
to manage the secure state of the pins of a pinctrl state since this is
now managed using the firewall framework.

Signed-off-by: Etienne Carriere <[email protected]>
…tate

Remove stm32_gpio_set_secure_cfg() and stm32_pinctrl_set_secure_cfg()
functions that are no more used since the GPIO and pins secure state
is defined by the st,protreg registers  already handled from
get_pinctrl_from_fdt() and stm32_gpio_get_dt() callback functions.

Signed-off-by: Etienne Carriere <[email protected]>
Remove the pin and GPIO secure state management from shared_resources
platform driver since this is now managed using the firewall framework.

Signed-off-by: Etienne Carriere <[email protected]>
@etienne-lms
Copy link
Contributor Author

Rebased to solve merge conflicts.

id - STM32MP1_SHRES_GPIOZ(0), get_gpioz_nbpin());
panic();
}
panic("Deprecated registering of GPIOz resources");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/GPIOz/GPIOZ

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could merge with PLL3 and specify only usage of deprecated shared resource

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to send another series to fully remove this plat-stm32mp1/shared_resources.c driver once this P-R and #7111 are merged so all these line will then disappear. It's something we referred to in this comment.

@@ -113,19 +113,13 @@ static void register_secure_uart(struct stm32_uart_pdata *pd __maybe_unused)
{
#ifndef CFG_STM32MP25
stm32mp_register_secure_periph_iomem(pd->base.pa);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

/* Resource can be accessed if CID1 is statically allowed */
accessible = true;
} else if (stm32_rif_semaphore_enabled_and_ok(cidcfgr, RIF_CID1)) {
/* We must acquire the semaphore to access the resource */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a stm32_rif_semaphore_is_available_or_taken() to check if the current CID has already taken the semaphore. This will avoid some issue when acquiring the semaphore (spurious IAC). Maybe we can do this in another P-R. (I can take care of that)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent such issues when acquiring an already taken semaphore, I think we should rather fix stm32_rif_acquire_semaphore():

 TEE_Result stm32_rif_acquire_semaphore(vaddr_t addr, unsigned int nb_cid_supp)
 {
 	uint32_t scid_mask = get_scid_mask(nb_cid_supp);
 
-	/* Take the semaphore */
-	io_setbits32(addr, _SEMCR_MUTEX);
+	/* Take the semaphore is not already taken */
+	if (stm32_rif_semaphore_is_available(addr))
+		io_setbits32(addr, _SEMCR_MUTEX);

	/* Check that the Cortex-A has the semaphore */
	if (stm32_rif_semaphore_is_available(addr) ||
	    ((io_read32(addr) & scid_mask) >> _CIDCFGR_SCID_SHIFT) != RIF_CID1)
		return TEE_ERROR_ACCESS_DENIED;

	return TEE_SUCCESS;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the acquisition of the semaphore here pin_is_accessible() and added it in stm32_pinctrl_conf_apply() and stm32_gpio_get_dt(). See the appended fixup commit.

fdt_get_name(fdt, consumer_node, NULL),
bank + 'A', pin);
do_panic = true;
} else if (!pin_is_accessible(bank_ref, pin)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem right. If the GPIO(s) are in semaphore, the semaphores will be taken whilst we don't apply the pinctrl, therefore don't consume it. I would remove this part. As I've said in my previous comment, if the CPU is not TDCID, we have no clue if the RIF configuration has been changed between getting the pinctrl and applying it.

}
}

if (!pin_is_accessible(bank, gpio->pin)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of the same fight here. I think we should "acquire" the access when consuming the GPIO. That probably is here or in each of the exposed APIs:

	.get_direction = stm32_gpio_get_direction,
	.set_direction = stm32_gpio_set_direction,
	.get_value = stm32_gpio_get_level,
	.set_value = stm32_gpio_set_level,

but we also need to release the access when we put the GPIO in stm32_gpio_put_gpio().

We don't have a pinctrl release

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect GPIOs consumed by OP-TEE are only owned by OP-TEE. We have no current plan on stm32mpX platforms to share GPIOs at runtime.

* Non RIF GPIO banks use TZPROT() bit mask (bits 0 to 15)
* and GPIO_STM32_NSEC bit flag in the DT bindings.
*/
gpios_mask = firewall->args[0] & GENMASK_32(15, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is what I meant. I think this should be mentionned in a comment somewhere

Fix inline description comment.

Signed-off-by: Etienne Carriere <[email protected]>
Change pin_is_accessible() to not take the semaphore so that
pinctrl consumers will only the required semaphore when applying
the pinctrl state.

Add local helper functions to specifically acquire and release
RIF semaphores: acquire_rif_semaphore_if_needed() and
release_rif_semaphore_if_acquired().

Since GPIOs are consumed when stm32_gpio_get_dt() is called, and
released when stm32_gpio_put_gpio(), call the new help functions
from these handlers.

Signed-off-by: Etienne Carriere <[email protected]>
Acquire RIF semaphore only when a pinctrl state is applied, not when
the consumer gets the pinctrl state reference from the DT during its
initialization sequence.

Signed-off-by: Etienne Carriere <[email protected]>
…e management

Fix panic trace message.

Signed-off-by: Etienne Carriere <[email protected]>
Release RIF semaphore taken at GPIO bank initialization since they
are to be taken only when the GPIO or pinctrl is used or when a
firewall configuration is requested.

Signed-off-by: Etienne Carriere <[email protected]>
@etienne-lms
Copy link
Contributor Author

Comments addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants