Allow NS secure call at pre-rtos stage
Open, Needs TriagePublic

Description

I am working on mbed-os/tf-m port on M2351. From M2351 NS, it needs to call platform-specific secure function at pre-rtos stage. Currently, tfm_ns_lock_dispatch doesn't dispatch until NS lock is ready. I would like to support it by exporting tfm_ns_lock_get_init_state. NS world can call secure function straight when it is now at pre-rtos stage (tfm_ns_lock_get_init_state returns false).

A related change is submitted in 1123.

ccli8 created this task.Wed, May 29, 9:34 AM

The NS lock is initialized at a point in time when the scheduler is not yet started, therefore there is a single thread of execution on the NS side.
I agree it is safe to assume that in such a scenario, the only actor on the NS side is privileged and therefore is assumed to be in full control of execution, there are no separate protection domains within NSPE.
Secure lock is already set up so there's no risk of introducing new exploits with this change.

I suggest to provide the implementation of the function in the respective change, I'm inviting subscribers to comment here and review.

I agree in principle with the idea, but I have a comment regarding the implementation.

Our current implementation of the OS abstraction layer on the NS side is not meant for RTX specifically, but is aimed at CMSIS-RTOS2 abstraction compatibility. The proposed implementation is RTX specific, and it requires accessing RTX-specific primitives related to the lock to infer the current state of the kernel, in an indirect way.

I think CMSIS-RTOS2 has made available an API which allows the application to probe the state of the kernel to understand if the kernel has started or we are still in pre-rtos stage:

https://www.keil.com/pack/doc/CMSIS/RTOS2/html/group__CMSIS__RTOS__KernelCtrl.html#ga48b69b81012fce051f639be288b243ba

I would prefer to see if there is a way to use this API to obtain the same result as the current proposed implementation, in a way that the CMSIS-RTOS2 abstraction is not broken to access the underlying objects of the kernel.

On a side note, changes 1123 and 1124 seem to me both related to this thread, but 1123 has the header only, while 1124 both header and implementation. please consolidate them both in a single change for easier review.

Just to be clear, as there has been some confusion between get_init_state() and get_lock_state (particularly on my side :) ), I think that the get_init_state(...) doesn't need to be exported as probably the same result can be obtained by proper usage of CMSIS-RTOS2 API's (or equivalent API's, based on the NS side scenario). Regarding the get_lock_state(...), I will comment on the other thread. T378

ccli8 added a comment.Fri, Jun 7, 2:19 AM

1123 is for NS secure call at pre-rtos stage and 1124 for in interrupt-disabled condition. They are different and so separate changes. For 1123, since osKernelGetState can substitute for get_init_state. I have three choices:

  1. Abandon 1123 (and also get_init_state)
  2. Re-implement get_init_state with osKernelGetState
  3. Abandon 1123 (and also get_init_state) and integrate pre-rtos NS secure call into tfm_ns_lock_dispatch

Thanks for summarising the three options.

Currently, get_init_state is not used by any module. It's probably a mistake that is still available, so to proceed further, I would probably drop it completely and let your integration to use osKernelGetState directly to check for pre-rtos stage. Thus dropping 1123.

In 1124, for interrupt disabled dispatching scenarios, I am still analysing the scenario. Your solution is feasible but not exhaustive, plus it needs to access RTX internals (to extract the lock value of the mutex) which personally I would avoid as it breaks the CMSIS-RTOS2 abstraction. I will try to come up with a plausible alternative to be discussed there, hopefully.

ccli8 added a comment.Mon, Jun 10, 2:44 AM

After dropping 1123, create another change which adds support for pre-retos dispatch in tfm_ns_lock_dispatch by checking kernel state with osKernelGetState, right?

adeaarm added a comment.EditedMon, Jun 10, 8:46 AM

Strictly speaking, the files in interface/src are a possible implementation of the interface described in interface/include. Your integration can provide a different implementation of tfm_ns_lock_dispatch(...) based on your requirements, without the need to upstream your change. But if you think that your change can be useful for a wider audience, yes, please create a change where you modify tfm_ns_lock_dispatch(...) using CMSIS-RTOS2 APIs to check for pre-rtos stage and we'll get that reviewed.

ccli8 added a comment.Mon, Jun 10, 9:08 AM

Upstream change 1231 to support secure call in pre-rtos stage in tfm_ns_lock_dispatch(...). I think some audience would benefit from it. Without it, I need to make an extra check for pre-rtos scenario before making a secure call.

This call involves a Thread -> Handler mode request on every service call to check if we are in pre-RTOS stage. I think this will introduce a non-negligible penalty; in most of the cases, we expect this call to happen when the RTOS has been loaded.

For the osKernelGetState() overhead in tfm_ns_lock_dispatch(...), I think it can be replaced by just checking ns_lock.init.

adeaarm added a comment.EditedMon, Jun 10, 1:17 PM
In T376#4490, @ccli8 wrote:

This call involves a Thread -> Handler mode request on every service call to check if we are in pre-RTOS stage. I think this will introduce a non-negligible penalty; in most of the cases, we expect this call to happen when the RTOS has been loaded.

For the osKernelGetState() overhead in tfm_ns_lock_dispatch(...), I think it can be replaced by just checking ns_lock.init.

There can be cases where the lock has not been initialised yet but the RTOS has started, depending on use cases. That's why I would like to have a direct check of the RTOS state instead of the NS lock init. (on a side note, as this is a review comment, please keep the discussion in the Gerrit review for better tracking and organisation).

Moreover, putting the check on the ns.init variable, it implies that the model of computation on the NS side makes the entire NS lock optional, i.e. if there is a lock available, lock it, but if there isn't any lock, just go ahead and proceed with the call without any lock. This wound't be restricted to a pre-RTOS stage.

jf549 added a subscriber: jf549.Mon, Jun 10, 5:05 PM

I don't really know the full context of this, so maybe I am way off here, but if there is some secure code that needs to be executed before the NS RTOS is started, is it not best executed as part of secure init? The secure partition containing the secure function (the one that must be called before the RTOS is started) will have an init function, so could that be used to execute the required code?

ccli8 added a comment.Tue, Jun 11, 1:57 AM

The secure partition init function cannot cover all use cases. The requirement of pre-rtos secure call actually comes from my mbed-os/tf-m port on Nuvoton's M2351 chip. For example, on mbed-os, the CMSIS API SystemCoreClockUpdate(...) is called to update SystemCoreClock in pre-rtos stage on NS side. On Nuvoton's M2351, SystemCoreClockUpdate(...)'s implementation needs to access CLK space registers which are hardwired to secure. That's where secure call in pre-rtos stage is necessary. I've also checked SystemCoreClockUpdate(...)'s implementation on Arm's Musca A1. It has SystemCoreClock fixed in macro, and so it needn't.