On Fri, Feb 23, 2018 at 06:29:02PM +0000, Ard Biesheuvel wrote:
Stable backport commit 173358a49173 ("arm64: kpti: Add ->enable callback to remap swapper using nG mappings") of upstream commit f992b4dfd58b did not survive the backporting process unscathed, and ends up writing garbage into the TTBR1_EL1 register, rather than pointing it to the zero page to disable translations. Fix that.
Cc: stable@vger.kernel.org #v4.14 Reported-by: Nicolas Dechesne nicolas.dechesne@linaro.org Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org
arch/arm64/mm/proc.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Any reason why you didn't cc: the stable list, as this is a patch that is not needed in mainline, right?
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S index 08572f95bd8a..2b473ddeb7a3 100644 --- a/arch/arm64/mm/proc.S +++ b/arch/arm64/mm/proc.S @@ -155,7 +155,7 @@ ENDPROC(cpu_do_switch_mm) .macro __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2 adrp \tmp1, empty_zero_page
- msr ttbr1_el1, \tmp2
- msr ttbr1_el1, \tmp1
I don't understand why this isn't also needed in Linus's tree. What commit there prevents this from being required?
thanks,
greg k-h
On Sat, Feb 24, 2018 at 9:34 AM, Greg KH gregkh@linuxfoundation.org wrote:
On Fri, Feb 23, 2018 at 06:29:02PM +0000, Ard Biesheuvel wrote:
Stable backport commit 173358a49173 ("arm64: kpti: Add ->enable callback to remap swapper using nG mappings") of upstream commit f992b4dfd58b did not survive the backporting process unscathed, and ends up writing garbage into the TTBR1_EL1 register, rather than pointing it to the zero page to disable translations. Fix that.
Cc: stable@vger.kernel.org #v4.14 Reported-by: Nicolas Dechesne nicolas.dechesne@linaro.org Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org
arch/arm64/mm/proc.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Any reason why you didn't cc: the stable list, as this is a patch that is not needed in mainline, right?
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S index 08572f95bd8a..2b473ddeb7a3 100644 --- a/arch/arm64/mm/proc.S +++ b/arch/arm64/mm/proc.S @@ -155,7 +155,7 @@ ENDPROC(cpu_do_switch_mm)
.macro __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2 adrp \tmp1, empty_zero_page
msr ttbr1_el1, \tmp2
msr ttbr1_el1, \tmp1I don't understand why this isn't also needed in Linus's tree. What commit there prevents this from being required?
in master this code is
.macro __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2 adrp \tmp1, empty_zero_page phys_to_ttbr \tmp2, \tmp1 msr ttbr1_el1, \tmp2 isb
which can also explain why the (non trivial) cherry-picked commit ended up wrong.
this change in master came from
529c4b05a3cb arm64: handle 52-bit addresses in TTBR
which afaik, is not needed on stable
thanks,
greg k-h
On 24 February 2018 at 08:34, Greg KH gregkh@linuxfoundation.org wrote:
On Fri, Feb 23, 2018 at 06:29:02PM +0000, Ard Biesheuvel wrote:
Stable backport commit 173358a49173 ("arm64: kpti: Add ->enable callback to remap swapper using nG mappings") of upstream commit f992b4dfd58b did not survive the backporting process unscathed, and ends up writing garbage into the TTBR1_EL1 register, rather than pointing it to the zero page to disable translations. Fix that.
Cc: stable@vger.kernel.org #v4.14 Reported-by: Nicolas Dechesne nicolas.dechesne@linaro.org Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org
arch/arm64/mm/proc.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Any reason why you didn't cc: the stable list, as this is a patch that is not needed in mainline, right?
Indeed, apologies. I added the Cc: tag but it appears not to have been picked up by git send-email.
Also, i suppose it is unclear from the tag that this should be applied to both v4.15 and v4.14
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S index 08572f95bd8a..2b473ddeb7a3 100644 --- a/arch/arm64/mm/proc.S +++ b/arch/arm64/mm/proc.S @@ -155,7 +155,7 @@ ENDPROC(cpu_do_switch_mm)
.macro __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2 adrp \tmp1, empty_zero_page
msr ttbr1_el1, \tmp2
msr ttbr1_el1, \tmp1I don't understand why this isn't also needed in Linus's tree. What commit there prevents this from being required?
Linus's tree has
+.macro __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2 + adrp \tmp1, empty_zero_page + phys_to_ttbr \tmp1, \tmp2 + msr ttbr1_el1, \tmp2 + isb + tlbi vmalle1 + dsb nsh + isb +.endm
but phys_to_ttbr does not exist in the v4.15 and earlier trees (it is related to 52-bit physical address support which landed in v4.16), so it was removed for the backport. However, that means tmp2 is never assigned, and whatever was there is poked into the translation table base register.
But let's wait for team-ARM to ack this in any case.
Thanks, Ard.
On Sat, Feb 24, 2018 at 08:50:42AM +0000, Ard Biesheuvel wrote:
On 24 February 2018 at 08:34, Greg KH gregkh@linuxfoundation.org wrote:
On Fri, Feb 23, 2018 at 06:29:02PM +0000, Ard Biesheuvel wrote:
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S index 08572f95bd8a..2b473ddeb7a3 100644 --- a/arch/arm64/mm/proc.S +++ b/arch/arm64/mm/proc.S @@ -155,7 +155,7 @@ ENDPROC(cpu_do_switch_mm)
.macro __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2 adrp \tmp1, empty_zero_page
msr ttbr1_el1, \tmp2
msr ttbr1_el1, \tmp1I don't understand why this isn't also needed in Linus's tree. What commit there prevents this from being required?
Linus's tree has
+.macro __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2
adrp \tmp1, empty_zero_pagephys_to_ttbr \tmp1, \tmp2msr ttbr1_el1, \tmp2isbtlbi vmalle1dsb nshisb+.endm
but phys_to_ttbr does not exist in the v4.15 and earlier trees (it is related to 52-bit physical address support which landed in v4.16), so it was removed for the backport. However, that means tmp2 is never assigned, and whatever was there is poked into the translation table base register.
Damnit, sorry again. I changed the argument order of phys_to_ttbr along the way, so must've confused myself during the backporting exercise. It's also one of those things that will lead to potential TLB corruption in rare circumstances where the junk in TTBR1 ends up giving a valid translation, so it didn't crop up in my testing. How did Nicolas see this? The bug report I saw didn't look related.
But let's wait for team-ARM to ack this in any case.
Acked-by: Will Deacon will.deacon@arm.com
Greg -- please can you apply this to the 4.14 and 4.15 stable trees?
Will
On 26 February 2018 at 11:30, Will Deacon will.deacon@arm.com wrote:
On Sat, Feb 24, 2018 at 08:50:42AM +0000, Ard Biesheuvel wrote:
On 24 February 2018 at 08:34, Greg KH gregkh@linuxfoundation.org wrote:
On Fri, Feb 23, 2018 at 06:29:02PM +0000, Ard Biesheuvel wrote:
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S index 08572f95bd8a..2b473ddeb7a3 100644 --- a/arch/arm64/mm/proc.S +++ b/arch/arm64/mm/proc.S @@ -155,7 +155,7 @@ ENDPROC(cpu_do_switch_mm)
.macro __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2 adrp \tmp1, empty_zero_page
msr ttbr1_el1, \tmp2
msr ttbr1_el1, \tmp1I don't understand why this isn't also needed in Linus's tree. What commit there prevents this from being required?
Linus's tree has
+.macro __idmap_cpu_set_reserved_ttbr1, tmp1, tmp2
adrp \tmp1, empty_zero_pagephys_to_ttbr \tmp1, \tmp2msr ttbr1_el1, \tmp2isbtlbi vmalle1dsb nshisb+.endm
but phys_to_ttbr does not exist in the v4.15 and earlier trees (it is related to 52-bit physical address support which landed in v4.16), so it was removed for the backport. However, that means tmp2 is never assigned, and whatever was there is poked into the translation table base register.
Damnit, sorry again. I changed the argument order of phys_to_ttbr along the way, so must've confused myself during the backporting exercise. It's also one of those things that will lead to potential TLB corruption in rare circumstances where the junk in TTBR1 ends up giving a valid translation, so it didn't crop up in my testing. How did Nicolas see this? The bug report I saw didn't look related.
This broke the boot on the Dragonboard 410c, but the bisect identified another unrelated commit initially.
But let's wait for team-ARM to ack this in any case.
Acked-by: Will Deacon will.deacon@arm.com
Greg -- please can you apply this to the 4.14 and 4.15 stable trees?
Will
On Mon, Feb 26, 2018 at 11:30:50AM +0000, Will Deacon wrote:
Damnit, sorry again. I changed the argument order of phys_to_ttbr along the way, so must've confused myself during the backporting exercise. It's also one of those things that will lead to potential TLB corruption in rare circumstances where the junk in TTBR1 ends up giving a valid translation, so it didn't crop up in my testing. How did Nicolas see this? The bug report I saw didn't look related.
FWIW, we've been hitting this bug with a distribution backport on ThunderX2 on every boot. Due to bad luck there was a non-zero value in TTBR1 that crashed the kernel immediately and dropped us to firmware.
--Jan
linux-stable-mirror@lists.linaro.org
-
Ard Biesheuvel -
Greg KH -
Jan Glauber -
Nicolas Dechesne -
Will Deacon