| 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
| ||
|
Message-ID: <814e7d66-4e97-e394-e183-ac174c7298ad@foss.st.com>
Date: Mon, 14 Nov 2022 15:20:56 +0100
From: Patrick DELAUNAY <patrick.delaunay@...s.st.com>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Alexandre TORGUE <alexandre.torgue@...s.st.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>
CC: Fabrice GASNIER <fabrice.gasnier@...s.st.com>,
Etienne CARRIERE <etienne.carriere@...aro.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>
Subject: Re: [PATCH v2 2/3] nvmem: stm32: add OP-TEE support for STM32MP13x
Hi,
On 11/11/22 18:18, Srinivas Kandagatla wrote:
>
>
> On 10/11/2022 15:45, Patrick Delaunay wrote:
>> For boot with OP-TEE on STM32MP13, the communication with the secure
>> world no more use STMicroelectronics SMC but communication with the
>> BSEC TA, for data access (read/write) or lock operation:
>> - all the request are sent to OP-TEE trusted application,
>> - for upper OTP with ECC protection and with word programming only
>> each OTP are permanently locked when programmed to avoid ECC error
>> on the second write operation
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay@...s.st.com>
>> ---
>>
>> Changes in v2:
>> - rebase series on linux-next/master
>> - minor update after V1 revue
>> - add missing sentinel in stm32_romem_of_match()
>> - reorder function and remove prototypes for stm32_bsec_pta... functions
>> - change stm32_bsec_pta_find to static
>> - add return value in dev_err()
>> - cleanups some comments, which can be on one line
>> - remove test on priv->ctx in stm32_bsec_pta_remove
>> - add missing tee_shm_free(shm) in stm32_bsec_pta_write() when
>> tee_shm_get_va failed
>> - return error in stm32_bsec_pta_find when devm_add_action_or_reset
>> failed
>> - handle driver_register error in stm32_romem_init
>>
>> drivers/nvmem/stm32-romem.c | 445 +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 441 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/nvmem/stm32-romem.c b/drivers/nvmem/stm32-romem.c
>> index d1d03c2ad081..0a0e29d09b67 100644
>> --- a/drivers/nvmem/stm32-romem.c
>> +++ b/drivers/nvmem/stm32-romem.c
>> @@ -11,6 +11,7 @@
>> #include <linux/module.h>
>> #include <linux/nvmem-provider.h>
>> #include <linux/of_device.h>
>> +#include <linux/tee_drv.h>
>> /* BSEC secure service access from non-secure */
>> #define STM32_SMC_BSEC 0x82001003
>> @@ -25,14 +26,401 @@
>> struct stm32_romem_cfg {
>> int size;
>> u8 lower;
>> + bool ta;
>> };
>> struct stm32_romem_priv {
>> void __iomem *base;
>> struct nvmem_config cfg;
>> u8 lower;
>> + struct device *ta;
>> };
>> +#if IS_ENABLED(CONFIG_OPTEE)
>> +/*
>
> ...
>
>> +
>> +static const struct tee_client_device_id stm32_bsec_id_table[] = {
>> + {
>> + UUID_INIT(0x94cf71ad, 0x80e6, 0x40b5,
>> + 0xa7, 0xc6, 0x3d, 0xc5, 0x01, 0xeb, 0x28, 0x03)
>> + },
>> + { }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(tee, stm32_bsec_id_table);
>> +
>> +static struct tee_client_driver stm32_bsec_pta_driver = {
>> + .id_table = stm32_bsec_id_table,
>> + .driver = {
>> + .name = "stm32-bsec-pta",
>> + .bus = &tee_bus_type,
>> + .probe = stm32_bsec_pta_probe,
>> + .remove = stm32_bsec_pta_remove,
>> + },
>> +};
>> +
>> +static void stm32_bsec_put_device(void *data)
>> +{
>> + put_device(data);
>> +}
>> +
>> +static struct device *stm32_bsec_pta_find(struct device *dev)
>> +{
>> + struct device *pta_dev;
>> + int ret;
>> +
>> + pta_dev = driver_find_next_device(&stm32_bsec_pta_driver.driver,
>> NULL);
>
> This is clearly not representing the dependencies in a proper device
> model.
>
>
> If the nvmem provider is a TEE client driver lets model it that way..
> brining in a additional device and somehow trying to link it with TEE
> driver is a hack.
>
TEE is a firmware which allow access to secure ressource... including
BSEC ressources
I think it is also the case on a other driver = mson_sm.c
=> econfig->priv = fw;
fw is a handle to the firmware (secure monitor) which provide
access to secure ressource
BSEC is a hardware device on the bus,
it it describe in the device tree, with a compatible,
the same description should be used for any SW, not only Linux kernel.
and the nvmem cell description are sub-node of BSEC node, used as
phandle by other device.
I need to have a link between the NVMEM driver and the OP-TEE session;
But I use the tee bus discovery here it is a error,
because that create a second uneeded driver "stm32_bsec_pta_driver"...
I will remove this part, and only kept the PTA request with new lib
functions "stm32_bsec_pta_XXX()".
>
>> +
>> + if (pta_dev) {
>> + ret = devm_add_action_or_reset(dev, stm32_bsec_put_device,
>> pta_dev);
>> + if (ret)
>> + dev_err(dev, "devm_add_action_or_reset() failed (%d)\n",
>> ret);
>> +
>> + return ERR_PTR(ret);
>> + }
>> +
>> + return pta_dev;
>> +}
>> +
>> +#else
>> +static int stm32_bsec_pta_read(void *context, unsigned int offset,
>> void *buf,
>> + size_t bytes)
>> +{
>> + pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
>> +
>> + return -ENXIO;
>> +}
>> +
>> +static int stm32_bsec_pta_write(void *context, unsigned int offset,
>> void *buf,
>> + size_t bytes)
>> +{
>> + pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
>> +
>> + return -ENXIO;
>> +}
>> +
>> +static struct device *stm32_bsec_pta_find(struct device *dev)
>> +{
>> + pr_debug("%s: TA BSEC request without OPTEE support\n", __func__);
>> +
>> + return NULL;
>> +}
>> +#endif
>
> ifdefing inside the drvier is really ugly, please move this libary
> functions to a seperate file and add dependecy properly in Kconfig.
Ok
regards
Patrick
Powered by blists - more mailing lists