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

Re: [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

Re: [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

Re: [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.

Re: [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

Re: [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

Re: [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

Patch "arm64: mm: don't write garbage into TTBR1_EL1 register" has been added to the 4.14-stable tree

[gregkh]

This is a note to let you know that I've just added the patch titled

    arm64: mm: don't write garbage into TTBR1_EL1 register

to the 4.14-stable tree which can be found at:
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
     arm64-mm-don-t-write-garbage-into-ttbr1_el1-register.patch
and it can be found in the queue-4.14 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.


>From ard.biesheuvel@linaro.org  Mon Feb 26 13:53:22 2018
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date: Fri, 23 Feb 2018 18:29:02 +0000
Subject: arm64: mm: don't write garbage into TTBR1_EL1 register
To: linux-arm-kernel@lists.infradead.org
Cc: catalin.marinas@arm.com, will.deacon@arm.com, marc.zyngier@arm.com, mark.rutland@arm.com, nicolas.dechesne@linaro.org, gregkh@linuxfoundation.org, Ard Biesheuvel <ard.biesheuvel@linaro.org>
Message-ID: <20180223182902.24873-1-ard.biesheuvel@linaro.org>

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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>
Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 arch/arm64/mm/proc.S |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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


Patches currently in stable-queue which might be from ard.biesheuvel@linaro.org are

queue-4.14/arm64-mm-don-t-write-garbage-into-ttbr1_el1-register.patch

Patch "arm64: mm: don't write garbage into TTBR1_EL1 register" has been added to the 4.15-stable tree

[gregkh]

This is a note to let you know that I've just added the patch titled

    arm64: mm: don't write garbage into TTBR1_EL1 register

to the 4.15-stable tree which can be found at:
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
     arm64-mm-don-t-write-garbage-into-ttbr1_el1-register.patch
and it can be found in the queue-4.15 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.


>From ard.biesheuvel@linaro.org  Mon Feb 26 13:53:22 2018
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date: Fri, 23 Feb 2018 18:29:02 +0000
Subject: arm64: mm: don't write garbage into TTBR1_EL1 register
To: linux-arm-kernel@lists.infradead.org
Cc: catalin.marinas@arm.com, will.deacon@arm.com, marc.zyngier@arm.com, mark.rutland@arm.com, nicolas.dechesne@linaro.org, gregkh@linuxfoundation.org, Ard Biesheuvel <ard.biesheuvel@linaro.org>
Message-ID: <20180223182902.24873-1-ard.biesheuvel@linaro.org>

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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>
Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 arch/arm64/mm/proc.S |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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


Patches currently in stable-queue which might be from ard.biesheuvel@linaro.org are

queue-4.15/arm64-mm-don-t-write-garbage-into-ttbr1_el1-register.patch

Patch "arm64: mm: don't write garbage into TTBR1_EL1 register" has been added to the 4.4-stable tree

[gregkh]

This is a note to let you know that I've just added the patch titled

    arm64: mm: don't write garbage into TTBR1_EL1 register

to the 4.4-stable tree which can be found at:
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
     arm64-mm-don-t-write-garbage-into-ttbr1_el1-register.patch
and it can be found in the queue-4.4 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.


>From ard.biesheuvel@linaro.org  Mon Feb 26 13:53:22 2018
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date: Fri, 23 Feb 2018 18:29:02 +0000
Subject: arm64: mm: don't write garbage into TTBR1_EL1 register
To: linux-arm-kernel@lists.infradead.org
Cc: catalin.marinas@arm.com, will.deacon@arm.com, marc.zyngier@arm.com, mark.rutland@arm.com, nicolas.dechesne@linaro.org, gregkh@linuxfoundation.org, Ard Biesheuvel <ard.biesheuvel@linaro.org>
Message-ID: <20180223182902.24873-1-ard.biesheuvel@linaro.org>

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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>
Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.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



Patches currently in stable-queue which might be from ard.biesheuvel@linaro.org are

queue-4.4/arm64-mm-don-t-write-garbage-into-ttbr1_el1-register.patch

Re: Patch "arm64: mm: don't write garbage into TTBR1_EL1 register" has been added to the 4.4-stable tree

[Ard Biesheuvel]

On 26 February 2018 at 13:02,  <gregkh@linuxfoundation.org> wrote:
>
> This is a note to let you know that I've just added the patch titled
>
>     arm64: mm: don't write garbage into TTBR1_EL1 register
>
> to the 4.4-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>

Please drop this. It only applies to v4.14 and v4.15

Re: Patch "arm64: mm: don't write garbage into TTBR1_EL1 register" has been added to the 4.4-stable tree

[Greg Kroah-Hartman]

On Mon, Feb 26, 2018 at 01:09:36PM +0000, Ard Biesheuvel wrote:
> On 26 February 2018 at 13:02,  <gregkh@linuxfoundation.org> wrote:
> >
> > This is a note to let you know that I've just added the patch titled
> >
> >     arm64: mm: don't write garbage into TTBR1_EL1 register
> >
> > to the 4.4-stable tree which can be found at:
> >     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> >
> 
> Please drop this. It only applies to v4.14 and v4.15

Ugh, my fault, it didn't even apply, but ended up in the wrong
directory.  Thanks for pointing it out, now dropped.

greg k-h