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.

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

drivers: se050: Avoid calling i2c functions #7041

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hoihochan
Copy link

@hoihochan hoihochan commented Sep 16, 2024

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.

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

Hi @hoihochan,

Please see comments & suggestions below. Thanks.

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated change. Please move to a separate commit.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines 53 to 60
#if !CFG_CORE_SE05X_I2C_TRAMPOLINE
transfer = native_i2c_transfer;

return native_i2c_init();
#else
/* not reached */
return 0;
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@hoihochan
Copy link
Author

all comments addressed.

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

hoihochan commented Sep 16, 2024

all comments addressed and CI builds passed.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

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

core/drivers/crypto/se050/glue/i2c.c Outdated Show resolved Hide resolved
core/drivers/crypto/se050/glue/include/i2c_native.h Outdated Show resolved Hide resolved
/* Stub only */
(void)req;
(void)bytes;
return TEE_SUCCESS;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

done

core/arch/arm/plat-stm32mp1/conf.mk Outdated Show resolved Hide resolved
@hoihochan hoihochan force-pushed the fix-se050-i2c-trampoline branch 2 times, most recently from aa2d2b8 to 67e4a93 Compare September 17, 2024 23:13
Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

@ldts ldts left a comment

Choose a reason for hiding this comment

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

could you explain under which condition this could trigger? c137bf3

@hoihochan
Copy link
Author

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
Copy link
Contributor

ldts commented Sep 19, 2024

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
Copy link
Contributor

ldts commented Sep 19, 2024

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

@@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@hoihochan
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

ldts commented Sep 19, 2024

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.

@@ -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)) {
Copy link
Contributor

@ldts ldts Sep 19, 2024

Choose a reason for hiding this comment

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

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

@ldts
Copy link
Contributor

ldts commented Sep 19, 2024

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
Copy link
Author

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
Copy link
Contributor

ldts commented Sep 20, 2024

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
Copy link
Contributor

ldts commented Sep 20, 2024

@jforissier both commits should be folded please

@ldts
Copy link
Contributor

ldts commented Sep 20, 2024

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

  1. in crypto_se_enable_scp03 ()
  2. 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
Copy link
Author

hoihochan commented Sep 20, 2024

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
Copy link
Contributor

ldts commented Sep 20, 2024

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
Copy link
Contributor

ldts commented Sep 20, 2024

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

@@ -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 */
Copy link
Contributor

Choose a reason for hiding this comment

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

we should open the session here instead of deferring

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ldts ldts Sep 22, 2024

Choose a reason for hiding this comment

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

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
Copy link
Contributor

ldts commented Oct 21, 2024

@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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants