drivers: se050: Avoid calling i2c functions #7041

hoihochan · 2024-09-16T05:19:25Z

Create a new option called CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY indicating
there is no native I2C driver inside TEE and all I2C access are done through
the I2C trampoline.

As a result some of the native I2C calls are either stubbed or skipped.

jforissier

Hi @hoihochan,

Please see comments & suggestions below. Thanks.

jforissier · 2024-09-16T06:47:55Z

core/drivers/crypto/se050/adaptors/apis/sss.c

@@ -162,7 +162,7 @@ sss_status_t se050_enable_scp03(sss_se05x_session_t *session)
 		if (status != kStatus_SSS_Success)
 			continue;

-		if (session->subsystem)
+		if (session && session->subsystem)


Unrelated change. Please move to a separate commit.

Without this, there would be a data-abort due to a null pointer:

E/TC:0 0 Core data-abort at address 0x0 (translation fault) E/TC:0 0 esr 0x96000005 ttbr0 0x441fc000 ttbr1 0x00000000 cidr 0x0 E/TC:0 0 cpu #0 cpsr 0x60000104 E/TC:0 0 x0 000000005a5a5a5a x1 00000000441cf000 E/TC:0 0 x2 0000000000000000 x3 0000000000000000 E/TC:0 0 x4 000000000000dc40 x5 fffffffffffffda0 E/TC:0 0 x6 0000000000000000 x7 0000000082590b16 E/TC:0 0 x8 00000000850982f5 x9 00000000d65c420d E/TC:0 0 x10 00000000441b0d78 x11 00000000850982f5 E/TC:0 0 x12 000000005e6c33c6 x13 0000000000000020 E/TC:0 0 x14 0000000000000000 x15 0000000000000000 E/TC:0 0 x16 00000000441887d4 x17 0000000000000000 E/TC:0 0 x18 0000000000000000 x19 000000005a5a5a5a E/TC:0 0 x20 0000000000000000 x21 0000000000000000 E/TC:0 0 x22 00000000441d1b20 x23 0000000000000001 E/TC:0 0 x24 0000000044201648 x25 000000005a5a5a5a E/TC:0 0 x26 00000000441ed8d0 x27 00000000000000fb E/TC:0 0 x28 00000000441ed8b8 x29 00000000442015f0 E/TC:0 0 x30 000000004411b298 elr 000000004411b2a4 E/TC:0 0 sp_el0 00000000442015f0

There exists a path for REE to enable SCP:

PTA_CMD_ENABLE_SCP03 -> crypto_se_enable_scp03 -> se050_enable_scp03

Now, on systems without native I2C driver in OP-TEE that solely relies on I2C trampolines, session is going to be NULL initially until SCP is enabled and working. So attempts to 'close' the previous session will just result in null-pointer exception.

yes, that is correct. I think the commit should be folded since it is not fixing the previous behavior but addressing the current implementation that you are proposing

Unrelated change. Please move to a separate commit.

this is not correct. The commits are related - it is not a bug but part of his initial implementation

Unrelated change. Please move to a separate commit.

this is not correct. The commits are related - it is not a bug but part of his initial implementation

My mistake. Let's have a single commit.

jforissier · 2024-09-16T06:50:06Z

core/drivers/crypto/se050/glue/i2c.c

+#if !CFG_CORE_SE05X_I2C_TRAMPOLINE
 	transfer = native_i2c_transfer;

 	return native_i2c_init();
+#else
+	/* not reached */
+	return 0;
+#endif


Suggested change

#if !CFG_CORE_SE05X_I2C_TRAMPOLINE

transfer = native_i2c_transfer;

return native_i2c_init();

#else

/* not reached */

return 0;

#endif

