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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f151394
drivers: firewall: stm32_etzpc: add check_access handler
etienne-lms Nov 14, 2024
01e18f2
plat-stm32mp1: scmi_server: test reset/clock access against ETZPC config
etienne-lms Oct 22, 2024
80adc56
plat-stm32mp1: remove unused stm32mp_nsec_can_access_clock()
etienne-lms Oct 23, 2024
c0cdadb
plat-stm32mp1: remove unused stm32mp_nsec_can_access_reset()
etienne-lms Oct 23, 2024
65855c4
[review] drivers: firewall: stm32_etzpc: add check_access handler
etienne-lms Nov 22, 2024
1f890b1
[review] plat-stm32mp1: scmi_server: test reset/clock access against …
etienne-lms Nov 22, 2024
36ba6d8
[review] plat-stm32mp1: scmi_server: test reset/clock access against …
etienne-lms Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 103 additions & 29 deletions core/arch/arm/plat-stm32mp1/scmi_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <confine_array_index.h>
#include <drivers/clk.h>
#include <drivers/clk_dt.h>
#include <drivers/firewall_device.h>
#include <drivers/regulator.h>
#include <drivers/rstctrl.h>
#include <drivers/scmi-msg.h>
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

fw is confusing for me (firewall / firmware)

"@etzpc_id"

* @name: Clock string ID exposed to channel
* @enabled: State of the SCMI clock
*/
struct stm32_scmi_clk {
unsigned long clock_id;
struct clk *clk;
unsigned int fw_id;
const char *name;
bool enabled;
};

/*
* struct stm32_scmi_rd - Data for the exposed reset controller
* @reset_id: Reset identifier in RCC reset driver
* @fw_id: Firewall ID of the peripheral related to the reset controller
* @name: Reset string ID exposed to channel
* @rstctrl: Reset controller device
*/
struct stm32_scmi_rd {
unsigned long reset_id;
unsigned int fw_id;
const char *name;
struct rstctrl *rstctrl;
};
Expand Down Expand Up @@ -99,16 +104,37 @@ register_phys_mem(MEM_AREA_IO_NSEC, CFG_STM32MP1_SCMI_SHM_BASE,
#endif
#endif /*CFG_STM32MP1_SCMI_SHM_BASE*/

/* SCMI clock always accessible */
#define CLOCK_CELL(_scmi_id, _id, _name, _init_enabled) \
[(_scmi_id)] = { \
.clock_id = (_id), \
.fw_id = STM32MP1_ETZPC_MAX_ID, \
.name = (_name), \
.enabled = (_init_enabled), \
}

/* SCMI clock accessible upon DECPROT ID assigned to non-secure */
#define CLOCK_CELL_DECPROT(_scmi_id, _id, _name, _init_enabled, _fw_id) \
[(_scmi_id)] = { \
.clock_id = (_id), \
.fw_id = (_fw_id), \
.name = (_name), \
.enabled = (_init_enabled), \
}

/* SCMI reset domain always accessible */
#define RESET_CELL(_scmi_id, _id, _name) \
[(_scmi_id)] = { \
.reset_id = (_id), \
.fw_id = STM32MP1_ETZPC_MAX_ID, \
.name = (_name), \
}

/* SCMI reset domain accessible upon DECPROT ID assigned to non-secure */
#define RESET_CELL_DECPROT(_scmi_id, _id, _name, _fw_id) \
[(_scmi_id)] = { \
.reset_id = (_id), \
.fw_id = (_fw_id), \
.name = (_name), \
}

Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 think it's still better to warn caller (U-Boot/Linux) in case it manipulates a clock it is not expected to.

STM32MP1_ETZPC_CRYP1_ID),
CLOCK_CELL(CK_SCMI_GPIOZ, GPIOZ, "gpioz", false),
CLOCK_CELL(CK_SCMI_HASH1, HASH1, "hash1", false),
CLOCK_CELL(CK_SCMI_I2C4, I2C4_K, "i2c4_k", false),
CLOCK_CELL(CK_SCMI_I2C6, I2C6_K, "i2c6_k", false),
CLOCK_CELL(CK_SCMI_IWDG1, IWDG1, "iwdg1", false),
CLOCK_CELL(CK_SCMI_RNG1, RNG1_K, "rng1_k", true),
CLOCK_CELL_DECPROT(CK_SCMI_HASH1, HASH1, "hash1", false,
STM32MP1_ETZPC_HASH1_ID),
CLOCK_CELL_DECPROT(CK_SCMI_I2C4, I2C4_K, "i2c4_k", false,
STM32MP1_ETZPC_I2C4_ID),
CLOCK_CELL_DECPROT(CK_SCMI_I2C6, I2C6_K, "i2c6_k", false,
STM32MP1_ETZPC_I2C6_ID),
CLOCK_CELL_DECPROT(CK_SCMI_IWDG1, IWDG1, "iwdg1", false,
STM32MP1_ETZPC_IWDG1_ID),
CLOCK_CELL_DECPROT(CK_SCMI_RNG1, RNG1_K, "rng1_k", true,
STM32MP1_ETZPC_RNG1_ID),
CLOCK_CELL(CK_SCMI_RTC, RTC, "ck_rtc", true),
CLOCK_CELL(CK_SCMI_RTCAPB, RTCAPB, "rtcapb", true),
CLOCK_CELL(CK_SCMI_SPI6, SPI6_K, "spi6_k", false),
CLOCK_CELL(CK_SCMI_USART1, USART1_K, "usart1_k", false),
CLOCK_CELL_DECPROT(CK_SCMI_SPI6, SPI6_K, "spi6_k", false,
STM32MP1_ETZPC_SPI6_ID),
CLOCK_CELL_DECPROT(CK_SCMI_USART1, USART1_K, "usart1_k", false,
STM32MP1_ETZPC_USART1_ID),
};
#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"),
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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 use MDMA in OP-TEE so we let non-secure world fully manage it, including its reset control.

