- TF-A - lists.trustedfirmware.org

Re: [TF-A] Erroneous speculative AT workaround

by Raghu K

This is interesting. It appears that there is no way on entry to EL3 to guarantee that the out-of-context(el2 and el1) translation regimes are in a consistent state and on every entry into EL3, we have to conservatively assume that it is in an inconsistent state. This is because of the situation Andrew mentioned(interrupts to EL3 can occur at any time). If this is the case, on EL3 entry: 1) For EL1, we will need to save SCTLR_EL1, set SCTLR_EL1.M = 1,.EPDx = 0 2) Set whatever bits we need to for EL2 and S2 translations to not succeed(What are these?) 3) DSB, to ensure no speculative AT can be issued until completion of DSB, so any AT that occurs will not fill the TLB with bad translations. On exit(right before ERET), we need to restore the registers saved on entry, and have the ERET followed by a DSB so that there can be no speculative execution of AT instructions. Thanks Raghu On 7/1/20 5:54 AM, Marc Zyngier via TF-A wrote: > Hi Manish, > > On Wed, 1 Jul 2020 at 13:14, Manish Badarkhe <Manish.Badarkhe(a)arm.com> wrote: >> Hi Andrew, >> >> As per current implementation, in “el1_sysregs_context_restore” routine do below things: >> >> 1. TCR_EL1.EPD0 = 1 >> 2. TCR_EL1.EPD1 = 1 >> 3. SCTR_EL1.M = 0 >> 4. Isb >> Code snippet: >> mrs x9, tcr_el1 >> orr x9, x9, #TCR_EPD0_BIT >> orr x9, x9, #TCR_EPD1_BIT >> msr tcr_el1, x9 >> mrs x9, sctlr_el1 >> bic x9, x9, #SCTLR_M_BIT >> msr sctlr_el1, x9 >> isb >> This is to avoid PTW through while updating system registers at step 5 > Unfortunately, this doesn't prevent anything. > > If SCTLR_EL1.M is clear, TCR_EL1.EPDx don't mean much (S1 MMU is > disabled, no S1 page table walk), and you can still have S2 PTWs > (using an idmap for S1) and creating TLB corruption if these entry > alias with any S1 mapping that exists at EL1. > > Which is why KVM does *set* SCTLR_EL1.M, which prevents the use of a > 1:1 mapping at S1, and at which point the TCR_EL1.EPDx bits are > actually useful in preventing a PTW. > >> 5. Restore all system registers for El1 except SCTLR_EL1 and TCR_EL1 >> 6. isb() >> 7. restore SCTLR_EL1 and TCR_EL1 >> Code Snippet: >> ldr x9, [x0, #CTX_SCTLR_EL1] -> saved value from "el2_sysregs_context_save" >> msr sctlr_el1, x9 >> ldr x9, [x0, #CTX_TCR_EL1] >> msr tcr_el1, x9 >> >> As per above steps. SCTLR_EL1 get restored back with actual settings at step 7. >> Similar flow is present for “el2_sysregs_context_restore” to restore SCTLR_EL1 register. >> >> In conclusion, this routine temporarily clear M bit of SCTLR_EL1 to avoid speculation but restored it back >> to its original setting while leaving back to its caller. Please let us know whether this align with KVM >> workaround for speculative AT erratum. > It doesn't, unfortunately. I believe this code actively creates > problems on a system that is affected by speculative AT execution. > > I don't understand your rationale for touching SCTLR_EL2.M either if > you are not context-switching the EL2 S1 state: as far as I understand > no affected cores have S-EL2, so no switch should happen at this > stage. > > Thanks, > > M.

5 years, 12 months

3
2
0 0

Re: [TF-A] Erroneous speculative AT workaround

by Manish Badarkhe

