plat-stm32mp1: scmi_server: test reset/clock access against ETZPC config #7111

etienne-lms · 2024-11-06T11:56:09Z

A series to change STM32MP1 SCMI server to check SCMI resource accessibility through the firewall API functions instead of deprecated platform local shared_resource.c driver.

etienne-lms · 2024-11-14T08:05:14Z

Rebased to solve merge conflicts.

Implement .check_access handler in stm32_etzpc driver. Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>

Check whether or not an SCMI clock or SCMI reset domain can be accessed using the firewall API instead of relying on shared_resources.c driver. This latter is not useless since integration of the firewall framework and will be soon removed. Remove also the buggy tests on SCMI reset being exposed that relied on wrong API function stm32mp_nsec_can_access_clock(). Test on reset domain being accessible or not is now dynamically handled with nsec_can_access_resource(). Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>

Remove unused platform functions stm32mp_nsec_can_access_clock() and stm32mp_gpio_bank_is_secure(). Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>

Remove unused platform functions stm32mp_nsec_can_access_reset() and stm32mp_gpio_bank_is_non_secure(). Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>

etienne-lms · 2024-11-14T17:01:49Z

Rebased (#7107 has been merged)

GseoC · 2024-11-15T14:38:23Z

core/arch/arm/plat-stm32mp1/scmi_server.c

@@ -179,38 +205,56 @@ static struct stm32_scmi_clk stm32_scmi_clock[] = {
 	CLOCK_CELL(CK_SCMI_MPU, CK_MPU, "ck_mpu", true),
 	CLOCK_CELL(CK_SCMI_AXI, CK_AXI, "ck_axi", true),
 	CLOCK_CELL(CK_SCMI_BSEC, BSEC, "bsec", true),
-	CLOCK_CELL(CK_SCMI_CRYP1, CRYP1, "cryp1", false),
+	CLOCK_CELL_DECPROT(CK_SCMI_CRYP1, CRYP1, "cryp1", false,


Not needed for clocks as there's apparently nothing malicious at letting a clock enabled.

I think it's still better to warn caller (U-Boot/Linux) in case it manipulates a clock it is not expected to.

GseoC · 2024-11-15T14:41:22Z

core/arch/arm/plat-stm32mp1/scmi_server.c

 };
 #endif

 #ifdef CFG_STM32MP13
 static struct stm32_scmi_rd stm32_scmi_reset_domain[] = {
-	RESET_CELL(RST_SCMI_LTDC, LTDC_R, "ltdc"),
+	RESET_CELL_DECPROT(RST_SCMI_LTDC, LTDC_R, "ltdc",
+			   STM32MP1_ETZPC_LTDC_ID),
 	RESET_CELL(RST_SCMI_MDMA, MDMA_R, "mdma"),


Maybe we shouldn't expose this reset as this peripheral is Trustzone aware (sec per channel) and I think there might be a software reset possible. Maybe add someone in the discussion?

We don't use MDMA in OP-TEE so we let non-secure world fully manage it, including its reset control.

We don't use MDMA in OP-TEE so we let non-secure world fully manage it, including its reset control.

agree

on MP15 it was required (shared resource not by OP-TEE but by M4)

So it was decided to be in secure world, see DT kernel, for example
arch/arm/boot/dts/st/stm32mp157c-ed1-scmi.dtsi:66: resets = <&scmi_reset RST_SCMI_MDMA>;

no needed for MP13 => MDMA reset is not handled in linux DT

mdma: dma-controller@58000000 { compatible = "st,stm32h7-mdma"; reg = <0x58000000 0x1000>; interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>; clocks = <&rcc MDMA>; #dma-cells = <5>; dma-channels = <32>; dma-requests = <48>; };

Maybe we shouldn't expose this reset as this peripheral is Trustzone aware (sec per channel) and I think there might be a software reset possible. Maybe add someone in the discussion?

in kernel, the SCMI reset is required to support secure UI / management of M4 layer

arch/arm/boot/dts/st/stm32mp135.dtsi:28: resets = <&scmi_reset RST_SCMI_LTDC>

today NOT decprot ( I think always accept the request as we have secure and unsecure layer

GseoC · 2024-11-15T14:45:59Z

core/arch/arm/plat-stm32mp1/scmi_server.c

+		struct dt_driver_provider *prov = NULL;
+		int node = 0;
+
+		node = fdt_node_offset_by_compatible(get_embedded_dt(), -1,


This is fine because we call the function at boot so we do not use this code at runtime but it's because we synchronize clocks in stm32mp1_init_scmi_server()

Indeed, we get the firewall controller reference (handle) at boot time to be able to use it a run time. You you prefer I explicitly state that in an inline function here?

I let you decide what's best. No strong opinion on what's clearer

GseoC · 2024-11-15T14:47:09Z

core/arch/arm/plat-stm32mp1/scmi_server.c

@@ -422,7 +499,7 @@ int32_t plat_scmi_clock_rates_array(unsigned int channel_id,
 	if (!clock)
 		return SCMI_NOT_FOUND;

-	if (!stm32mp_nsec_can_access_clock(clock->clock_id))
+	if (!nsec_can_access_resource(clock->fw_id))


Again, not sure we need to protect clock operations

See my response above.

patrickdelaunay · 2024-11-21T15:30:13Z

core/arch/arm/plat-stm32mp1/scmi_server.c

+			panic();
+
+		etzpc_fw_ctrl = dt_driver_provider_priv_data(prov);
+		query.ctrl = etzpc_fw_ctrl;


this line is not needed:

query.ctrl = etzpc_fw_ctrl;

as initialized line 428

It is needed. The first time this function is called, etzpc_fw_ctrl is NULL (note that it is static) and is set to a reliable value only at line 447.

patrickdelaunay · 2024-11-21T15:38:25Z

core/arch/arm/plat-stm32mp1/scmi_server.c

+			   STM32MP1_ETZPC_I2C6_ID),
+	RESET_CELL_DECPROT(RST_SCMI_USART1, USART1_R, "usart1",
+			   STM32MP1_ETZPC_USART1_ID),
+	RESET_CELL_DECPROT(RST_SCMI_STGEN, STGEN_R, "stgen",


it is strange to expose STGEN reset by SCMI (RST_SCMI_STGEN)
as STGEN / source of ARM generic timer is always handled in secure world...

That's right. Yet, this P-R makes STGEN reset controller to not be exposed when STGEN is assigned to secure world, thanks to nsec_can_access_resource(). It was also the case before this change, based on shared_resources.c driver, but we intended to remove it since we now have the firewall framework to handle SoC resources access rights management.

patrickdelaunay · 2024-11-21T15:39:19Z

core/arch/arm/plat-stm32mp1/scmi_server.c

+			   STM32MP1_ETZPC_USART1_ID),
+	RESET_CELL_DECPROT(RST_SCMI_STGEN, STGEN_R, "stgen",
+			   STM32MP1_ETZPC_STGENC_ID),
+	RESET_CELL_DECPROT(RST_SCMI_GPIOZ, GPIOZ_R, "gpioz",


GPIOZ reset is not allowed => not exposed

Ditto: htis P-R makes GPIOZ reset controller not exposed to non-secure world when it is assigned to secure world.

ANY reset on GPIO is not allowed => no need to expoze GPIOZ reset (it is never used)

Ok, i'm fine fully disabling GPIO reset controller exposure.

patrickdelaunay · 2024-11-21T16:01:39Z

core/arch/arm/plat-stm32mp1/scmi_server.c

@@ -39,24 +40,28 @@
 /*
 * struct stm32_scmi_clk - Data for the exposed clock
 * @clock_id: Clock identifier in RCC clock driver
+ * @fw_id: Firewall ID of the peripheral related to the clock


fw is confusing for me (firewall / firmware)

"@etzpc_id"

Fix stm32_etzpc firewall check_access handler to report access is permitted only when the request strictly matches the ETZPC configuration. Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>

…ETZPC config Rename fw_id to etzpc_id. Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>

…ETZPC config Ensure STM32MP15 GPIOZ reset controller is never accessible through SCMI. Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>

etienne-lms · 2024-11-22T10:34:13Z

Review comments addressed. I've added a fixup commit for the stm32_etzpc firewall check_access handler that had an issue.

etienne-lms mentioned this pull request Nov 6, 2024

plat-stm32: SCMI reset controllers on remoteproc are disabled on secure fmw loading #7106

Merged

etienne-lms force-pushed the stm32-scmi branch from 14473ec to 48f486d Compare November 14, 2024 07:36

etienne-lms added 4 commits November 14, 2024 17:59

drivers: firewall: stm32_etzpc: add check_access handler

f151394

Implement .check_access handler in stm32_etzpc driver. Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>

plat-stm32mp1: remove unused stm32mp_nsec_can_access_clock()

80adc56

Remove unused platform functions stm32mp_nsec_can_access_clock() and stm32mp_gpio_bank_is_secure(). Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>

plat-stm32mp1: remove unused stm32mp_nsec_can_access_reset()

c0cdadb

Remove unused platform functions stm32mp_nsec_can_access_reset() and stm32mp_gpio_bank_is_non_secure(). Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>

etienne-lms force-pushed the stm32-scmi branch from 48f486d to c0cdadb Compare November 14, 2024 16:59

GseoC reviewed Nov 15, 2024

View reviewed changes

etienne-lms mentioned this pull request Nov 20, 2024

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

Open

patrickdelaunay reviewed Nov 21, 2024

View reviewed changes

etienne-lms added 3 commits November 22, 2024 11:25

[review] drivers: firewall: stm32_etzpc: add check_access handler

65855c4

Fix stm32_etzpc firewall check_access handler to report access is permitted only when the request strictly matches the ETZPC configuration. Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>

[review] plat-stm32mp1: scmi_server: test reset/clock access against …

1f890b1

…ETZPC config Rename fw_id to etzpc_id. Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>

[review] plat-stm32mp1: scmi_server: test reset/clock access against …

36ba6d8

…ETZPC config Ensure STM32MP15 GPIOZ reset controller is never accessible through SCMI. Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plat-stm32mp1: scmi_server: test reset/clock access against ETZPC config #7111

plat-stm32mp1: scmi_server: test reset/clock access against ETZPC config #7111

etienne-lms commented Nov 6, 2024

etienne-lms commented Nov 14, 2024

etienne-lms commented Nov 14, 2024

GseoC Nov 15, 2024

etienne-lms Nov 15, 2024

GseoC Nov 15, 2024

etienne-lms Nov 15, 2024

patrickdelaunay Nov 21, 2024 •

edited

Loading

patrickdelaunay Nov 21, 2024

GseoC Nov 15, 2024

etienne-lms Nov 15, 2024

GseoC Nov 20, 2024

GseoC Nov 15, 2024

etienne-lms Nov 15, 2024

patrickdelaunay Nov 21, 2024 •

edited

Loading

etienne-lms Nov 21, 2024

patrickdelaunay Nov 21, 2024

etienne-lms Nov 21, 2024

patrickdelaunay Nov 21, 2024

etienne-lms Nov 21, 2024

patrickdelaunay Nov 21, 2024

etienne-lms Nov 22, 2024

patrickdelaunay Nov 21, 2024

etienne-lms commented Nov 22, 2024

plat-stm32mp1: scmi_server: test reset/clock access against ETZPC config #7111

Are you sure you want to change the base?

plat-stm32mp1: scmi_server: test reset/clock access against ETZPC config #7111

Conversation

etienne-lms commented Nov 6, 2024

etienne-lms commented Nov 14, 2024

etienne-lms commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickdelaunay Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickdelaunay Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms commented Nov 22, 2024

patrickdelaunay Nov 21, 2024 •

edited

Loading

patrickdelaunay Nov 21, 2024 •

edited

Loading