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.

Sign up for GitHub

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

Jump to bottom

drivers: se050: Avoid calling i2c functions #7041

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

drivers: se050: Avoid calling i2c functions #7041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/drivers/crypto/se050/adaptors/apis/sss.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

sss_se05x_session_close(session);

if (!se050_core_early_init(&keys)) {
Expand Down
5 changes: 5 additions & 0 deletions core/drivers/crypto/se050/core/apdu.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

return TEE_ERROR_COMMUNICATION;
}

status = sss_se05x_do_apdu(&se050_session->s_ctx, type,
hdr, hdr_len, src_data, src_len,
dst_data, dst_len);
Expand Down
2 changes: 2 additions & 0 deletions core/drivers/crypto/se050/crypto.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ CFG_CORE_SE05X_BAUDRATE ?= 3400000
CFG_CORE_SE05X_I2C_BUS ?= 2
# I2C access via REE after TEE boot
CFG_CORE_SE05X_I2C_TRAMPOLINE ?= y
# TEE has no native I2C driver - all I2C access done via REE
CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY ?= n

# Extra stacks required to support the Plug and Trust external library
ifeq ($(shell test $(CFG_STACK_THREAD_EXTRA) -lt 8192; echo $$?), 0)
Expand Down
3 changes: 2 additions & 1 deletion core/drivers/crypto/se050/glue/i2c.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ int glue_i2c_write(uint8_t *buffer, int len)

int glue_i2c_init(void)
{
if (transfer == rpc_io_i2c_transfer)
if (transfer == rpc_io_i2c_transfer ||
IS_ENABLED(CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY))
return 0;

transfer = native_i2c_transfer;
Expand Down
16 changes: 16 additions & 0 deletions core/drivers/crypto/se050/glue/include/i2c_native.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,24 @@

#include <kernel/rpc_io_i2c.h>

#ifdef CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY
static inline TEE_Result
native_i2c_transfer(struct rpc_i2c_request *req __unused,
size_t *bytes __unused)
{
/* Stub only */
return TEE_ERROR_NOT_SUPPORTED;
}

static inline int native_i2c_init(void)
{
/* Stub only */
return 0;
}
hoihochan marked this conversation as resolved.
Show resolved Hide resolved
#else
TEE_Result native_i2c_transfer(struct rpc_i2c_request *req,
size_t *bytes);
int native_i2c_init(void);
#endif

#endif
5 changes: 5 additions & 0 deletions core/drivers/crypto/se050/session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ static TEE_Result se050_early_init_scp03(void)

static TEE_Result se050_session_init(void)
{
if (IS_ENABLED(CFG_CORE_SE05X_I2C_TRAMPOLINE_ONLY)) {
/* Nothing to do */
return TEE_SUCCESS;
}

if (IS_ENABLED(CFG_CORE_SCP03_ONLY))
return se050_early_init_scp03();

Expand Down