Hi Andrew, As per current implementation, in “el1_sysregs_context_restore” routine do below things: 1. TCR_EL1.EPD0 = 1 2. TCR_EL1.EPD1 = 1 3. SCTR_EL1.M = 0 4. Isb Code snippet: mrs x9, tcr_el1 orr x9, x9, #TCR_EPD0_BIT orr x9, x9, #TCR_EPD1_BIT msr tcr_el1, x9 mrs x9, sctlr_el1 bic x9, x9, #SCTLR_M_BIT msr sctlr_el1, x9 isb This is to avoid PTW through while updating system registers at step 5 5. Restore all system registers for El1 except SCTLR_EL1 and TCR_EL1 6. isb() 7. restore SCTLR_EL1 and TCR_EL1 Code Snippet: ldr x9, [x0, #CTX_SCTLR_EL1] -> saved value from "el2_sysregs_context_save" msr sctlr_el1, x9 ldr x9, [x0, #CTX_TCR_EL1] msr tcr_el1, x9 As per above steps. SCTLR_EL1 get restored back with actual settings at step 7. Similar flow is present for “el2_sysregs_context_restore” to restore SCTLR_EL1 register. In conclusion, this routine temporarily clear M bit of SCTLR_EL1 to avoid speculation but restored it back to its original setting while leaving back to its caller. Please let us know whether this align with KVM workaround for speculative AT erratum. Thanks Manish Badarkhe On 01/07/2020, 17:02, "TF-A on behalf of Joanna Farley via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote: Thanks Andrew for the notification. We will look into it and get back to this thread. Thanks Joanna On 01/07/2020, 10:16, "TF-A on behalf of Andrew Scull via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote: Whilst looking at the speculative AT workaround in KVM, I compared it against the workaround in TF-A and noticed an inconsistency whereby TF-A **breaks** KVM's workaround. In `el1_sysregs_context_restore`, the M bit of SCTRL_EL1 is cleared however Linux requires this to be set for its workaround to be correct. If an exception is taken to EL3 partway through a VM context switch, e.g. a secure interrupt, causing a switch to the secure world, TF-A will reintroduce the possibility of TLB corruption. The above explains how it is broken for Linux's chosen workaround however TF-A will also have to be compatible with whatever workaround the EL2 software is using. Starting this thread with the issue identified and we can add more details as needed. Thanks Andrew -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a

6 years

2
1
0 0

Re: [TF-A] Erroneous speculative AT workaround

by Manish Badarkhe

+Updated comment for SCTLR_EL2 register restoration. On 01/07/2020, 17:45, "TF-A on behalf of Manish Badarkhe via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote: Hi Andrew, As per current implementation, in “el1_sysregs_context_restore” routine do below things: 1. TCR_EL1.EPD0 = 1 2. TCR_EL1.EPD1 = 1 3. SCTR_EL1.M = 0 4. Isb Code snippet: mrs x9, tcr_el1 orr x9, x9, #TCR_EPD0_BIT orr x9, x9, #TCR_EPD1_BIT msr tcr_el1, x9 mrs x9, sctlr_el1 bic x9, x9, #SCTLR_M_BIT msr sctlr_el1, x9 isb This is to avoid PTW through while updating system registers at step 5 5. Restore all system registers for El1 except SCTLR_EL1 and TCR_EL1 6. isb() 7. restore SCTLR_EL1 and TCR_EL1 Code Snippet: ldr x9, [x0, #CTX_SCTLR_EL1] -> saved value from "el2_sysregs_context_save" msr sctlr_el1, x9 ldr x9, [x0, #CTX_TCR_EL1] msr tcr_el1, x9 As per above steps. SCTLR_EL1 get restored back with actual settings at step 7. Similar flow is present for “el2_sysregs_context_restore” to restore SCTLR_EL2 register. In conclusion, this routine temporarily clear M bit of SCTLR_EL1 to avoid speculation but restored it back to its original setting while leaving back to its caller. Please let us know whether this align with KVM workaround for speculative AT erratum. Thanks Manish Badarkhe On 01/07/2020, 17:02, "TF-A on behalf of Joanna Farley via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote: Thanks Andrew for the notification. We will look into it and get back to this thread. Thanks Joanna On 01/07/2020, 10:16, "TF-A on behalf of Andrew Scull via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote: Whilst looking at the speculative AT workaround in KVM, I compared it against the workaround in TF-A and noticed an inconsistency whereby TF-A **breaks** KVM's workaround. In `el1_sysregs_context_restore`, the M bit of SCTRL_EL1 is cleared however Linux requires this to be set for its workaround to be correct. If an exception is taken to EL3 partway through a VM context switch, e.g. a secure interrupt, causing a switch to the secure world, TF-A will reintroduce the possibility of TLB corruption. The above explains how it is broken for Linux's chosen workaround however TF-A will also have to be compatible with whatever workaround the EL2 software is using. Starting this thread with the issue identified and we can add more details as needed. Thanks Andrew -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a