if (IS_ENABLED(CFG_CORE_SE05X_I2C_TRAMPOLINE)

return 0;

transfer = native_i2c_transfer;

return native_i2c_init();

Hmm. I think this will generate linker errors though, since only imx/stm32 has a native i2c implementation and has native_i2c_init() defined.

Then perhaps native_i2c_transfer() and native_i2c_init() should to be stubbed in core/drivers/crypto/se050/glue/include/i2c_native.h when CFG_CORE_SE05X_I2C_TRAMPOLINE is enabled? @ldts

sorry I am seeing all of the comments now. The issue is that usage of this configuration is being misinterpreted. The configuration does what is supposed to do -and the implementation is correct.

What @hoihochan is after is a new configuration flag to postpone the creation of the session until the REE is ready to handle the I2C transfers. At that point, the session can be created using the PTA to configure the secure channel.

core/drivers/crypto/se050/session.c

hoihochan · 2024-09-16T17:49:52Z

all comments addressed.

core/drivers/crypto/se050/glue/include/i2c_native.h

hoihochan · 2024-09-16T21:11:09Z

all comments addressed and CI builds passed.

etienne-lms

Could you squash the commits the relate to the same goal?

core/drivers/crypto/se050/glue/i2c.c

core/drivers/crypto/se050/glue/include/i2c_native.h

etienne-lms · 2024-09-17T12:24:19Z

core/drivers/crypto/se050/glue/include/i2c_native.h

+	/* Stub only */
+	(void)req;
+	(void)bytes;
+	return TEE_SUCCESS;


prefer:

static inline TEE_Result native_i2c_transfer(struct rpc_i2c_request *req __unused, size_t *bytes __unused) { return TEE_SUCCESS; }

return TEE_ERROR_NBOT_SUPPORTED would be even better, unless some side effect on caller functions falling into the __no_return case.

core/arch/arm/plat-stm32mp1/conf.mk

jforissier

For "drivers: se050: Fix potential null pointer dereference":

Acked-by: Jerome Forissier <[email protected]>

core/arch/arm/plat-stm32mp1/conf.mk

core/drivers/crypto/se050/glue/include/i2c_native.h

ldts

could you explain under which condition this could trigger? c137bf3

hoihochan · 2024-09-19T05:51:34Z

c137bf3

I have a platform that doesn't have native i2c driver inside OP-TEE, but we use the i2c trampoline and enables SCP once Linux fully boots up.

ldts · 2024-09-19T05:58:10Z

c137bf3

I have a platform that doesn't have native i2c driver inside OP-TEE, but we use the i2c trampoline and enables SCP once Linux fully boots up.

Sure but the fix is only required due to the modification of the time at which the session is being open (ie your first commit). So one depends on the other I believe - just thinking out-loud :)

Yes that is a configuration we validated but I never pushed those changes as there are other things to consider since the driver routes TEE requests to I2C and might affect the default configurations (ie, RNG)

In your case, is the REE (u-boot or linux) available before the TEE is loaded (I believe Chromium loads OP-TEE after Linux) or does it come after OP-TEE ?

ldts · 2024-09-19T06:07:42Z

Another thing to consider is that some types of the SE, must have SCP03 enabled on session startup, so this new configuration - if enabled - would be breaking the support for those devices ( ie for instance the SE050F FIPS 140-2 certified device) - check the commit that introduced CFG_CORE_SCP03_ONLY

ldts · 2024-09-19T13:56:44Z

core/drivers/crypto/se050/glue/i2c.c

@@ -47,7 +47,7 @@ int glue_i2c_write(uint8_t *buffer, int len)

 int glue_i2c_init(void)
 {
-	if (transfer == rpc_io_i2c_transfer)
+	if (IS_ENABLED(CFG_CORE_SE05X_I2C_TRAMPOLINE))


removing the previous protection is not a good idea. I dont recall the exact details but it meant that once we have exit the boot_final step, we cant go back to using the native i2c.

It depends on the order in which glue_i2c_init and load_trampoline is called?

If glue_i2c_init is always called after load_trampoline, then it doesn't really matter because transfer is only equal to rpc_io_i2c_transfer if IS_ENABLED(CFG_CORE_SE05X_I2C_TRAMPOLINE) is true.

And I think glue_i2c_init is called inside a pseudo-TA which normally only REE calls and happens way after boot_final.

sorry I dont understand your point. You are breaking the behaviour.
With your change, if glue_i2c_init is called after we have exited the TEE and CFG_CORE_SE05X_I2C_TRAMPOLINE is not enabled then we will likely crash.

please keep the original condition and just OR it with the new one

hoihochan · 2024-09-19T15:31:04Z

c137bf3

I have a platform that doesn't have native i2c driver inside OP-TEE, but we use the i2c trampoline and enables SCP once Linux fully boots up.

Sure but the fix is only required due to the modification of the time at which the session is being open (ie your first commit). So one depends on the other I believe - just thinking out-loud :)

