linux-kernel - Re: [PATCH] arm64/mm: Re-organise setting up FEAT_S1PIE registers PIRE0_EL1 and PIR

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: <5d975762-7678-419f-8e2f-40547c079276@arm.com>
Date: Mon, 14 Apr 2025 13:31:53 +0100
From: Ryan Roberts <ryan.roberts@....com>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: Anshuman Khandual <anshuman.khandual@....com>,
 linux-arm-kernel@...ts.infradead.org,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Mark Rutland <mark.rutland@....com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64/mm: Re-organise setting up FEAT_S1PIE registers
 PIRE0_EL1 and PIR_EL1

On 14/04/2025 13:28, Ard Biesheuvel wrote:
> On Mon, 14 Apr 2025 at 14:04, Ryan Roberts <ryan.roberts@....com> wrote:
>>
>> On 14/04/2025 10:41, Ard Biesheuvel wrote:
>>> On Mon, 14 Apr 2025 at 09:52, Ryan Roberts <ryan.roberts@....com> wrote:
>>>>
>>>> On 10/04/2025 08:40, Anshuman Khandual wrote:
>>>>> mov_q cannot really move PIE_E[0|1] macros into a general purpose register
>>>>> as expected if those macro constants contain some 128 bit layout elements,
>>>>> required for D128 page tables. Fix this problem via first loading up these
>>>>> macro constants into a given memory location and then subsequently setting
>>>>> up registers PIRE0_EL1 and PIR_EL1 by retrieving the memory stored values.
>>>>
>>>> From memory, the primary issue is that for D128, PIE_E[0|1] are defined in terms
>>>> of 128-bit types with shifting and masking, which the assembler can't do? It
>>>> would be good to spell this out.
>>>>
>>>>>
>>>>> Cc: Catalin Marinas <catalin.marinas@....com>
>>>>> Cc: Will Deacon <will@...nel.org>
>>>>> Cc: Mark Rutland <mark.rutland@....com>
>>>>> Cc: Ard Biesheuvel <ardb@...nel.org>
>>>>> Cc: Ryan Roberts <ryan.roberts@....com>
>>>>> Cc: linux-arm-kernel@...ts.infradead.org
>>>>> Cc: linux-kernel@...r.kernel.org
>>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
>>>>> ---
>>>>> This patch applies on v6.15-rc1
>>>>>
>>>>>  arch/arm64/kernel/head.S         | 3 +++
>>>>>  arch/arm64/kernel/pi/map_range.c | 6 ++++++
>>>>>  arch/arm64/kernel/pi/pi.h        | 1 +
>>>>>  arch/arm64/mm/mmu.c              | 1 +
>>>>>  arch/arm64/mm/proc.S             | 5 +++--
>>>>>  5 files changed, 14 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>>>>> index 2ce73525de2c..4950d9cc638a 100644
>>>>> --- a/arch/arm64/kernel/head.S
>>>>> +++ b/arch/arm64/kernel/head.S
>>>>> @@ -126,6 +126,9 @@ SYM_CODE_START(primary_entry)
>>>>>        * On return, the CPU will be ready for the MMU to be turned on and
>>>>>        * the TCR will have been set.
>>>>>        */
>>>>> +     adr_l   x0, pir_data
>>>>> +     bl      __pi_load_pir_data
>>>>
>>>> Using C code to pre-calculate the values into global variables that the assembly
>>>> code then loads and stuffs into the PIR registers feels hacky. I wonder if we
>>>> can instead pre-calculate into asm-offsets.h? e.g. add the following to
>>>> asm-offsets.c:
>>>>
>>>> DEFINE(PIE_E0_ASM, PIE_E0);
>>>> DEFINE(PIE_E1_ASM, PIE_E1);
>>>>
>>>> Which will generate the asm-offsets.h header with PIE_E[0|1]_ASM with the
>>>> pre-calculated values that you can then use in proc.S?
>>>>
>>>
>>> There is another issue, which is that mov_q tries to be smart, and
>>> emit fewer than 4 MOVZ/MOVK instructions if possible. So the .if
>>> directive evaluates the argument, which does not work with symbolic
>>> constants.
>>
>> I'm not quite understanding the detail here; what do you mean by "symbolic
>> constants"? asm-offsets.h will provide something like:
>>
>> #define PIE_E0_ASM 1234567890
>>
>> The current code is using a hash-define and that's working fine:
>>
>> mov_q   x0, PIE_E0
>>
>>
>> Won't the C preprocessor just substitute and everything will work out?
>>
> 
> Yeah, you're right. I was experimenting with something like
> 
> .set .Lpie_e0, PIE_E0_ASM
> mov_q xN, .Lpie_e0
> 
> where this problem does exist, but we can just use PIE_E0_ASM directly
> and things should work as expected.

Ahh great, sounds like this should be pretty simple then!