Hi Robert,
Some answers below.
On Wed, Dec 2, 2020 at 1:00 PM Robert Deliën via OP-TEE op-tee@lists.trustedfirmware.org wrote:
Hi,
We are using op-tee 3.10 in our i.MX6UL-based product and we are running into a small problem we hope you can help us out with. Apologies for the large mail; It's mostly pictures.
The nature of our product requires us to protect access to the display contents. To achieve this, we have added a display driver to op-tee and have allocated the frame buffers in op-tee too.
We are conforming to the memory-map as layed out in .../core/arch/arm/include/mm/generic_ram_layout.h:
TEE RAM layout without CFG_WITH_PAGER
+----------------------------------+ <-- CFG_TZDRAM_START | TEE core secure RAM (TEE_RAM) | +----------------------------------+ | Trusted Application RAM (TA_RAM) | +----------------------------------+ | SDP test memory (optional) | +----------------------------------+ <-- CFG_TZDRAM_START + CFG_TZDRAM_SIZE
+----------------------------------+ <-- CFG_SHMEM_START | Non-secure static SHM | +----------------------------------+ <-- CFG_SHMEM_START + CFG_SHMEM_SIZE
With this memory map in mind, we came up with the following solution:
+----------------------------------+ <-- CFG_DRAM_BASE | | | Non-secure memory | | | +----------------------------------+ <-- CFG_TZDRAM_START | TEE core secure RAM (TEE_RAM) | +----------------------------------+ | Trusted Application RAM (TA_RAM) | +----------------------------------+ <-- CFG_POS_SEC_FB0_START | Secure frame buffer 0 | +----------------------------------+ <-- CFG_POS_SEC_FB1_START | Secure frame buffer 1 | +----------------------------------+ <-- CFG_SHMEM_START (== CFG_TZDRAM_START + CFG_TZDRAM_SIZE) | Non-secure static SHM | +----------------------------------+ <-- Top of DDR (CFG_SHMEM_START + CFG_SHMEM_SIZE) CFG_DDR_SIZE = 0x10000000 CFG_SHMEM_SIZE = 0x00200000 CFG_SHMEM_START = ($(CFG_DRAM_BASE) + $(CFG_DDR_SIZE) - $(CFG_SHMEM_SIZE)) CFG_SEC_FB_SIZE = 0x00096000 CFG_POS_SEC_FB1_START = ($(CFG_SHMEM_START) - $(CFG_SEC_FB_SIZE)) CFG_POS_SEC_FB0_START = ($(CFG_POS_SEC_FB1_START) - $(CFG_SEC_FB_SIZE)) CFG_TZDRAM_START = ($(CFG_DRAM_BASE) + 0x0e000000) CFG_TZDRAM_SIZE = ($(CFG_SHMEM_START) - $(CFG_TZDRAM_START))
This solution keeps the frame buffers within the region CFG_TZDRAM_START:CFG_TZDRAM_SIZE, but when booting the system, we notice an overlap between TA_RAM and both frame buffer areas:
D/TC:0 dump_mmap_table:732 type TA_RAM va 0x88c00000..0x8a8fffff pa 0x8e100000..0x8fdfffff size 0x01d00000 (pgdir) ... D/TC:0 dump_mmap_table:732 type RAM_SEC va 0x8ded4000..0x8df69fff pa 0x8fd6a000..0x8fdfffff size 0x00096000 (smallpg) D/TC:0 dump_mmap_table:732 type RAM_SEC va 0x8df6a000..0x8dffffff pa 0x8fcd4000..0x8fd69fff size 0x00096000 (smallpg)
Without fully comprehending how memory is allocated, we fear that this situation will allow TA's to be loaded into these frame buffer areas. To mitigate this, we came up with this alternative:
+----------------------------------+ <-- CFG_DRAM_BASE | | | Non-secure memory | | | +----------------------------------+ <-- CFG_TZDRAM_START | TEE core secure RAM (TEE_RAM) | +----------------------------------+ | Trusted Application RAM (TA_RAM) | +----------------------------------+ <-- CFG_POS_SEC_FB0_START (== CFG_TZDRAM_START + CFG_TZDRAM_SIZE) | Secure frame buffer 0 | +----------------------------------+ <-- CFG_POS_SEC_FB1_START | Secure frame buffer 1 | +----------------------------------+ <-- CFG_SHMEM_START | Non-secure static SHM | +----------------------------------+ <-- Top of DDR (CFG_SHMEM_START + CFG_SHMEM_SIZE) CFG_DDR_SIZE = 0x10000000 CFG_SHMEM_SIZE = 0x00200000 CFG_SHMEM_START = ($(CFG_DRAM_BASE) + $(CFG_DDR_SIZE) - $(CFG_SHMEM_SIZE)) CFG_SEC_FB_SIZE = 0x00096000 CFG_POS_SEC_FB1_START = ($(CFG_SHMEM_START) - $(CFG_SEC_FB_SIZE)) CFG_POS_SEC_FB0_START = ($(CFG_POS_SEC_FB1_START) - $(CFG_SEC_FB_SIZE)) CFG_TZDRAM_START = ($(CFG_DRAM_BASE) + 0x0e000000) CFG_TZDRAM_SIZE = ($(CFG_POS_SEC_FB0_START) - $(CFG_TZDRAM_START))
This solution puts the frame buffers outside the region CFG_TZDRAM_START:CFG_TZDRAM_SIZE so they no longer overlap with TA_RAM. However, when booting the system, op-tee panics:
D/TC:0 dump_mmap_table:732 type RAM_SEC va 0x8c2d4000..0x8c369fff pa 0x8fd6a000..0x8fdfffff size 0x00096000 (smallpg) D/TC:0 dump_mmap_table:732 type RAM_SEC va 0x8c36a000..0x8c3fffff pa 0x8fcd4000..0x8fd69fff size 0x00096000 (smallpg) ... D/TC:0 dump_mmap_table:732 type TA_RAM va 0x8c400000..0x8dfd3fff pa 0x8e100000..0x8fcd3fff size 0x01bd4000 (smallpg) ... E/TC:0 0 check_phys_mem_is_outside:332 Non-sec mem (0x8fcd4000:0x12c000) overlaps map (type 12 0x8fd6a000:0x96000) E/TC:0 0 Panic at core/arch/arm/mm/core_mmu.c:336 <check_phys_mem_is_outside> E/TC:0 0 TEE load address @ 0x8e000000 E/TC:0 0 Call stack: E/TC:0 0 0x8e005621 E/TC:0 0 0x8e01d397 E/TC:0 0 0x8e005aaf E/TC:0 0 0x8e004da7 E/TC:0 0 0x8e000174
In this situation, the memory area intended for the frame buffers (0x8fd6a000:0x96000) is now implicitly regarded as non-secure RAM, regardless of us registering it as MEM_AREA_RAM_SEC in main.c:
register_phys_mem(MEM_AREA_RAM_SEC, CFG_POS_SEC_FB0_START, CFG_SEC_FB_SIZE); register_phys_mem(MEM_AREA_RAM_SEC, CFG_POS_SEC_FB1_START, CFG_SEC_FB_SIZE);
Could you please confirm (or deny) that our first solution is the proper way to allocate these secure frame buffers and that they will not be overwritten by the process of loading TAs?
It looks like something went wrong in the first solution. The failure in the last solution is because OP-TEE thinks that you've accidentally tried to configure non-secure memory as secure memory.
If this is not the proper way, could you give us some pointers on how to do this? We do not feel comfortable allocating such large areas on the heap because of good taste and because we're not sure yet whether or not there are additional alignment requirement for these frame buffers. Perhaps something similar to the Linux slab-allocator is available?
I think the easiest is to allocate memory from the pool used for TAs with: mm = tee_mm_alloc(&tee_mm_sec_ddr, size); Until this mm is freed no one else will use the memory it represents. Returned memory is page aligned (based on CORE_MMU_USER_CODE_SHIFT).
This depends on paging being disabled, which it seems it is for the IMX platforms. If paging would have been enabled this memory might have been less secure than what you require, depending on how the platform is configured.
Cheers, Jens
Hi Jens,
Thank you very much for your prompt answer.
It looks like something went wrong in the first solution.
Ok. Since I don't fully understand the failure mode, I'm going to abandon that solution.
The failure in the last solution is because OP-TEE thinks that you've accidentally tried to configure non-secure memory as secure memory.
Ok, I figured that much, so that's clearly not the way to go.
I think the easiest is to allocate memory from the pool used for TAs with: mm = tee_mm_alloc(&tee_mm_sec_ddr, size); Until this mm is freed no one else will use the memory it represents.
That's a great idea! And thanks to this solution, I don't have to discard half a day of work refactoring the code to use calloc. because It was fairly easy to convert from calloc to tee_mm_alloc.
Returned memory is page aligned (based on CORE_MMU_USER_CODE_SHIFT).
I wasn't really sure exactly what it is that tee_mm_alloc() returns, but two consecutive calls for 0x96000 bytes of data produced returned values (1st = 0x8e076e60, 2nd = 0x8e076e48) of only 0x18 bytes apart. So perhaps they're TLB entries? But I went through the code for examples and this is what I came up with:
/* Allocate memory for all frame buffers */ for (ndx = 0; ndx < ARRAY_SIZE(fb); ndx++) { tee_mm_entry_t *mm;
if (!(mm = tee_mm_alloc(&tee_mm_sec_ddr, MXSFB_SIZE))) { EMSG("Error allocating fb[%d]\n", ndx); return; } fb[ndx].va = phys_to_virt(tee_mm_get_smem(mm), MEM_AREA_TA_RAM); fb[ndx].pa = (void *)virt_to_phys(fb[ndx].va); DMSG("fb[%d] @%p (pa %p)\n", ndx, fb[ndx].va, fb[ndx].pa); memset(fb[ndx].va, 0x00, MXSFB_SIZE); } Does that look al right to you? Could you suggest a more elegant way to convert the contents of mm to fb[ndx].va and fb[ndx].pa? Is the memset() still necessary? I didn't see that in any of the examples I've found, and it's an expensive function. Is the memory allocated with tee_mm_alloc() garranteed to be a single block of consecutive pages?
This depends on paging being disabled, which it seems it is for the IMX platforms. If paging would have been enabled this memory might have been less secure than what you require, depending on how the platform is configured.
Paging is disabled, so we're good here. Thank you for making me check.
Again, thank you very much!
On Wed, Dec 2, 2020 at 9:28 PM Robert Deliën robert@delien.nl wrote:
Hi Jens,
Thank you very much for your prompt answer.
It looks like something went wrong in the first solution.
Ok. Since I don't fully understand the failure mode, I'm going to abandon that solution.
The failure in the last solution is because OP-TEE thinks that you've accidentally tried to configure non-secure memory as secure memory.
Ok, I figured that much, so that's clearly not the way to go.
I think the easiest is to allocate memory from the pool used for TAs with: mm = tee_mm_alloc(&tee_mm_sec_ddr, size); Until this mm is freed no one else will use the memory it represents.
That's a great idea! And thanks to this solution, I don't have to discard half a day of work refactoring the code to use calloc. because It was fairly easy to convert from calloc to tee_mm_alloc.
Returned memory is page aligned (based on CORE_MMU_USER_CODE_SHIFT).
I wasn't really sure exactly what it is that tee_mm_alloc() returns, but two consecutive calls for 0x96000 bytes of data produced returned values (1st = 0x8e076e60, 2nd = 0x8e076e48) of only 0x18 bytes apart. So perhaps they're TLB entries? But I went through the code for examples and this is what I came up with:
/* Allocate memory for all frame buffers */ for (ndx = 0; ndx < ARRAY_SIZE(fb); ndx++) { tee_mm_entry_t *mm; if (!(mm = tee_mm_alloc(&tee_mm_sec_ddr, MXSFB_SIZE))) { EMSG("Error allocating fb[%d]\n", ndx); return; } fb[ndx].va = phys_to_virt(tee_mm_get_smem(mm), MEM_AREA_TA_RAM); fb[ndx].pa = (void *)virt_to_phys(fb[ndx].va); DMSG("fb[%d] @%p (pa %p)\n", ndx, fb[ndx].va, fb[ndx].pa); memset(fb[ndx].va, 0x00, MXSFB_SIZE); }Does that look al right to you? Could you suggest a more elegant way to convert the contents of mm to fb[ndx].va and fb[ndx].pa? Is the memset() still necessary? I didn't see that in any of the examples I've found, and it's an expensive function. Is the memory allocated with tee_mm_alloc() garranteed to be a single block of consecutive pages?
Looks good, the tee_mm functions handle resource allocation typically virtual or physical memory. It doesn't touch the memory itself. The code above looks OK. Yep, the memory will be physically contiguous since the entire TA RAM range (represented here by tee_mm_sec_ddr) is mapped with physically contiguous memory.
Cheers, Jens
This depends on paging being disabled, which it seems it is for the IMX platforms. If paging would have been enabled this memory might have been less secure than what you require, depending on how the platform is configured.
Paging is disabled, so we're good here. Thank you for making me check.
Again, thank you very much!
Hi,
Again, thank you very much for your prompt answer.
Looks good, the tee_mm functions handle resource allocation typically virtual or physical memory. It doesn't touch the memory itself. The code above looks OK. Yep, the memory will be physically contiguous since the entire TA RAM range (represented here by tee_mm_sec_ddr) is mapped with physically contiguous memory.
The code 'works' beautifully. That is; The FB addresses it produces seem plausible: D/TC:0 0 imx_lcdif_init:334 fb[0] @0x88e00000 (pa 0x8e100000) D/TC:0 0 imx_lcdif_init:334 fb[1] @0x88e96000 (pa 0x8e196000) Even accessing the buffer using the corresponding virtual addresses produces no Abort faults.
Unfortunately, it doesn't produce an image on the LCD either. However, if I (illegally) re-use part of shared memory as a frame buffer, I _do_ get an image. Details from conf.mk: CFG_DRAM_BASE = 0x80000000 CFG_DDR_SIZE = 0x10000000 CFG_SHMEM_SIZE = 0x00200000 CFG_SHMEM_START = ($(CFG_DRAM_BASE) + $(CFG_DDR_SIZE) - $(CFG_SHMEM_SIZE))
Details from code: fb_pa = 0x90000000 - FB_SIZE; fb_va = (void *fb_pa, MEM_AREA_NSEC_SHM);
We are starting to suspect that the LCD peripheral is denied access to the frame buffer by the TZ controller, and this specific peripheral's bus master doesn't seem to have corresponding Supervisor mode bits in the HP0 or SA register.
Now I do know that this problem is beyond the scope of this mailing list, but if anybody on this list knows more about this, I'd be more than happy to hear about it!
With kind regards,
Robert.
op-tee@lists.trustedfirmware.org
-
Jens Wiklander -
Robert Deliën