Yes that is a configuration we validated but I never pushed those changes as there are other things to consider since the driver routes TEE requests to I2C and might affect the default configurations (ie, RNG)

In your case, is the REE (u-boot or linux) available before the TEE is loaded (I believe Chromium loads OP-TEE after Linux) or does it come after OP-TEE ?

In my case, TrustedFirmware-A loads the OP-TEE prior to running Linux. So REE runs after OP-TEE.

hoihochan · 2024-09-19T15:32:13Z

Another thing to consider is that some types of the SE, must have SCP03 enabled on session startup, so this new configuration - if enabled - would be breaking the support for those devices ( ie for instance the SE050F FIPS 140-2 certified device) - check the commit that introduced CFG_CORE_SCP03_ONLY

The SE05x part I have specifically require SCP03 enabled for most operations - however we don't have a native i2c driver inside OP-TEE, so we have to defer all i2c operations till Linux/REE runs.

ldts · 2024-09-19T19:35:25Z

Another thing to consider is that some types of the SE, must have SCP03 enabled on session startup, so this new configuration - if enabled - would be breaking the support for those devices ( ie for instance the SE050F FIPS 140-2 certified device) - check the commit that introduced CFG_CORE_SCP03_ONLY

The SE05x part I have specifically require SCP03 enabled for most operations - however we don't have a native i2c driver inside OP-TEE, so we have to defer all i2c operations till Linux/REE runs.

yes, that is exactly how I brought up initially STM32. I think your PR is mostly right but I think we should have some more documentation and a slight change.

ldts · 2024-09-19T19:44:16Z

core/drivers/crypto/se050/session.c

@@ -123,6 +123,11 @@ static TEE_Result se050_session_init(void)
 	if (IS_ENABLED(CFG_CORE_SCP03_ONLY))
 		return se050_early_init_scp03();