6 years

1
0
0 0

Re: [TF-A] Erroneous speculative AT workaround

by Joanna Farley

Thanks Andrew for the notification. We will look into it and get back to this thread. Thanks Joanna On 01/07/2020, 10:16, "TF-A on behalf of Andrew Scull via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote: Whilst looking at the speculative AT workaround in KVM, I compared it against the workaround in TF-A and noticed an inconsistency whereby TF-A **breaks** KVM's workaround. In `el1_sysregs_context_restore`, the M bit of SCTRL_EL1 is cleared however Linux requires this to be set for its workaround to be correct. If an exception is taken to EL3 partway through a VM context switch, e.g. a secure interrupt, causing a switch to the secure world, TF-A will reintroduce the possibility of TLB corruption. The above explains how it is broken for Linux's chosen workaround however TF-A will also have to be compatible with whatever workaround the EL2 software is using. Starting this thread with the issue identified and we can add more details as needed. Thanks Andrew -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a

6 years

1
0
0 0

Erroneous speculative AT workaround

by Andrew Scull

Whilst looking at the speculative AT workaround in KVM, I compared it against the workaround in TF-A and noticed an inconsistency whereby TF-A **breaks** KVM's workaround. In `el1_sysregs_context_restore`, the M bit of SCTRL_EL1 is cleared however Linux requires this to be set for its workaround to be correct. If an exception is taken to EL3 partway through a VM context switch, e.g. a secure interrupt, causing a switch to the secure world, TF-A will reintroduce the possibility of TLB corruption. The above explains how it is broken for Linux's chosen workaround however TF-A will also have to be compatible with whatever workaround the EL2 software is using. Starting this thread with the issue identified and we can add more details as needed. Thanks Andrew

6 years

1
0
0 0

Re: [TF-A] Announcing some changes around the code review label in Gerrit

by Julius Werner

