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]

Date:   Tue, 14 May 2019 14:08:55 +0100
From:   Julien Thierry <julien.thierry@....com>
To:     Robin Murphy <robin.murphy@....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 13:01, Robin Murphy wrote:
> 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 :/
> 

Right, I see. I misunderstood what was meant by "cc" indicating that the
assembly modified the flags.

Due to the context I interpreted it as irqflags whereas it concerns the
condition flags (hence the 'c' I presume...). It all makes more sense now.

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

Yes, definitely seems like it would be better! I'll take that
suggestion, thanks.

Cheers,

-- 
Julien Thierry