Copy link
Contributor

@patrickdelaunay patrickdelaunay Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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>;
	};

Copy link
Contributor

Choose a reason for hiding this comment

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

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

};
#endif

#ifdef CFG_STM32MP15
static struct stm32_scmi_rd stm32_scmi_reset_domain[] = {
RESET_CELL(RST_SCMI_SPI6, SPI6_R, "spi6"),
RESET_CELL(RST_SCMI_I2C4, I2C4_R, "i2c4"),
RESET_CELL(RST_SCMI_I2C6, I2C6_R, "i2c6"),
RESET_CELL(RST_SCMI_USART1, USART1_R, "usart1"),
RESET_CELL(RST_SCMI_STGEN, STGEN_R, "stgen"),
RESET_CELL(RST_SCMI_GPIOZ, GPIOZ_R, "gpioz"),
RESET_CELL(RST_SCMI_CRYP1, CRYP1_R, "cryp1"),
RESET_CELL(RST_SCMI_HASH1, HASH1_R, "hash1"),
RESET_CELL(RST_SCMI_RNG1, RNG1_R, "rng1"),
RESET_CELL_DECPROT(RST_SCMI_SPI6, SPI6_R, "spi6",
STM32MP1_ETZPC_SPI6_ID),
RESET_CELL_DECPROT(RST_SCMI_I2C4, I2C4_R, "i2c4",
STM32MP1_ETZPC_I2C4_ID),
RESET_CELL_DECPROT(RST_SCMI_I2C6, I2C6_R, "i2c6",
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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

STM32MP1_ETZPC_STGENC_ID),
RESET_CELL_DECPROT(RST_SCMI_GPIOZ, GPIOZ_R, "gpioz",
Copy link
Contributor

Choose a reason for hiding this comment

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

GPIOZ reset is not allowed => not exposed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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'm fine fully disabling GPIO reset controller exposure.

STM32MP1_ETZPC_GPIOZ_ID),
RESET_CELL_DECPROT(RST_SCMI_CRYP1, CRYP1_R, "cryp1",
STM32MP1_ETZPC_CRYP1_ID),
RESET_CELL_DECPROT(RST_SCMI_HASH1, HASH1_R, "hash1",
STM32MP1_ETZPC_HASH1_ID),
RESET_CELL_DECPROT(RST_SCMI_RNG1, RNG1_R, "rng1",
STM32MP1_ETZPC_RNG1_ID),
RESET_CELL(RST_SCMI_MDMA, MDMA_R, "mdma"),
RESET_CELL(RST_SCMI_MCU, MCU_R, "mcu"),
RESET_CELL(RST_SCMI_MCU_HOLD_BOOT, MCU_HOLD_BOOT_R, "mcu_hold_boot"),
Expand Down Expand Up @@ -374,6 +418,39 @@ const uint8_t *plat_scmi_protocol_list(unsigned int channel_id __unused)
return plat_protocol_list;
}

