linux-kernel - Re: [PATCH v2 3/5] arm64: Fix incorrect irqflag restore for priority masking

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: <5e8a85f5-c837-3ce8-5830-f3ae7897e326@arm.com>
Date:   Tue, 14 May 2019 13:01:14 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Julien Thierry <julien.thierry@....com>,
        Marc Zyngier <marc.zyngier@....com>,
        linux-arm-kernel@...ts.infradead.org
Cc:     mark.rutland@....com, Suzuki K Pouloze <suzuki.poulose@....com>,
        catalin.marinas@....com, will.deacon@....com,
        linux-kernel@...r.kernel.org, rostedt@...dmis.org,
        Christoffer Dall <christoffer.dall@....com>,
        james.morse@....com, Oleg Nesterov <oleg@...hat.com>,
        yuzenghui@...wei.com, wanghaibin.wang@...wei.com,
        liwei391@...wei.com
Subject: Re: [PATCH v2 3/5] arm64: Fix incorrect irqflag restore for priority
 masking

On 14/05/2019 10:25, Julien Thierry wrote:
[...]
>>> +static inline int arch_irqs_disabled_flags(unsigned long flags)
>>> +{
>>> +	int res;
>>> +
>>> +	asm volatile(ALTERNATIVE(
>>> +		"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
>>> +		"nop",
>>> +		"cmp	%w1, #" __stringify(GIC_PRIO_IRQON) "\n"
>>> +		"cset	%w0, ne",
>>> +		ARM64_HAS_IRQ_PRIO_MASKING)
>>> +		: "=&r" (res)
>>> +		: "r" ((int) flags)
>>> +		: "memory");
>>
>> I wonder if this should have "cc" as part of the clobber list.
> 
> Is there any special semantic to "cc" on arm64? All I can find is that
> in the general case it indicates that it is modifying the "flags" register.
> 
> Is your suggestion only for the PMR case? Or is it something that we
> should add regardless of PMR?
> The latter makes sense to me, but for the former, I fail to understand
> why this should affect only PMR.

The PMR case really ought to have have a cc clobber, because who knows 
what this may end up inlined into, and compilers can get pretty 
aggressive with instruction scheduling in ways which leave a live value 
in CPSR across sizeable chunks of other code. It's true that the non-PMR 
case doesn't need it, but the surrounding code still needs to be 
generated to accommodate both possible versions of the alternative. From 
the look of the rest of the patch, the existing pseudo-NMI code has this 
bug in a few places.

Technically you could omit it when ARM64_PSEUDO_NMI is configured out 
entirely, but at that point you may as well omit the whole alternative 
as well. It's probably not worth the bother unless it proves to have a 
significant impact on codegen overall. On which note the memory clobber 
also seems superfluous either way :/

That said, now that I've been looking at it for this long, if the aim is 
just to create a zero/nonzero value then couldn't the PMR case just be 
"eor %w0, %w1, #GIC_PRIO_IRQON" and avoid the need for clobbers at all?

Robin.