Message-ID: <qxzsz2r7ugiyb7njphmdyupihqmalnmfbbsrtpi7meua37qfqb@bobtx24bwl6r>
Date: Tue, 4 Mar 2025 09:58:01 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>
Cc: Mathieu Poirier <mathieu.poirier@...aro.org>, 
	linux-kernel@...r.kernel.org, linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH v15 2/8] remoteproc: Add TEE support

On Wed, Feb 12, 2025 at 02:42:28PM +0100, Arnaud POULIQUEN wrote:
> Hello,
> 
> On 2/12/25 04:18, Bjorn Andersson wrote:
> > On Tue, Dec 10, 2024 at 09:57:40AM +0100, Arnaud POULIQUEN wrote:
> >> Hello Bjorn,
> >>
> >> On 12/6/24 23:07, Bjorn Andersson wrote:
> >>> On Thu, Nov 28, 2024 at 09:42:09AM GMT, Arnaud Pouliquen wrote:
> >>>> Add a remoteproc TEE (Trusted Execution Environment) driver
> >>>> that will be probed by the TEE bus. If the associated Trusted
> >>>> application is supported on secure part this driver offers a client
> >>>> interface to load a firmware by the secure part.
> >>>
> >>> If...else?
> >>>
> >>>> This firmware could be authenticated by the secure trusted application.
> >>>>
> >>>
> >>> I would like for this to fully describe how this fits with the bus and
> >> Are you speaking about the OP-TEE bus?
> >>
> >> I assume that your attempt is that I provide more details on the live cycle
> >> sequence, right?
> >>
> > 
> > Right, there's a tee_client_driver and there's a remoteproc driver.
> > Let's document clearly how these interact.
> > 
> >>> how it is expected to be used by a specific remoteproc driver.
> >>>
> >>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>
> >>>> ---
> >>>> Updates vs version v13:
> >>>> - define REMOTEPROC_TEE as bool instead of tristate,
> >>>> - remove the load of the firmware in rproc_tee_parse_fw as we will ensure
> >>>>   that the firmware is loaded using the load_fw() operation.
> >>>> ---
> >>>>  drivers/remoteproc/Kconfig          |  10 +
> >>>>  drivers/remoteproc/Makefile         |   1 +
> >>>>  drivers/remoteproc/remoteproc_tee.c | 508 ++++++++++++++++++++++++++++
> >>>>  include/linux/remoteproc.h          |   4 +
> >>>>  include/linux/remoteproc_tee.h      | 105 ++++++
> >>>>  5 files changed, 628 insertions(+)
> >>>>  create mode 100644 drivers/remoteproc/remoteproc_tee.c
> >>>>  create mode 100644 include/linux/remoteproc_tee.h
> >>>>
> >>>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> >>>> index 955e4e38477e..f6335321d540 100644
> >>>> --- a/drivers/remoteproc/Kconfig
> >>>> +++ b/drivers/remoteproc/Kconfig
> >>>> @@ -23,6 +23,16 @@ config REMOTEPROC_CDEV
> >>>>  
> >>>>  	  It's safe to say N if you don't want to use this interface.
> >>>>  
> >>>> +config REMOTEPROC_TEE
> >>>> +	bool "Remoteproc support by a TEE application"
> >>>> +	depends on OPTEE
> >>>> +	help
> >>>> +	  Support a remote processor with a TEE application.
> >>>
> >>> Does the remote processor run TEE applications? (Rethorical question...)
> >>>
> >>>> 	  The Trusted
> >>>> +	  Execution Context is responsible for loading the trusted firmware
> >>>> +	  image and managing the remote processor's lifecycle.
> >>>> +
> >>>> +	  It's safe to say N if you don't want to use remoteproc TEE.
> >>>
> >>> It's not really about "wanting to use", it's a question whether your
> >>> device implements/provides the remoteproc TEE.
> >>>
> >>>> +
> >>>>  config IMX_REMOTEPROC
> >>>>  	tristate "i.MX remoteproc support"
> >>>>  	depends on ARCH_MXC
> >>>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> >>>> index 5ff4e2fee4ab..f77e0abe8349 100644
> >>>> --- a/drivers/remoteproc/Makefile
> >>>> +++ b/drivers/remoteproc/Makefile
> >>>> @@ -11,6 +11,7 @@ remoteproc-y				+= remoteproc_sysfs.o
> >>>>  remoteproc-y				+= remoteproc_virtio.o
> >>>>  remoteproc-y				+= remoteproc_elf_loader.o
> >>>>  obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.o
> >>>> +obj-$(CONFIG_REMOTEPROC_TEE)		+= remoteproc_tee.o
> >>>>  obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
> >>>>  obj-$(CONFIG_IMX_DSP_REMOTEPROC)	+= imx_dsp_rproc.o
> >>>>  obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
> >>>> diff --git a/drivers/remoteproc/remoteproc_tee.c b/drivers/remoteproc/remoteproc_tee.c
> >>>> new file mode 100644
> >>>> index 000000000000..3fe3f31068f2
> >>>> --- /dev/null
> >>>> +++ b/drivers/remoteproc/remoteproc_tee.c
> >>>> @@ -0,0 +1,508 @@
> >>>> +// SPDX-License-Identifier: GPL-2.0-or-later
> >>>> +/*
> >>>> + * Copyright (C) STMicroelectronics 2024
> >>>> + * Author: Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>
> >>>> + */
> >>>> +
> >>>> +#include <linux/firmware.h>
> >>>> +#include <linux/io.h>
> >>>> +#include <linux/module.h>
> >>>> +#include <linux/remoteproc.h>
> >>>> +#include <linux/remoteproc_tee.h>
> >>>> +#include <linux/slab.h>
> >>>> +#include <linux/tee_drv.h>
> >>>> +
> >>>> +#define MAX_TEE_PARAM_ARRAY_MEMBER	4
> >>>> +
> >>>> +/*
> >>>> + * Authentication of the firmware and load in the remote processor memory
> >>>
> >>> Exactly what does this imply? Will the content of @memref be copied into
> >>> some other memory?
> >>
> >> The objective is to authenticate and load in one step. So, yes, the image is
> >> loaded into the remoteproc destination memory.
> >>
> > 
> > So, some separate device-memory, or some preallocated carveout which is
> > only accessible from secure world?
> 
> No sure to understand the difference you do between eparate device-memory and
> preallocated carveout.
> 

The main clarification I was looking for was that in your design you
don't use any resources on the Linux side for when your remoteproc
instance is running - i.e. no carveouts etc on the Linux side.

> In OP-TEE, we use the same principle as in Linux. OP-TEE uses memory regions
> declared in its device tree to list memories usable for the coprocessor (with
> associated access rights). On load, it checks that the segments to load  are
> included in these memory regions.
> 
> Linux only declares the shared memory-regions in the device tree, for IPC.
> 
> > 
> > Does the OS need to retain @memref past this point?
> 
> No need,and as the area contains the reult of request_firmware() that can be
> corrupted by Linux, OP-TEE considered this as a temporaray unsafe memory. After
> the load + authentication step this buffer is no more used.
> For detail, OPTEE make a copy of the header and TLV (metadata) in a secure
> memory. and load the firmware images in destination memories All these memories
>  are not accessible from the Linux.
> 

No concerns with this, but these semantics should be clearly documented
here.

> > 
> >> On stm32mp1 we can not store the elf file in a temporary secure memory as
> >> the memory is encrypted by software (this would take to much time).
> >>
> >> For your information, in OP-TEE, the application code is split into a generic
> >> part and a platform adaptation layer. The generic application is mainly
> >> responsible for:
> >>
> >> - Copying the binary header and metadata into secure memory and authenticating them.
> >> - Parsing the ELF images and providing segments to load with associated
> >> authenticated hashes to the platform application.
> >> In the future, someone can add their own format if needed.
> >>
> >> But the generic part could be enhance to authenticate and load a non ELF binary.
> >> So I'm trying to be generic as possible here.
> >>
> > 
> > Generic might be okay, but I'd prefer this to be less vague.
> > Also worth noting is the Qualcomm implementation of TZ-backed
> > remoteproc, which is already in the tree. 
> 
> Could you point me the code in Linux and your TEE, please?
> 

One example is drivers/remoteproc/qcom_q6v5_pas.c where this is captured
in adsp_start().

qcom_mdt_pas_init() parses out the ELF header and signature information
and passes this to the secure world, it then loads the segments of the
ELF into the carvouts (qcom_mdt_load_no_init()) and finally it jumps to
secure world with qcom_scm_pas_auth_and_reset(), which will lock down
Linux's access to the carveouts, then based on previously established
data will authenticate the loaded firmware and finally start execution
on the remote processor.

The difference in this model though is that we don't need the resource
table for rproc_handle_resources() - so this doesn't meet your needs.

> > There the firmware is loaded
> > into carveouts, the certificates and hashes are validated. 
> 
> Seems to me that there is also a partial Authentication done during the load step.
> 

Given that the ELF header and signature information is vetted before the
actually copy the segments into carveouts, it's conceivable that the ELF
header could be sanity checked...

> > Lastly
> > the operation "authenticate and start" is invoked, which does that, and
> > locks the OS out of the given memory region - until "shutdown" is
> > invoked.
> 
> The differnce between the 2 implementations is the authentication method done in
> 2 steps for Qualcomm implementation , in one step for OP-TEE.
> 

Yes, but it needs to be pointed out that this is because you want the
resource table to be authenticated.

> So here if I just remove the term 'authentication' in the command description
> does it ok for you?
> 

No, perhaps I'm misinterpreting you here; but the goal isn't to play
word games until it's good enough - the goal is to have a clean design
that will cover the various cases, and for that we need to establish
what your actual requirements on the host OS side is (typically while
considering the "other side" to be a black box).

> > 
> >>
> >>>
> >>>> + *
> >>>> + * [in]  params[0].value.a:	unique 32bit identifier of the remote processor
> >>>
> >>> Why not just "remote processor identifier"?
> >>>
> >>>> + * [in]	 params[1].memref:	buffer containing the image of the buffer
> >>>> + */
> >>>> +#define TA_RPROC_FW_CMD_LOAD_FW		1
> >>>> +
> >>>> +/*
> >>>> + * Start the remote processor
> >>>> + *
> >>>> + * [in]  params[0].value.a:	unique 32bit identifier of the remote processor
> >>>> + */
> >>>> +#define TA_RPROC_FW_CMD_START_FW	2
> >>>
> >>> Why is there two "FW" in this constant? Why isn't it just
> >>> "TA_RPROC_FW_CMD_START"?
> >>>
> >>> And why is it not TEE_PROC_FW_CMD_START?
> >>>
> >>>> +
> >>>> +/*
> >>>> + * Stop the remote processor
> >>>> + *
> >>>> + * [in]  params[0].value.a:	unique 32bit identifier of the remote processor
> >>>> + */
> >>>> +#define TA_RPROC_FW_CMD_STOP_FW		3
> >>>> +
> >>>> +/*
> >>>> + * Return the address of the resource table, or 0 if not found
> >>>> + * No check is done to verify that the address returned is accessible by
> >>>> + * the non secure context. If the resource table is loaded in a protected
> >>>> + * memory the access by the non secure context will lead to a data abort.
> >>>
> >>> These three lines describe a scenario that doesn't make any sense to me.
> >>> But if that's the case, you should be able to describe that the API
> >>> might give you a inaccessible pointer, by design.
> >>
> >> On STM32MP, we have a kind of firewall in OP-TEE that sets memory access rights
> >> from the device tree. So if the firmware image is not linked according to the
> >> firewall configuration, the pointer may not be accessible.
> >>
> >> I will update the comment as you propose.
> >>
> >>>
> >>>> + *
> >>>> + * [in]  params[0].value.a:	unique 32bit identifier of the remote processor
> >>>> + * [out]  params[1].value.a:	32bit LSB resource table memory address
> >>>> + * [out]  params[1].value.b:	32bit MSB resource table memory address
> >>>> + * [out]  params[2].value.a:	32bit LSB resource table memory size
> >>>> + * [out]  params[2].value.b:	32bit MSB resource table memory size
> >>>> + */
> >>>> +#define TA_RPROC_FW_CMD_GET_RSC_TABLE	4
> >>>> +
> >>>> +/*
> >>>> + * Return the address of the core dump
> >>>
> >>> What does this mean? What will I find at @memref after this call?
> >>
> >> I do not have a simple answer here as it depends on the OP-TEE strategy.
> >> It could be an obscure core dump with possible encryption.
> >>
> >> I will remove this as it is not yet implemented in OP-TEE.
> >>
> > 
> > Okay. But I would prefer that we define the semantics before it's
> > implemented...
> 
> that seems fair, I notice that we will have to address this in a separate thread
> strating with a series in Linux.
> 
> 
> > 
> >> https://elixir.bootlin.com/op-tee/4.4.0/source/ta/remoteproc/src/remoteproc_core.c#L1131
> >>
> >>>
> >>>> + *
> >>>> + * [in]  params[0].value.a:	unique 32bit identifier of the remote processor
> >>>> + * [out] params[1].memref:	address of the core dump image if exist,
> >>>> + *				else return Null
> >>>
> >>> s/else return Null/or NULL/
> >>>
> >>>> + */
> >>>> +#define TA_RPROC_FW_CMD_GET_COREDUMP	5
> >>>> +
> >>>> +/*
> >>>> + * Release remote processor firmware images and associated resources.
> >>>
> >>> Exactly what does this mean for the caller?
> >>
> >> It is platform dependent. It can consist in cleaning the memory, but
> >> can be also something else such as firewall configuration.
> >> On stm323mp we clean all the memories region reserved for the remote processor.
> >>
> > 
> > We can't have an ABI which isn't well defined in intent. Your examples
> > would easily fall in the realm of a well defined interface, but this
> > ties into the question above - what does is actually mean in terms of
> > the memory carveouts and such.
> > 
> 
> Regarding this comment and the one below, does following description would
> respond to your expectations? else do you have a suggestion?
> 
> /*
>  * This command should be used in case an error occurs between the loading of
>  * the firmware images (TA_RPROC_CMD_LOAD_FW) and the starting of the remote
>  * processor (TA_RPROC_CMD_START_FW),

This is valuable information related to TA_RPROC_CMD_LOAD_FW and
TA_RPROC_CMD_START_FW, so let's document this there instead.

>  * or after stopping the remote processor
>  * (TA_RPROC_CMD_STOP_FW).
>  *
>  * This command is used to inform the TEE (Trusted Execution Environment) that
>  * resources associated with the remote processor can be released. After this
>  * command, the firmware is no longer present in the remote processor's memories
>  * and must be reloaded (TA_RPROC_FW_CMD_LOAD_FW) to restart the remote
>  * processor.

I guess it's fine to define it like this on this level, but in the
remoteproc core I'd like us to express the related logic as "release
resources allocated durign rproc_parse_fw(). (And I don't think these
two interpretations are in conflict).

What's unexpected to me then is that you're not actually reloading your
firmware across a recovery/restart?

Regards,
Bjorn

Powered by blists - more mailing lists