static bool nsec_can_access_resource(unsigned int fw_id)
{
static struct firewall_controller *etzpc_fw_ctrl;
uint32_t query_arg = DECPROT(fw_id, DECPROT_NS_RW, DECPROT_UNLOCK);
struct firewall_query query = {
.arg_count = 1,
.args = &query_arg,
.ctrl = etzpc_fw_ctrl,
};

if (fw_id == STM32MP1_ETZPC_MAX_ID)
return true;

if (!etzpc_fw_ctrl) {
struct dt_driver_provider *prov = NULL;
int node = 0;

node = fdt_node_offset_by_compatible(get_embedded_dt(), -1,
Copy link
Contributor

Choose a reason for hiding this comment

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

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

"st,stm32-etzpc");
if (node < 0)
panic();

prov = dt_driver_get_provider_by_node(node, DT_DRIVER_FIREWALL);
if (!prov)
panic();

etzpc_fw_ctrl = dt_driver_provider_priv_data(prov);
query.ctrl = etzpc_fw_ctrl;
Copy link
Contributor

@patrickdelaunay patrickdelaunay Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this line is not needed:

query.ctrl = etzpc_fw_ctrl;

as initialized line 428

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

return firewall_check_access(&query) == TEE_SUCCESS;
}

/*
* Platform SCMI clocks
*/
Expand Down Expand Up @@ -407,7 +484,7 @@ const char *plat_scmi_clock_get_name(unsigned int channel_id,
{
struct stm32_scmi_clk *clock = find_clock(channel_id, scmi_id);

if (!clock || !stm32mp_nsec_can_access_clock(clock->clock_id))
if (!clock || !nsec_can_access_resource(clock->fw_id))
return NULL;

return clock->name;
Expand All @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, not sure we need to protect clock operations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my response above.

return SCMI_DENIED;

/* Exposed clocks are currently fixed rate clocks */
Expand All @@ -444,7 +521,7 @@ unsigned long plat_scmi_clock_get_rate(unsigned int channel_id,
{
struct stm32_scmi_clk *clock = find_clock(channel_id, scmi_id);

if (!clock || !stm32mp_nsec_can_access_clock(clock->clock_id))
if (!clock || !nsec_can_access_resource(clock->fw_id))
return 0;

return clk_get_rate(clock->clk);
Expand All @@ -454,7 +531,7 @@ int32_t plat_scmi_clock_get_state(unsigned int channel_id, unsigned int scmi_id)
{
struct stm32_scmi_clk *clock = find_clock(channel_id, scmi_id);

if (!clock || !stm32mp_nsec_can_access_clock(clock->clock_id))
if (!clock || !nsec_can_access_resource(clock->fw_id))
return 0;

return (int32_t)clock->enabled;
Expand All @@ -468,7 +545,7 @@ int32_t plat_scmi_clock_set_state(unsigned int channel_id, unsigned int scmi_id,
if (!clock)
return SCMI_NOT_FOUND;

if (!stm32mp_nsec_can_access_clock(clock->clock_id))
if (!nsec_can_access_resource(clock->fw_id))
return SCMI_DENIED;

if (enable_not_disable) {
Expand Down Expand Up @@ -534,7 +611,7 @@ int32_t plat_scmi_rd_autonomous(unsigned int channel_id, unsigned int scmi_id,
if (!rd)
return SCMI_NOT_FOUND;

if (!rd->rstctrl || !stm32mp_nsec_can_access_reset(rd->reset_id))
if (!rd->rstctrl || !nsec_can_access_resource(rd->fw_id))
return SCMI_DENIED;

#ifdef CFG_STM32MP15
Expand Down Expand Up @@ -570,7 +647,7 @@ int32_t plat_scmi_rd_set_state(unsigned int channel_id, unsigned int scmi_id,
if (!rd)
return SCMI_NOT_FOUND;

if (!rd->rstctrl || !stm32mp_nsec_can_access_reset(rd->reset_id))
if (!rd->rstctrl || !nsec_can_access_resource(rd->fw_id))
return SCMI_DENIED;

#ifdef CFG_STM32MP15
Expand Down Expand Up @@ -874,7 +951,7 @@ static TEE_Result stm32mp1_init_scmi_server(void)

/* Sync SCMI clocks with their targeted initial state */
if (clk->enabled &&
stm32mp_nsec_can_access_clock(clk->clock_id))
nsec_can_access_resource(clk->fw_id))
clk_enable(clk->clk);
}

Expand All @@ -886,9 +963,6 @@ static TEE_Result stm32mp1_init_scmi_server(void)
strlen(rd->name) >= SCMI_RD_NAME_SIZE)
panic("SCMI reset domain name invalid");

if (stm32mp_nsec_can_access_clock(rd->reset_id))
continue;

rstctrl = stm32mp_rcc_reset_id_to_rstctrl(rd->reset_id);
assert(rstctrl);
if (rstctrl_get_exclusive(rstctrl))
Expand Down
Loading