Hi Sandrine, Sounds like good changes in general! I'm curious what the ACLs are for Code-Owner-Review? Is it tied to docs/about/maintainers.rst or just based on the honor system? (I notice that I seem to be able to give a +1 for code I'm not owning, but maybe that's because I am a maintainer?) Also, are code owners allowed to +1 themselves (I think we said we didn't want maintainers to do that, but for code owners I could see how we might want to allow it since there are usually not that many)? What do we do when someone uploads the first patch for a new platform, do they just COR+1 themselves (since there is no code owner yet)? I think it might still be useful to retain the existing Code-Review as a +1/-1 label next to the two new ones, just to allow other interested parties to record their opinion (without it having any enforcing effect on the merge). In other Gerrit instances I have used people often like to give CR+1 as a "I'm not the one who needs to approve this but I have looked at it and think it's fine" or a CR-1 as a "I can't stop you from doing this but I still want to be clear that I don't think it's a good idea". It allows people outside the official approval process a clearer way to participate and can aid the official approvers in their decisions (e.g. when I am reviewing a patch as a maintainer that already has a CR-1 from someone else I know to pay extra attention to their concerns, and it's more visible than just some random comment further up in the list). What do you think? Best regards, Julius

6 years

2
1
0 0

Re: [TF-A] Announcing some changes around the code review label in Gerrit

by Javier Almansa Sobrino

Hi Sandrine, If my understanding is right, Code-Owner-Review is the equivalent to the old Code-Review+1 and therefore anyone can add a vote there, owner or not. If that is so, I think the current name might be misleading. Wouldn't it be better just "Code-Review", for instance? As an example, I am not code owner for Measure Boot, but sometimes I am requested to review the patches for it, so in that case it might lead to confusion if I give Code-Owner-Review score. What do you think? Best regards, Javier -----Original Message----- From: Sandrine Bailleux via TF-A <tf-a(a)lists.trustedfirmware.org<mailto:Sandrine%20Bailleux%20via%20TF-A%20%3ctf-a@lists.trustedfirmware.org%3e>> Reply-To: Sandrine Bailleux <sandrine.bailleux(a)arm.com<mailto:Sandrine%20Bailleux%20%3csandrine.bailleux@arm.com%3e>> To: tf-a <tf-a(a)lists.trustedfirmware.org<mailto:tf-a%20%3ctf-a@lists.trustedfirmware.org%3e>> Subject: [TF-A] Announcing some changes around the code review label in Gerrit Date: Tue, 30 Jun 2020 13:29:00 +0000 Hello all, If you recall, the tf.org Project Maintenance Process [1] advocates a code review model where both code owners and maintainers need to review and approve a patch before it gets merged. Until now, the way we would record this in Gerrit was a bit cumbersome and ambiguous. * Code-Review+1 was for code owners to approve a patch. * Code-Review+2 was for maintainers to approve a patch. The submission rules in Gerrit were such that a patch needed a Code-Review+2 to be merged. This meant that if a maintainer was first to take a look at a patch and approved it (Code-Review+2), Gerrit would allow the patch to be merged without any code owners review. To align better with the Project Maintenance Process, I've done some changes in our Gerrit configuration. You will notice that the Code-Review label is gone and has been replaced by 2 new labels: * Code-Owner-Review * Maintainer-Review Stating the obvious but code owners are expected to vote on the Code-Owner-Review label and maintainers on the Maintainer-Review. Note that maintainers might also be code owners for some modules. If they are doing a "code owner" type of review on a patch, they would vote on the Code-Owner-Review label in this case. Any doubt, please ask. I hope I didn't break anything but if you notice any permissions problems (things you used to be able to do and cannot do anymore, or vice versa!) please let me know. One annoying consequence of this change is that if you've got some patches in review right now and got some votes on the former 'Code-Review' label, these have been partially lost. The history of review comments in Gerrit will still show that somebody had voted Code-Review -1/+1 on your patch but this will no longer appear in the labels summary frame and won't count towards the approvals required to get the patch merged. I could not think of a way to avoid this issue... This should only be temporary, while we transition all in-flight patches to the new code review model. Roughly: * Existing "Code-Review -1/+1" votes will have to be replayed as "Code-Owner-Review -1/+1". * Existing "Code-Review -2/+2" votes will have to be replayed as "Maintainer-Review -1/+1". Sorry for the inconvenience. I hope, once we've gone through the transition period, these changes will make the code review process clearer to everybody. I will update the TF-A documentation to explain all of this in the coming future. Best regards, Sandrine [1] https://developer.trustedfirmware.org/w/collaboration/project-maintenance-p…

6 years

1
0
0 0

New TrustedFirmware.org security incident process is now live

by Dan Handley

Hi all The new TrustedFirmware.org security incident process is now live. This process is described here: https://developer.trustedfirmware.org/w/collaboration/security_center/repor… Initially the process will be used for the following projects: TF-A, TF-M, OP-TEE and Mbed TLS. The security documentation for each project will be updated soon to reflect this change. If you are part of an organization that believes it should receive security vulnerability information before it is made public then please ask your relevant colleagues to register as Trusted Stakeholders as described here: https://developer.trustedfirmware.org/w/collaboration/security_center/trust… Note we prefer individuals in each organization to coordinate their registration requests with each other and to provide us with an email alias managed by your organization instead of us managing a long list of individual addresses. Best regards Dan. (on behalf of the TrustedFirmware.org security team)

6 years

1
0
0 0

Announcing some changes around the code review label in Gerrit

by Sandrine Bailleux

Hello all, If you recall, the tf.org Project Maintenance Process [1] advocates a code review model where both code owners and maintainers need to review and approve a patch before it gets merged. Until now, the way we would record this in Gerrit was a bit cumbersome and ambiguous. * Code-Review+1 was for code owners to approve a patch. * Code-Review+2 was for maintainers to approve a patch. The submission rules in Gerrit were such that a patch needed a Code-Review+2 to be merged. This meant that if a maintainer was first to take a look at a patch and approved it (Code-Review+2), Gerrit would allow the patch to be merged without any code owners review. To align better with the Project Maintenance Process, I've done some changes in our Gerrit configuration. You will notice that the Code-Review label is gone and has been replaced by 2 new labels: * Code-Owner-Review * Maintainer-Review Stating the obvious but code owners are expected to vote on the Code-Owner-Review label and maintainers on the Maintainer-Review. Note that maintainers might also be code owners for some modules. If they are doing a "code owner" type of review on a patch, they would vote on the Code-Owner-Review label in this case. Any doubt, please ask. I hope I didn't break anything but if you notice any permissions problems (things you used to be able to do and cannot do anymore, or vice versa!) please let me know. One annoying consequence of this change is that if you've got some patches in review right now and got some votes on the former 'Code-Review' label, these have been partially lost. The history of review comments in Gerrit will still show that somebody had voted Code-Review -1/+1 on your patch but this will no longer appear in the labels summary frame and won't count towards the approvals required to get the patch merged. I could not think of a way to avoid this issue... This should only be temporary, while we transition all in-flight patches to the new code review model. Roughly: * Existing "Code-Review -1/+1" votes will have to be replayed as "Code-Owner-Review -1/+1". * Existing "Code-Review -2/+2" votes will have to be replayed as "Maintainer-Review -1/+1". Sorry for the inconvenience. I hope, once we've gone through the transition period, these changes will make the code review process clearer to everybody. I will update the TF-A documentation to explain all of this in the coming future. Best regards, Sandrine [1] https://developer.trustedfirmware.org/w/collaboration/project-maintenance-p…

6 years

1
0
0 0

New Defects reported by Coverity Scan for ARM-software/arm-trusted-firmware

by scan-admin＠coverity.com

Hi, Please find the latest report on new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. 1 new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. New defect(s) Reported-by: Coverity Scan Showing 1 of 1 defect(s) ** CID 360213: Null pointer dereferences (NULL_RETURNS) /plat/arm/common/arm_dyn_cfg.c: 96 in arm_bl1_set_mbedtls_heap() ________________________________________________________________________________________________________ *** CID 360213: Null pointer dereferences (NULL_RETURNS) /plat/arm/common/arm_dyn_cfg.c: 96 in arm_bl1_set_mbedtls_heap() 90 * In the latter case, if we still wanted to write in the DTB the heap 91 * information, we would need to call plat_get_mbedtls_heap to retrieve 92 * the default heap's address and size. 93 */ 94 95 tb_fw_config_info = FCONF_GET_PROPERTY(dyn_cfg, dtb, TB_FW_CONFIG_ID); >>> CID 360213: Null pointer dereferences (NULL_RETURNS) >>> Dereferencing "tb_fw_config_info", which is known to be "NULL". 96 tb_fw_cfg_dtb = tb_fw_config_info->config_addr; 97 98 if ((tb_fw_cfg_dtb != 0UL) && (mbedtls_heap_addr != NULL)) { 99 /* As libfdt use void *, we can't avoid this cast */ 100 void *dtb = (void *)tb_fw_cfg_dtb; 101 ________________________________________________________________________________________________________ To view the defects in Coverity Scan visit, https://u2389337.ct.sendgrid.net/ls/click?upn=nJaKvJSIH-2FPAfmty-2BK5tYpPkl…

6 years

1
0
0 0

2026

2025

2024

2023

2022

2021

2020

2019

2018

TF-A