Skip to content

Commit

Permalink
druvers: tee: optee: Use arch_page_phys_get when MMU enabled
Browse files Browse the repository at this point in the history
According to the description of z_mem_phys_addr it uses
Z_MEM_VM_OFFSET to make a conversion from virtual to physical
address. And these macros are intended for assembly, linker code,
and static initializers.
So when external libraries, such as PICOLIBC or NEWLIB_LIBC
uses internal mappings for dynamic memory allocation z_mem_phys_addr
may not work properly and return wrong physical address.

We have met this problem when testing work with MBEDTLS to generate
certificate using PKCS11 in libckteec.
Mbedtls allocates buffer dynamically and ivokes tee command to request
certificate from PKCS11 TA which should place result to the allocated
buffer.
The result is the MBEDTLS is used with minimal libc - then everything
works fine. But when CONFIG_PICOLIBC or CONFIG_NEWLIB_LIBC is enabled
- it returns empty buffer. This happens because z_mem_phys_addr
returns incorrect physical address.

Signed-off-by: Oleksii Moisieiev <[email protected]>
  • Loading branch information
oleksiimoisieiev committed Feb 12, 2024
1 parent 4c65677 commit 311b9a8
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 11 deletions.
2 changes: 2 additions & 0 deletions drivers/tee/optee/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@
zephyr_library()

zephyr_library_sources(optee.c)

zephyr_library_include_directories(${ZEPHYR_BASE}/arch/${ARCH}/include)
2 changes: 1 addition & 1 deletion drivers/tee/optee/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

config OPTEE
bool "OP-TEE driver"
depends on ARM64 || ZTEST
depends on (ARM64 && MMU) || ZTEST
help
This implements support of the OP-TEE firmware which is loaded
as BL32 image. OP-TEE is a Trust Zone OS which implements mechanisms
Expand Down
46 changes: 36 additions & 10 deletions drivers/tee/optee/optee.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
#include <zephyr/sys/bitarray.h>
#include <zephyr/sys/dlist.h>

#ifdef CONFIG_MMU
#include <kernel_arch_interface.h>
#endif

#include "optee_msg.h"
#include "optee_rpc_cmd.h"
#include "optee_smc.h"
Expand Down Expand Up @@ -88,10 +92,31 @@ struct optee_driver_data {
unsigned long sec_caps;
};

/*
* Use arch_page_phys_get instead of z_mem_phys_addr for address
* conversion.
* According to the description of z_mem_phys_addr it uses
* Z_MEM_VM_OFFSET to make a conversion from virtual to physical
* address. And these macros are intended for assembly, linker code,
* and static initializers.
* So when external libraries, such as PICOLIBC or NEWLIB_LIBC
* uses internal mappings for dynamic memory allocation z_mem_phys_addr
* may not work properly and return wrong physical address.
*/
static uintptr_t optee_mem_phys_addr(void *virt)
{
uintptr_t phys;
uint32_t offt = (uintptr_t)virt % CONFIG_MMU_PAGE_SIZE;

arch_page_phys_get((void *)((uintptr_t)virt - offt), &phys);
return phys + offt;
}

/* Wrapping functions so function pointer can be used */
static void optee_smccc_smc(unsigned long a0, unsigned long a1, unsigned long a2, unsigned long a3,
unsigned long a4, unsigned long a5, unsigned long a6, unsigned long a7,
struct arm_smccc_res *res)
static void
optee_smccc_smc(unsigned long a0, unsigned long a1, unsigned long a2, unsigned long a3,
unsigned long a4, unsigned long a5, unsigned long a6, unsigned long a7,
struct arm_smccc_res *res)
{
arm_smccc_smc(a0, a1, a2, a3, a4, a5, a6, a7, res);
}
Expand Down Expand Up @@ -165,7 +190,7 @@ static void msg_param_to_tmp_mem(struct tee_param *p, uint32_t attr,
return;
}

p->a = mp->u.tmem.buf_ptr - z_mem_phys_addr(shm->addr);
p->a = mp->u.tmem.buf_ptr - optee_mem_phys_addr(shm->addr);
p->c = mp->u.tmem.shm_ref;
}

Expand Down Expand Up @@ -649,7 +674,8 @@ static void handle_rpc_call(const struct device *dev, struct optee_rpc_param *pa
if (!tee_add_shm(dev, NULL, OPTEE_MSG_NONCONTIG_PAGE_SIZE,
param->a1,
TEE_SHM_ALLOC, &shm)) {
u64_to_regs((uint64_t)z_mem_phys_addr(shm->addr), &param->a1, &param->a2);
u64_to_regs((uint64_t)optee_mem_phys_addr(shm->addr), &param->a1,
&param->a2);
u64_to_regs((uint64_t)shm, &param->a4, &param->a5);
} else {
param->a1 = 0;
Expand Down Expand Up @@ -684,7 +710,7 @@ static int optee_call(const struct device *dev, struct optee_msg_arg *arg)
};
void *pages = NULL;

u64_to_regs((uint64_t)z_mem_phys_addr(arg), &param.a1, &param.a2);
u64_to_regs((uint64_t)optee_mem_phys_addr(arg), &param.a1, &param.a2);
while (true) {
struct arm_smccc_res res;

Expand Down Expand Up @@ -941,7 +967,7 @@ static void *optee_construct_page_list(void *buf, uint32_t len, uint64_t *phys_b

for (uint32_t pl_idx = 0; pl_idx < list_size / page_size; pl_idx++) {
for (uint32_t page_idx = 0; num_pages && page_idx < num_pages_in_pl; page_idx++) {
pl[pl_idx].pages[page_idx] = z_mem_phys_addr(buf_page);
pl[pl_idx].pages[page_idx] = optee_mem_phys_addr(buf_page);
buf_page += page_size;
num_pages--;
}
Expand All @@ -950,13 +976,13 @@ static void *optee_construct_page_list(void *buf, uint32_t len, uint64_t *phys_b
break;
}

pl[pl_idx].next_page = z_mem_phys_addr(pl + 1);
pl[pl_idx].next_page = optee_mem_phys_addr(pl + 1);
}

/* 12 least significant bits of optee_msg_param.u.tmem.buf_ptr should hold page offset
* of user buffer
*/
*phys_buf = z_mem_phys_addr(pl) | page_offset;
*phys_buf = optee_mem_phys_addr(pl) | page_offset;

return pl;
}
Expand Down Expand Up @@ -1113,7 +1139,7 @@ static int optee_suppl_send(const struct device *dev, unsigned int ret, unsigned
break;
case TEE_PARAM_ATTR_TYPE_MEMREF_OUTPUT:
case TEE_PARAM_ATTR_TYPE_MEMREF_INOUT:
LOG_WRN("Memref params are not fully tested");
/* LOG_WRN("Memcpyref params are not fully tested"); */
p->a = param[n].a;
p->b = param[n].b;
p->c = param[n].c;
Expand Down

0 comments on commit 311b9a8

Please sign in to comment.