i.MX TZASC improvements #7595
i.MX TZASC improvements #7595
Conversation
The i.MX6DP/QP SoCs have a 2nd memory controller as well which must be configured. This commit covers only the i.MX6QP because there is no i.MX6DP OP-TEE platform yet. Signed-off-by: Marco Felsch <[email protected]>
Dumping region0 is interesting too since it may have a insecure sp configuration applied by the previous running firmware. Signed-off-by: Marco Felsch <[email protected]>
|
Please fix the style issues. |
|
Sure, I wasn't quite sure:
|
|
Fixed the style issues except for the |
Yeah, that's a false positive. Please squash in the updates. |
bith3ad
force-pushed
the
topic/tzasc-v2
branch
from
0386e29 to
54e7dbd
Compare
|
Done |
jenswi-linaro
left a comment
jenswi-linaro
left a comment
There was a problem hiding this comment.
A comment for the commit "core: mm: add core_mmu_for_each_nsec_ddr support"
|
For "drivers: tzc380: add tzc_verify_region0_secure helper", please apply: |
|
|
||
| #ifndef CFG_DDR_SIZE | ||
| #error "CFG_DDR_SIZE not defined" | ||
| #if !defined(CFG_DDR_SIZE) && !defined(CFG_EXTERNAL_DT) |
There was a problem hiding this comment.
@bith3ad This commit core: imx: relax CFG_DDR_SIZE decision should not be the part of this PR.
It should be in different PR.
Also we have CFG_EXTERNAL_DT = y on almost all platforms, but we are not using any DTB for these platforms.
It will fail eventually, On which platforms you tested this patch ?
There was a problem hiding this comment.
@bith3ad This commit core: imx: relax CFG_DDR_SIZE decision should not be the part of this PR.
It should be in different PR.
Thiis PR adds the support to make it possible to set CFG_DDR_SIZE=n and to rely on the external-dt information. Why not adding it here?
Also we have CFG_EXTERNAL_DT = y on almost all platforms, but we are not using any DTB for these platforms.
By "we" you meant NXP downstream OP-TEE? Mainline ARMv8 NXP platforms don't enable it per default, so for all mainline default-value users it is disabled.
It will fail eventually, On which platforms you tested this patch ?
How so? This just relaxes the static compile check by taking the CFG_EXTERNAL_DT into account too. I have tested it on NXP-IMX8MP-EVK (6G) and Debix-SoM (8G).
That's fine, But IMO this patch is in general fixup and should not be included in a patch set containing TZASC improvements.
For i.MX6/7 platforms CFG_DT = y and CFG_EXTERNAL_DT ?= $(CFG_DT) is defined as this, how this will work on these platforms ? |
Yep I know, therefore I wrote ARMv8 platforms. I don't know how exactly this commit should break the existing NXP ARMv7 platforms. The previous commit which adds the support to dyn. detect the NS memory could influence these systems but not in a negative manner. I would rather say it improves these platforms as well since with my changes applied the "actual" non-secure memory gets exposed via region1 NS-RW. "actual" means all non-secure memory removed by OP-TEE core. At the moment the NXP TZC integration exposes the whole memory as non-secure and rely on the fact that region2 (secure OS region) overlays the below region1. |
Add a helper which verifies that region0 is only accessible by the secure world. Signed-off-by: Marco Felsch <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>
There are platforms where memory aliasing can't be prevented, e.g. the i.MX8M. If the previous running firmware configured region0, which covers the whole AXI address space, to be accessible from secure and non-secure world the OP-TEE core memory would be accessible via memory aliasing. To prevent such attacks we need to ensure that region0 is accessible from the secure world only. Signed-off-by: Marco Felsch <[email protected]>
If OP-TEE is used the TZASC should be enabled to validate the memory access. This adds the initial support for the i.MX6 and i.MX8M to check if the TZASC is enabled and throw a panic if not. Once all platforms are covered this CFG_TZASC_CHECK_ENABLED should be removed and the check should be done by default to enforce that the TZASC is running. Signed-off-by: Marco Felsch <[email protected]>
Bundle the region number handling within imx_tzc_auto_configure() to make it possible to call it without ext. context. This is required for the upcoming dynamic ddr size configuration. Signed-off-by: Marco Felsch <[email protected]>
Currently all TZC drivers implement the nsec_ddr configuration via compile time configuration switches. This fact is not ideal for platforms which have various DRAM settings. OP-TEE already supports discovering the nsec_ddr chunks during the early boot process but doesn't expose this information. Therefore this foreach helper is added which can be used by the TZC drivers to address the above use-case. Signed-off-by: Marco Felsch <[email protected]>
Convert the driver to use the new core_mmu_for_each_nsec_ddr() to allow dynamic configurations of the NS DRAM region(s). The DRAM configuration parsed by the OP-TEE core is either based on: - manifest-dt - external-dt - internal/embedded-dt - builtin compile-time defines This logic allows the imx-tzc380 driver to use the runtime information provided by an external DT. The compile-time builtin defines are used if no external DT is found or the external DT doesn't contain any memory information. For plat-imx this mapps to register_ddr(CFG_DRAM_BASE, CFG_DDR_SIZE), which is equivalent to imx_tzc_auto_configure(CFG_DRAM_BASE, CFG_DDR_SIZE, TZC_ATTR_SP_NS_RW). Signed-off-by: Marco Felsch <[email protected]>
The overall NS DRAM size can be passed via DT if CFG_EXTERNAL_DT is enabled. So don't throw an error in case no size was specified. Signed-off-by: Marco Felsch <[email protected]>
bith3ad
force-pushed
the
topic/tzasc-v2
branch
from
54e7dbd to
6120d7b
Compare
|
| int end = 1; | ||
| int i = 0; | ||
|
|
||
| addr[0] = core_mmu_get_va(TZASC_BASE, MEM_AREA_IO_SEC, 1); |
There was a problem hiding this comment.
If we don't have i.MX6DP platform enabled in OP-TEE, then commit description should not mention that.
There was a problem hiding this comment.
Sorry but where do I mention the i.MX6DP here?
There was a problem hiding this comment.
Not sure whats wrong with the commit message. I could drop it but in that case one could say, what's with the i.MX6DP? Therefore I mentioned the i.MX6DP explicit here to make it clear, that I'm aware that this commit doesn't cover the i.MX6DP.
There was a problem hiding this comment.
Sorry, If I am not clear.
We have only i.MX6QP platform in optee-os as of now, why we are mentioning i.MX6DP platform in the commit message ?
There was a problem hiding this comment.
Because I'm aware that NXP sells i.MX6DP as well. Not sure why mention a platform is such a big deal. The commit message is pretty clear and very hard to get wrong. But if you insist on dropping the i.MX6DP from the message, I will do so.
| * previous running firmware. Region0 covers the complete | ||
| * platform AXI address space. | ||
| */ | ||
| if (IS_ENABLED(CFG_TZASC_REGION0_SECURE) && |
There was a problem hiding this comment.
We already have a patch incoming in Q4 release, which is solving this issue by configuring region0.
/*
* It is possible to access memory protected by the TZASC in
* case the DDR installed is smaller than the memory space
* supported by the controller. (Ref: RM, section about the
* TZASC: "Address Mapping in various memory mapping modes").
*
* Without aliasing protection it is possible to use an address
* outside of the DDR ranged and bypass TZASC protection.
*/
tzc_configure_region(0, 0x00000000, TZC_ATTR_SP_S_RW);
We need to add this configuration for region 0.
There was a problem hiding this comment.
Sorry, but what do you mean by:
We need to add this configuration for region 0.
?
Do you mean, that CFG_TZASC_REGION0_SECURE is no longer required?
There was a problem hiding this comment.
@sahilnxp I rebased my local branch on top of the current master and there is no such patch. Please don't refer to your internal release.
Also I would like to not accept such patch upstream. If someone (BL2, BL31) played around with the TZASC region0, it should be fixed there. According the TZASC ARM TRM, region0 is secure-only per default.
The chances are very high that you will raise cache issues if some pre-bootloader messed around with the region0. E.g. BL2 (barebox-pbl) configured region0 as NS and passed a memory location which can be used by OP-TEE to pass data to BL33. Now OP-TEE reconfigures region0 to S and writes the data required for BL33. BL33 (barebox) which is now executed in the NS world, tries to retrieve the data from OP-TEE which will fail.
Therefore all OP-TEE pre-bootloaders must configure an early region1 and leave the region0 untouched.
There was a problem hiding this comment.
So, you mean to say, this should be decided by the CFG_TZASC_REGION0_SECURE, if it is enabled then in cases you mentioned above OP-TEE will panic.
But what about protecting memory aliasing in this case ? For protecting memory aliasing need to configure region0 as secure.
Also in your commit message, you are trying to solve the memory aliasing issue, but here with CFG_TZASC_REGION0_SECURE option, again giving an option
There was a problem hiding this comment.
CFG_TZASC_REGION0_SECURE just enables the region0 check. I didn't want to cause any regression on existing systems, therefore I added the CFG_ switch. This CFG_ switch can be dropped once all platforms can ensure that the region0 is secure.
But what about protecting memory aliasing in this case ? For protecting memory aliasing need to configure region0 as secure.
Sorry but can you please read my code-comment. I have written that region0 is secure per default if not changed by any previous running firmware (u-boot spl, barebox-pbl, tf-a). That region0 is secure per default is written in the ARM TZC380 TRM.
Also in your commit message, you are trying to solve the memory aliasing issue, but here with CFG_TZASC_REGION0_SECURE option, again giving an option
It is solved by panic the system if region0 is not marked as secure.
|
Gentle ping. |
| temp_32reg = tzc_read_region_attributes(tzc.base, n); | ||
| if (!(temp_32reg & TZC_ATTR_REGION_EN_MASK)) | ||
|
|
||
| /* |
There was a problem hiding this comment.
If I understood this correctly, without this patch it will dump region0 information, right ?
There was a problem hiding this comment.
It will not dumped because the TZC_ATTR_REGION_EN_MASK is marked as reserved and therefore can be 0.
Since region0 is always enabled, the TZC_ATTR_REGION_EN_MASK check can be skipped.
I thought, that I made it clear within the below comment.
| #else | ||
| register_phys_mem(MEM_AREA_IO_SEC, IOMUXC_BASE, IOMUXC_SIZE); | ||
| #endif | ||
| #elif defined(IOMUXC_GPR_BASE) |
There was a problem hiding this comment.
Shouldn't this be a check on CFG_MX8M
There was a problem hiding this comment.
In case of i.MX6ULL platform for instance, this code will do:
register_phys_mem(MEM_AREA_IO_SEC, IOMUXC_GPR_BASE, IOMUXC_SIZE); first and also IOMUXC_GPR_BASE is defined for all i.MX6 platforms in core/arch/arm/plat-imx/registers/imx6.h this will again do the register_phys_mem(MEM_AREA_IO_SEC, IOMUXC_GPR_BASE, IOMUXC_SIZE); after #elif defined(IOMUXC_GPR_BASE)
Same statement will execute 2 times.
There was a problem hiding this comment.
Same statement will execute 2 times.
It won't be executed twice because in case of the i.MX6ULL it will always trigger the first nested #ifdef block since CFG_MX6 and CFG_MX6ULL is always defined.
| * i.MX6 needs special handling due to the different GPR locations. | ||
| */ | ||
| #if defined(CFG_MX6) | ||
| #if defined(CFG_MX6UL) || defined(CFG_MX6ULL) || \ |
There was a problem hiding this comment.
TZC380 driver is not enabled on all of MX6 platforms, it is enabled on MX6UL, MX6ULL, MX6Q, MX6D and MX6DL and moreover this pull request is not tested on any of these platforms, Only tested on i.MX8MP
There was a problem hiding this comment.
Why do you think that this PR was only tested on i.MX8MP?
We tested this on i.MX6UL(L) and i.MX6D which is basically a i.MX6Q in several projects!
I need to ask if i.MX6DL was tested as well.
Furthermore this register_phys_mem() define was added to cover all current MX6 platforms to make the driver ready for all platforms. It doesn't hurt, that we cover all MX6 platforms if there are platforms which don't use this driver.
And last but least, the driver support can be simply enabled externally via the CFG_TZC380=y switch, we basically do for all our projects. So because it's not enabled per default within the OP-TEE Makefile doesn't mean that the driver is not used for platforms like CFG_MX6SLL.
There was a problem hiding this comment.
You mentioned here in this comment that you tested on NXP-IMX8MP-EVK (6G) and Debix-SoM (8G). and : https://github.com/OP-TEE/optee_os/pull/7595/changes/0c6dca2b442252988bb291b0a1bb9735e2227f60..6120d7bdea6e5af9f20947461bc5a9d3327d9756#r2514042393
Were you talking about that commit only ?
There was a problem hiding this comment.
I see. Yes, my comment was related to this particular commit.
|
@sahilnxp can you please point to the points, which are not yet clear to you? I wanna get this PR merged. |
|
I tested this PR on i.MX8MM platform on my side and hitting panic because region0 is not secure. I am currently debugging this |
Yes, U-Boot is affected by the region0 reconfiguration as well. We fixed this in barebox for i.MX6 and there are patches for i.MX8M as well. The mainline TF-A port for i.MX8M also reconfigured it, but I already send patches to fix this, which are merged. In the end, this shows you, that your secure-configured, platform is not so secure, which was the intention by this commit. You could use the |
Are you talking about this patch set https://lore.kernel.org/barebox/[email protected]/T/#m9efb0988a0b594ba23fc8622671f2828c07a1e0f ? Can you point me to ATF patch also. |
The above link targets the i.MX6 and i.MX6UL(L) and is already in barebox mainline, i.MX8M isn't fixed yet. |
Please correct me if I am wrong, does this means that till the time these fixes (TZASC region0 should not be changed) are not available in uboot we will be seeing this panic ? |
|
I am testing this PR and seeing below panic on i.MX6DL board, any idea why so ? 
Full logs here: imx6dl_tzasc_logs.txt Looks like tzasc configuration is initiated again after doing once and the number of tzasc regions got exhausted in that call. All other platforms are booting up fine but having issue while suspend/resume test. |
|
|
||
| static void imx_tzc_auto_configure(vaddr_t addr, vaddr_t rsize, uint32_t attr) | ||
| { | ||
| static uint8_t region = 1; |
There was a problem hiding this comment.
Need to remove this static variable, because in case of suspend/resume TZASC configuration is done again and region starts from the last incremented value which increases beyond the tzc.num_region and results in this assert getting hit.
There was a problem hiding this comment.
I came to the same conclusion but why do you need to re-run setting up the firewall? Doesn't this mean, that the whole security concept is broken if the firewall bits are lost after a suspend/resume cycle and a person having access resume address could hack us (gain access to the whole DRAM)? Can you run a dump of the tzasc settings while performing the resume please?
There was a problem hiding this comment.
I am also looking at this thing; I will share the dump.
You will see this panic if you configured your system as secure aka |
Why do we need to reconfigure the firewall after a suspend? I thought that those important state is kept during suspend :-/ if that isn't the case, the whole system is insecure right? |
I added the patch on top of your patch to mark region0 as secure, so this check under |
Looking into this, will get back to you with more data. |
As explained here: #7595 (comment) I highly recommend to not do that to not cause any cache issues later on. |
3 participants
Hi,
this PR is based on #7455 and includes all the feedback I received. Unfortunately the automatic bot closed the PR so I couldn't follow the contributors-guideline.
Furthermore this v2 add additional patches to configure the TZASC automatically based on the dt-information if any.