+	if (IS_ENABLED(CFG_CORE_SE05X_I2C_TRAMPOLINE)) {


move your new configuration CFG_CORE_SE05X_I2C_TRAMPOLINE_EARLY above the previous condition - and do not modify this one please

ldts · 2024-09-19T21:07:45Z

CFG_CORE_SE05X_I2C_TRAMPOLINE does not mean what you say in your commit. So please do not reuse it - for reference, CFG_CORE_SE05X_I2C_TRAMPOLINE means that the user is allowed to use the trampoline after TEE boot. If not set, it will continue to use the native driver.

# I2C access via REE after TEE boot
CFG_CORE_SE05X_I2C_TRAMPOLINE ?= y

You need a new configuration for the feature you are implementing.
maybe CFG_CORE_SE05X_I2C_TRAMPOLINE_EARLY ?

hoihochan · 2024-09-20T04:06:58Z

Have refactored the code to use a new config option CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY to indicate the platform does not have any native I2C drivers and rely solely on REE I2C trampolines for accessing the SE05X.

That being said, se050_session will continue to be NULL (i.e. no valid SE05X session) prior to invoking PTA_CMD_ENABLE_SCP03 on the SCP PTA from REE, so the second commit 4fca423 will still be necessary. Re-pasting the call flow below:

PTA_CMD_ENABLE_SCP03
   -> crypto_se_enable_scp03(bool rotate_keys)
     -> se050_enable_scp03(sss_se05x_session_t *session) <-- will crash if session is NULL

Feedbacks welcome.

ldts · 2024-09-20T06:32:41Z

Have refactored the code to use a new config option CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY to indicate the platform does not have any native I2C drivers and rely solely on REE I2C trampolines for accessing the SE05X.

That being said, se050_session will continue to be NULL (i.e. no valid SE05X session) prior to invoking PTA_CMD_ENABLE_SCP03 on the SCP PTA from REE, so the second commit 4fca423 will still be necessary. Re-pasting the call flow below:
PTA_CMD_ENABLE_SCP03
   -> crypto_se_enable_scp03(bool rotate_keys)
     -> se050_enable_scp03(sss_se05x_session_t *session) <-- will crash if session is NULL
Feedbacks welcome.

thanks for doing this @hoihochan , I'll have a look.

yes the second commit will be necessary but just as part of the first one (they are not independent, one causes the need for the other so the second one would be a fix for the first one... and since introducing a commit causing a segmentation fault doesn't make sense, it should be folded IMO)

ldts · 2024-09-20T07:37:32Z

@jforissier both commits should be folded please

ldts · 2024-09-20T08:04:51Z

@hoihochan could you reword the commit message please?

drivers: se050: new CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY

Also maybe add something explicitly warning in the commit that that when enabling this option OP-TEE must not use any of the resources provided by the secure element until the REE is up. This is because the secure element driver can present services that OP-TEE could try to use early.

The option CFG_CORE_SCP03_ONLY must still be supported; so it requires to be added in the places where the session could be open now due to CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY being enabled (TRAMPOLINE_ONLY just delays opening the session till the REE is ready)

in crypto_se_enable_scp03 ()
in crypto_se_do_apdu()

You also need to support the new CFG_ in crypto_se_do_apdu

Notice that the way to enable SCP03 is slightly different when the session didnt exist: you need to do as it is done in se050_early_init_scp03() so we can store the OEFID.

hoihochan · 2024-09-20T16:35:14Z

The option CFG_CORE_SCP03_ONLY must still be supported

By the way, how are we going to support CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY and CFG_CORE_SCP03_ONLY at the same time?

OP-TEE wouldn't know REE is ready unless someone tells it (e.g. via an invoke to the SCP03 PTA to enable SCP).

Another thing I'd like to ask is why we fill the msg with 0x55s in se050_scp03_subkey_derive when CFG_CORE_SCP03_ONLY is enabled?

        if (IS_ENABLED(CFG_CORE_SCP03_ONLY)) {
                memset(msg, 0x55, sizeof(msg));
        } else {
                /* add some randomness */
                if (tee_otp_get_die_id(msg + 3, SE050_SCP03_KEY_SZ))
                        return kStatus_SSS_Fail;
        }

Don't we always want device-unique entropy?

Create a new option called CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY indicating there is no native I2C driver inside TEE and all I2C access are done through the I2C trampoline. As a result some of the native I2C calls are either stubbed or skipped. NOTE: when this option is enabled, do not invoke any of the APIs involving secure element access (e.g. from within OP-TEE OS or a PTA) as they will not work until REE is up and running. Signed-off-by: Donald Chan <[email protected]>

ldts · 2024-09-20T22:08:19Z

Another thing I'd like to ask is why we fill the msg with 0x55s in se050_scp03_subkey_derive when CFG_CORE_SCP03_ONLY is enabled?
        if (IS_ENABLED(CFG_CORE_SCP03_ONLY)) {
                memset(msg, 0x55, sizeof(msg));
        } else {
                /* add some randomness */
                if (tee_otp_get_die_id(msg + 3, SE050_SCP03_KEY_SZ))
                        return kStatus_SSS_Fail;
        }
Don't we always want device-unique entropy?

the reason for that bit of code is because tee_otp_get_die_id could be calling the SE - and with SCP03_ONLY that would be a catch22 since the session has not been established at that point.

ldts · 2024-09-20T22:11:59Z

The option CFG_CORE_SCP03_ONLY must still be supported

By the way, how are we going to support CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY and CFG_CORE_SCP03_ONLY at the same time?

OP-TEE wouldn't know REE is ready unless someone tells it (e.g. via an invoke to the SCP03 PTA to enable SCP).

CFG_CORE_SCP03_ONLY means that the SE can only be accessed from a secure session...so we can't establish the session and then enable SCP03 as we use in the default case.; for CFG_CORE_SCP03_ONLY it doesn't matter whether we use the trampoline or the native i2c interface really

ldts · 2024-09-22T08:00:12Z

core/drivers/crypto/se050/core/apdu.c

@@ -16,6 +16,11 @@ TEE_Result crypto_se_do_apdu(enum crypto_apdu_type type,
 {
 	sss_status_t status = kStatus_SSS_Fail;

+	if (IS_ENABLED(CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY) && !se050_session) {
+		/* Defer until REE is ready */


we should open the session here instead of deferring

we should also open in crypto_se_enable_scp03 (handle the open in that function as well); otherwise se050_enable_scp03 will break looking for the OEFID unless CFG_CORE_SE05X_OEFID is set which is not necessarily true.

an to avoid racing while doing it - since we might be coming from a multi-threaded environment - please use some form of exclusion mechanism

try calling se050_session_init() from both places - so no longer a static method - I think that should cover all cases. then when that CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY is enabled, dont let driver_init call the function

I think we should perhaps have a tee_session.c and a ree_session.c for clarity - I dont think we should panic() when the secure element fails to open when loaded from the REE.

ldts · 2024-10-21T10:29:22Z

@hoihochan just checking, do you plan to continue with this feature? it is not a lot of extra work on top of what you have done already

hoihochan · 2024-11-02T03:36:43Z

@hoihochan just checking, do you plan to continue with this feature? it is not a lot of extra work on top of what you have done already

Yes, sorry I was out on an extended vacation. Will get back to it next week.

ldts · 2024-11-04T10:45:10Z

Yes, sorry I was out on an extended vacation. Will get back to it next week.

awesome. I think there is value here

jforissier reviewed Sep 16, 2024

View reviewed changes

hoihochan force-pushed the fix-se050-i2c-trampoline branch from a5d8baf to 02b72df Compare September 16, 2024 07:04

jforissier reviewed Sep 16, 2024

View reviewed changes

core/drivers/crypto/se050/glue/include/i2c_native.h Show resolved Hide resolved

hoihochan force-pushed the fix-se050-i2c-trampoline branch 2 times, most recently from d8b3576 to f9d2d22 Compare September 16, 2024 20:28

etienne-lms reviewed Sep 17, 2024

View reviewed changes

hoihochan force-pushed the fix-se050-i2c-trampoline branch 2 times, most recently from aa2d2b8 to 67e4a93 Compare September 17, 2024 23:13

jforissier reviewed Sep 18, 2024

View reviewed changes

etienne-lms reviewed Sep 18, 2024

View reviewed changes

core/arch/arm/plat-stm32mp1/conf.mk Outdated Show resolved Hide resolved

core/drivers/crypto/se050/glue/include/i2c_native.h Outdated Show resolved Hide resolved

core/drivers/crypto/se050/glue/include/i2c_native.h Outdated Show resolved Hide resolved

hoihochan force-pushed the fix-se050-i2c-trampoline branch from 67e4a93 to c137bf3 Compare September 18, 2024 16:03

ldts reviewed Sep 19, 2024

View reviewed changes

hoihochan force-pushed the fix-se050-i2c-trampoline branch from c137bf3 to 4fca423 Compare September 19, 2024 21:21

hoihochan force-pushed the fix-se050-i2c-trampoline branch from 4fca423 to f1cf9d6 Compare September 20, 2024 16:36

hoihochan force-pushed the fix-se050-i2c-trampoline branch from f1cf9d6 to 364a794 Compare September 20, 2024 17:44

hoihochan force-pushed the fix-se050-i2c-trampoline branch from 364a794 to 1b57da0 Compare September 20, 2024 18:16

ldts suggested changes Sep 22, 2024

View reviewed changes

drivers: se050: Avoid calling i2c functions #7041

Are you sure you want to change the base?

drivers: se050: Avoid calling i2c functions #7041

Conversation

hoihochan commented Sep 16, 2024 • edited Loading

jforissier left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoihochan commented Sep 16, 2024

hoihochan commented Sep 16, 2024 • edited Loading

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

ldts left a comment • edited Loading

Choose a reason for hiding this comment

hoihochan commented Sep 19, 2024

ldts commented Sep 19, 2024 • edited Loading

ldts commented Sep 19, 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

hoihochan commented Sep 19, 2024

hoihochan commented Sep 19, 2024

ldts commented Sep 19, 2024

ldts Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

ldts commented Sep 19, 2024 • edited Loading

hoihochan commented Sep 20, 2024

ldts commented Sep 20, 2024

ldts commented Sep 20, 2024

ldts commented Sep 20, 2024 • edited Loading

hoihochan commented Sep 20, 2024 • edited Loading

ldts commented Sep 20, 2024

ldts commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldts Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

ldts commented Oct 21, 2024

hoihochan commented Nov 2, 2024

ldts commented Nov 4, 2024

hoihochan commented Sep 16, 2024 •

edited

Loading

hoihochan commented Sep 16, 2024 •

edited

Loading

ldts left a comment •

edited

Loading

ldts commented Sep 19, 2024 •

edited

Loading

ldts commented Sep 19, 2024 •

edited

Loading

ldts Sep 19, 2024 •

edited

Loading

ldts commented Sep 19, 2024 •

edited

Loading

ldts commented Sep 20, 2024 •

edited

Loading

hoihochan commented Sep 20, 2024 •

edited

Loading

ldts Sep 22, 2024 •

edited

Loading