linux-kernel - Re: [PATCH v8 12/26] arm64: irqflags: Use ICC_PMR

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]

Date:   Tue, 8 Jan 2019 15:51:18 +0000
From:   Marc Zyngier <marc.zyngier@....com>
To:     Dave Martin <Dave.Martin@....com>,
        Julien Thierry <julien.thierry@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, mark.rutland@....com,
        daniel.thompson@...aro.org, catalin.marinas@....com,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        will.deacon@....com, linux-kernel@...r.kernel.org,
        christoffer.dall@....com, james.morse@....com,
        Oleg Nesterov <oleg@...hat.com>, joel@...lfernandes.org
Subject: Re: [PATCH v8 12/26] arm64: irqflags: Use ICC_PMR_EL1 for interrupt
 masking

On 08/01/2019 15:40, Dave Martin wrote:
> On Tue, Jan 08, 2019 at 02:07:30PM +0000, Julien Thierry wrote:
>> Instead disabling interrupts by setting the PSR.I bit, use a priority
>> higher than the one used for interrupts to mask them via PMR.
>>
>> When using PMR to disable interrupts, the value of PMR will be used
>> instead of PSR.[DAIF] for the irqflags.
>>
>> Signed-off-by: Julien Thierry <julien.thierry@....com>
>> Suggested-by: Daniel Thompson <daniel.thompson@...aro.org>
>> Cc: Catalin Marinas <catalin.marinas@....com>
>> Cc: Will Deacon <will.deacon@....com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@...aro.org>
>> Cc: Oleg Nesterov <oleg@...hat.com>
>> ---
>>  arch/arm64/include/asm/efi.h      |  11 ++++
>>  arch/arm64/include/asm/irqflags.h | 123 +++++++++++++++++++++++++++++---------
>>  2 files changed, 106 insertions(+), 28 deletions(-)
> 
> [...]
> 
>> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
>> index 24692ed..fa3b06f 100644
>> --- a/arch/arm64/include/asm/irqflags.h
>> +++ b/arch/arm64/include/asm/irqflags.h
>> @@ -18,7 +18,9 @@
> 
> [...]
> 
>>  static inline void arch_local_irq_enable(void)
>>  {
>> -	asm volatile(
>> -		"msr	daifclr, #2		// arch_local_irq_enable"
>> -		:
>> +	unsigned long unmasked = GIC_PRIO_IRQON;
>> +
>> +	asm volatile(ALTERNATIVE(
>> +		"msr	daifclr, #2		// arch_local_irq_enable\n"
>> +		"nop",
>> +		"msr_s  " __stringify(SYS_ICC_PMR_EL1) ",%0\n"
>> +		"dsb	sy",
> 
> I'm still not convinced these dsbs are needed.
> 
> Without the dsb, we are probably not guaranteed to take a pending
> interrupt _immediately_ on unmasking, but I'm not sure that's a
> problem.
> 
> What goes wrong if we omit them?

Then the GIC doesn't know it can now deliver interrupts of a lower
priority. Only a dsb can guarantee that the GIC's view of PMR will get
updated.

See 9.1.6 (Observability of the effects of accesses to the GIC
registers), which states:

<quote>
Architectural execution of a DSB instruction guarantees that
— The last value written to ICC_PMR_EL1 or GICC_PMR is observed by the
associated Redistributor.
</quote>

So yes, DSB is required.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...