arm64: Fix TTBR + PAN + 52-bit PA logic in cpu_do_switch_mm
PATCH 1/1 arm64: Fix TTBR + PAN + 52-bit PA logic in cpu_do_switch_mm
In cpu_do_switch_mm(.) with ARM64_SW_TTBR0_PAN=y we apply phys_to_ttbr to a value that already has an ASID inserted into the upper bits. For 52-bit PA configurations this then can give us TTBR0_EL1 registers that cause translation table walks to attempt to access non-zero PA[51:48] spuriously. Ultimately leading to a Synchronous External Abort on level 1 translation.
This patch re-arranges the logic in cpu_do_switch_mm(.) such that phys_to_ttbr is called before the ASID is inserted into the TTBR0 value.
---
This applies to the arm64 for-next/core branch.
--- arch/arm64/mm/proc.S | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
arch/arm64/mm/proc.S +3 -3
@@ -153,14 +153,14 @@ ENDPROC(cpu_do_resume) ENTRY(cpu_do_switch_mm) mrs x2, ttbr1_el1 mmid x1, x1 // get mm->context.id + phys_to_ttbr x0, x3 #ifdef CONFIG_ARM64_SW_TTBR0_PAN - bfi x0, x1, #48, #16 // set the ASID field in TTBR0 + bfi x3, x1, #48, #16 // set the ASID field in TTBR0 #endif bfi x2, x1, #48, #16 // set the ASID msr ttbr1_el1, x2 // in TTBR1 (since TCR.A1 is set) isb - phys_to_ttbr x0, x2 - msr ttbr0_el1, x2 // now update TTBR0 + msr ttbr0_el1, x3 // now update TTBR0 isb b post_ttbr_update_workaround // Back to C code... ENDPROC(cpu_do_switch_mm)
Signature · 1 line
2.11.0
Catalin Marinas
maintainerPATCH 1/1 arm64: Fix TTBR + PAN + 52-bit PA logic in cpu_do_switch_mm
Quoted 31 lines · Steve Capper
In
cpu_do_switch_mm(.)with ARM64_SW_TTBR0_PAN=y we applyphys_to_ttbrto a value that already has an ASID inserted into the upper bits. For 52-bit PA configurations this then can give usTTBR0_EL1registers that cause translation table walks to attempt to access non-zero PA[51:48] spuriously. Ultimately leading to a Synchronous External Abort on level 1 translation.This patch re-arranges the logic in
cpu_do_switch_mm(.)such thatphys_to_ttbris called before the ASID is inserted into the TTBR0 value.Signed-off-by Steve Capper <[email protected]>---
This applies to the arm64 for-next/core branch.
--- arch/arm64/mm/proc.S | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)arch/arm64/mm/proc.S +2
-1@@ -153,14 +153,14 @@ ENDPROC(cpu_do_resume) ENTRY(cpu_do_switch_mm) mrs x2, ttbr1_el1 mmid x1, x1 // get mm->context.id + phys_to_ttbr x0, x3 #ifdef CONFIG_ARM64_SW_TTBR0_PAN - bfi x0, x1, #48, #16 // set the ASID field in TTBR0 + bfi x3, x1, #48, #16 // set the ASID field in TTBR0
arch/arm64/mm/proc.S ENDPROC(cpu_do_resume) · line 166
Quoted 10 lines
arch/arm64/mm/proc.S +1
-2#endif bfi x2, x1, #48, #16 // set the ASID msr ttbr1_el1, x2 // in TTBR1 (since TCR.A1 is set) isb - phys_to_ttbr x0, x2 - msr ttbr0_el1, x2 // now update TTBR0 + msr ttbr0_el1, x3 // now update TTBR0 isb b post_ttbr_update_workaround // Back to C code... ENDPROC(cpu_do_switch_mm)
Thanks Steve. It looks like I messed up the merge: 1f911c3a1140 ("Merge branch 'for-next/52-bit-pa' into for-next/core").
I'll wait for Kristina and Suzuki to ack but considered it queued.
Signature · 1 line
Catalin
PATCH 1/1 arm64: Fix TTBR + PAN + 52-bit PA logic in cpu_do_switch_mm
Quoted 34 lines · Catalin Marinas
Quoted 33 lines · Steve Capper
In
cpu_do_switch_mm(.)with ARM64_SW_TTBR0_PAN=y we applyphys_to_ttbrto a value that already has an ASID inserted into the upper bits. For 52-bit PA configurations this then can give usTTBR0_EL1registers that cause translation table walks to attempt to access non-zero PA[51:48] spuriously. Ultimately leading to a Synchronous External Abort on level 1 translation.This patch re-arranges the logic in
cpu_do_switch_mm(.)such thatphys_to_ttbris called before the ASID is inserted into the TTBR0 value.Signed-off-by Steve Capper <[email protected]>---
This applies to the arm64 for-next/core branch.
--- arch/arm64/mm/proc.S | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)arch/arm64/mm/proc.S +2
-1@@ -153,14 +153,14 @@ ENDPROC(cpu_do_resume) ENTRY(cpu_do_switch_mm) mrs x2, ttbr1_el1 mmid x1, x1 // get mm->context.id + phys_to_ttbr x0, x3 #ifdef CONFIG_ARM64_SW_TTBR0_PAN - bfi x0, x1, #48, #16 // set the ASID field in TTBR0 + bfi x3, x1, #48, #16 // set the ASID field in TTBR0 #endif bfi x2, x1, #48, #16 // set the ASID
Quoted 8 lines
arch/arm64/mm/proc.S ENDPROC(cpu_do_resume) · line 166
Quoted 8 lines
arch/arm64/mm/proc.S +1
-2msr ttbr1_el1, x2 // in TTBR1 (since TCR.A1 is set) isb - phys_to_ttbr x0, x2 - msr ttbr0_el1, x2 // now update TTBR0 + msr ttbr0_el1, x3 // now update TTBR0 isb b post_ttbr_update_workaround // Back to C code... ENDPROC(cpu_do_switch_mm)
Thanks for finding and fixing this Steve, the patch looks good to me and I can reproduce the issue as well as confirm that this patch fixes it:
Quoted 2 lines
Thanks Steve. It looks like I messed up the merge:
1f911c3a1140("Merge branch 'for-next/52-bit-pa' into for-next/core").
The merge commit is fine, the bug was introduced in commit 6b88a32c7af6 ("arm64: kpti: Fix the interaction between ASID switching and software PAN").
Kristina
Quoted 3 lines
I'll wait for Kristina and Suzuki to ack but considered it queued.
Catalin Marinas
maintainerPATCH 1/1 arm64: Fix TTBR + PAN + 52-bit PA logic in cpu_do_switch_mm
Quoted 50 lines · Kristina Martsenko
Quoted 34 lines · Catalin Marinas
Quoted 33 lines · Steve Capper
In
cpu_do_switch_mm(.)with ARM64_SW_TTBR0_PAN=y we applyphys_to_ttbrto a value that already has an ASID inserted into the upper bits. For 52-bit PA configurations this then can give usTTBR0_EL1registers that cause translation table walks to attempt to access non-zero PA[51:48] spuriously. Ultimately leading to a Synchronous External Abort on level 1 translation.This patch re-arranges the logic in
cpu_do_switch_mm(.)such thatphys_to_ttbris called before the ASID is inserted into the TTBR0 value.Signed-off-by Steve Capper <[email protected]>---
This applies to the arm64 for-next/core branch.
--- arch/arm64/mm/proc.S | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)arch/arm64/mm/proc.S +2
-1@@ -153,14 +153,14 @@ ENDPROC(cpu_do_resume) ENTRY(cpu_do_switch_mm) mrs x2, ttbr1_el1 mmid x1, x1 // get mm->context.id + phys_to_ttbr x0, x3 #ifdef CONFIG_ARM64_SW_TTBR0_PAN - bfi x0, x1, #48, #16 // set the ASID field in TTBR0 + bfi x3, x1, #48, #16 // set the ASID field in TTBR0 #endif bfi x2, x1, #48, #16 // set the ASIDQuoted 8 lines
arch/arm64/mm/proc.S ENDPROC(cpu_do_resume) · line 166
Quoted 8 lines
arch/arm64/mm/proc.S +1
-2msr ttbr1_el1, x2 // in TTBR1 (since TCR.A1 is set) isb - phys_to_ttbr x0, x2 - msr ttbr0_el1, x2 // now update TTBR0 + msr ttbr0_el1, x3 // now update TTBR0 isb b post_ttbr_update_workaround // Back to C code... ENDPROC(cpu_do_switch_mm)Thanks for finding and fixing this Steve, the patch looks good to me and I can reproduce the issue as well as confirm that this patch fixes it:
Tested-by Kristina Martsenko <[email protected]>Reviewed-by Kristina Martsenko <[email protected]>
Quoted 6 lines
Quoted 2 lines
Thanks Steve. It looks like I messed up the merge:
1f911c3a1140("Merge branch 'for-next/52-bit-pa' into for-next/core").The merge commit is fine, the bug was introduced in commit
6b88a32c7af6("arm64: kpti: Fix the interaction between ASID switching and software PAN").
Good point. I was bouncing between two versions of this patch, one for the kpti branch and the other for for-next/core (the former doesn't have the 52-bit PA).
I'll amend the commit.
Signature · 1 line
Catalin
PATCH 1/1 arm64: Fix TTBR + PAN + 52-bit PA logic in cpu_do_switch_mm
In reply to Steve CapperQuoted 32 lines · Steve Capper
In
cpu_do_switch_mm(.)with ARM64_SW_TTBR0_PAN=y we applyphys_to_ttbrto a value that already has an ASID inserted into the upper bits. For 52-bit PA configurations this then can give usTTBR0_EL1registers that cause translation table walks to attempt to access non-zero PA[51:48] spuriously. Ultimately leading to a Synchronous External Abort on level 1 translation.This patch re-arranges the logic in
cpu_do_switch_mm(.)such thatphys_to_ttbris called before the ASID is inserted into the TTBR0 value.Signed-off-by Steve Capper <[email protected]>---
This applies to the arm64 for-next/core branch.
--- arch/arm64/mm/proc.S | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)arch/arm64/mm/proc.S +2
-1@@ -153,14 +153,14 @@ ENDPROC(cpu_do_resume) ENTRY(cpu_do_switch_mm) mrs x2, ttbr1_el1 mmid x1, x1 // get mm->context.id + phys_to_ttbr x0, x3 #ifdef CONFIG_ARM64_SW_TTBR0_PAN - bfi x0, x1, #48, #16 // set the ASID field in TTBR0 + bfi x3, x1, #48, #16 // set the ASID field in TTBR0
arch/arm64/mm/proc.S ENDPROC(cpu_do_resume) · line 166
Quoted 10 lines
arch/arm64/mm/proc.S +1
-2#endif bfi x2, x1, #48, #16 // set the ASID msr ttbr1_el1, x2 // in TTBR1 (since TCR.A1 is set) isb - phys_to_ttbr x0, x2 - msr ttbr0_el1, x2 // now update TTBR0 + msr ttbr0_el1, x3 // now update TTBR0 isb b post_ttbr_update_workaround // Back to C code... ENDPROC(cpu_do_switch_mm)
Steve,
Nice catch !