linux-kernel - Re: [PATCH] ARM: decompressor: reset ttbcr fields to use TTBR0 on ARMv7

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3441d0c3-c4b6-a795-a841-b61a8866ce17@arm.com>
Date:   Tue, 27 Sep 2016 13:54:35 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Srinivas Ramana <sramana@...eaurora.org>
Cc:     nicolas.pitre@...aro.org, linux-arm-msm@...r.kernel.org,
        will.deacon@....com, linux@...linux.org.uk,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] ARM: decompressor: reset ttbcr fields to use TTBR0 on
 ARMv7

On 27/09/16 13:16, Srinivas Ramana wrote:
> Hi Robin,

Sorry! This one had slipped my mind already...

> On 09/13/2016 08:22 PM, Srinivas Ramana wrote:
>> On 09/12/2016 11:21 PM, Robin Murphy wrote:
>>> On 12/09/16 07:57, Srinivas Ramana wrote:
>>>> If the bootloader uses the long descriptor format and jumps to
>>>> kernel decompressor code, TTBCR may not be in a right state.
>>>> Before enabling the MMU, it is required to clear the TTBCR.PD0
>>>> field to use TTBR0 for translation table walks.
>>>>
>>>> The 'commit dbece45894d3a ("ARM: 7501/1: decompressor:
>>>> reset ttbcr for VMSA ARMv7 cores")' does the reset of TTBCR.N, but
>>>> doesn't consider all the bits for the size of TTBCR.N.
>>>>
>>>> Clear TTBCR.PD0 field and reset all the three bits of TTBCR.N to
>>>> indicate the use of TTBR0 and the correct base address width.
>>>>
>>>> Signed-off-by: Srinivas Ramana <sramana@...eaurora.org>
>>>> ---
>>>>   arch/arm/boot/compressed/head.S | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/boot/compressed/head.S
>>>> b/arch/arm/boot/compressed/head.S
>>>> index af11c2f8f3b7..fc6d541549a2 100644
>>>> --- a/arch/arm/boot/compressed/head.S
>>>> +++ b/arch/arm/boot/compressed/head.S
>>>> @@ -779,7 +779,7 @@ __armv7_mmu_cache_on:
>>>>           orrne    r0, r0, #1        @ MMU enabled
>>>>           movne    r1, #0xfffffffd        @ domain 0 = client
>>>>           bic     r6, r6, #1 << 31        @ 32-bit translation system
>>>
>>> Hmm, if TTBCR.EAE _was_ actually set...
>>>
>>>> -        bic     r6, r6, #3 << 0         @ use only ttbr0
>>>> +        bic     r6, r6, #(7 << 0) | (1 << 4)    @ use only ttbr0
>>>>           mcrne    p15, 0, r3, c2, c0, 0    @ load page table pointer
>>>>           mcrne    p15, 0, r1, c3, c0, 0    @ load domain access
>>>> control
>>>>           mcrne   p15, 0, r6, c2, c0, 2   @ load ttb control
>>>
>>> ...then strictly the TLBIALL needs to happen after the ISB following
>>> this update. Otherwise per B3.10.2 of DDI406C.c I think we might be into
>>> unpredictable territory - i.e. if the TLB happens to treat long- and
>>> short-descriptor entries differently then the TLBI beforehand (with EAE
>>> set) may be at liberty to only discard long-descriptor entries and leave
>>> bogus short-descriptor entries sitting around.
>> Yes, it seems this has to be taken care of, along with resetting
>> TTBCR.PD0 and TTBCR.N. Do you say that this needs to be done in the same
>> patch or a different one?
>>>
>>> In other words, something like (completely untested):
>>>
>>> ---8<---
>>> diff --git a/arch/arm/boot/compressed/head.S
>>> b/arch/arm/boot/compressed/head.S
>>> index af11c2f8f3b7..536b7781024a 100644
>>> --- a/arch/arm/boot/compressed/head.S
>>> +++ b/arch/arm/boot/compressed/head.S
>>> @@ -764,7 +764,6 @@ __armv7_mmu_cache_on:
>>>                  mov     r0, #0
>>>                  mcr     p15, 0, r0, c7, c10, 4  @ drain write buffer
>>>                  tst     r11, #0xf               @ VMSA
>>> -               mcrne   p15, 0, r0, c8, c7, 0   @ flush I,D TLBs
>>
>> Shouldn't this be still there for the same reason you explained above? I
>> mean to discard the long descriptor entries when EAE was 1 (before we
>> reset it).
>>>   #endif
>>>                  mrc     p15, 0, r0, c1, c0, 0   @ read control reg
>>>                  bic     r0, r0, #1 << 28        @ clear SCTLR.TRE
>>> @@ -783,8 +782,11 @@ __armv7_mmu_cache_on:
>>>                  mcrne   p15, 0, r3, c2, c0, 0   @ load page table
>>> pointer
>>>                  mcrne   p15, 0, r1, c3, c0, 0   @ load domain access
>>> control
>>>                  mcrne   p15, 0, r6, c2, c0, 2   @ load ttb control
>>> -#endif
>>>                  mcr     p15, 0, r0, c7, c5, 4   @ ISB
>>> +               mcrne   p15, 0, r0, c8, c7, 0   @ flush I,D TLBs
>>> +#else
>>> +               mcr     p15, 0, r0, c7, c5, 4   @ ISB
>>> +#endif
>>>                  mcr     p15, 0, r0, c1, c0, 0   @ load control register
>>>                  mrc     p15, 0, r0, c1, c0, 0   @ and read it back
>>> ---8<---
>>>
>>> Robin.
>>>
>> i have tested this change (flush I, D, TLBs after TTB control is
>> written) and don't see any issue. But on my setup decompression is
>> successful even without this (probably not hitting the case in
>> discussion).
>>
>>
>> Thanks,
>> -- Srinivas R
>>
> 
> Would like your feedback on the above. Can we get the TTBCR fix merged
> first?(will send final patch with Russell Kings comments fixed)
> 
> For testing the TLB flush change we may have to check if we can create a
> failure case.

Yeah, the TLBI being in the wrong place is a separate, pre-existing
problem; as far as this patch goes, it does what it claims to do, and
matches what the ARMv7 (and ARMv6) docs say, so:

Acked-by: Robin Murphy <robin.murphy@....com>

> 
> Thanks,
> -- Srinivas R
>