[PATCH -stable] arm64: mm: don't write garbage into TTBR1_EL1 register

[PATCH -stable] arm64: mm: don't write garbage into TTBR1_EL1 register

[Ard Biesheuvel]

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(-)

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
 	isb
 	tlbi	vmalle1
 	dsb	nsh
-- 
2.11.0

[PATCH -stable] arm64: mm: don't write garbage into TTBR1_EL1 register

[Nicolas Dechesne]

hi Ard,

many thanks for your help and the debug session ;-)

On Fri, Feb 23, 2018 at 7:29 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> 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>

I have tested this patch on Qualcomm Dragonboard 410c where the issue
was found initially.

Tested-by: Nicolas Dechesne <nicolas.dechesne@linaro.org>

This patch is also needed on 4.15-stable.

> ---
>  arch/arm64/mm/proc.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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
>         isb
>         tlbi    vmalle1
>         dsb     nsh
> --
> 2.11.0
>

[PATCH -stable] arm64: mm: don't write garbage into TTBR1_EL1 register

[Greg KH]

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

[PATCH -stable] arm64: mm: don't write garbage into TTBR1_EL1 register

[Nicolas Dechesne]

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, \tmp1
>
> I 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

[PATCH -stable] arm64: mm: don't write garbage into TTBR1_EL1 register

[Ard Biesheuvel]

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, \tmp1
>
> I 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.

[PATCH -stable] arm64: mm: don't write garbage into TTBR1_EL1 register

[Will Deacon]

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, \tmp1
> >
> > I 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.

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

[PATCH -stable] arm64: mm: don't write garbage into TTBR1_EL1 register

[Ard Biesheuvel]

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, \tmp1
>> >
>> > I 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.
>
> 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

[PATCH -stable] arm64: mm: don't write garbage into TTBR1_EL1 register

[Jan Glauber]

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