linux-kernel - Re: [PATCH v7 11/25] 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:   Thu, 13 Dec 2018 12:02:40 +0000
From:   Julien Thierry <julien.thierry@....com>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc:     linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        joel@...lfernandes.org, Marc Zyngier <marc.zyngier@....com>,
        Christoffer Dall <christoffer.dall@....com>,
        James Morse <james.morse@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Mark Rutland <mark.rutland@....com>, oleg@...hat.com
Subject: Re: [PATCH v7 11/25] arm64: irqflags: Use ICC_PMR_EL1 for interrupt
 masking



On 13/12/2018 11:35, Ard Biesheuvel wrote:
> On Thu, 13 Dec 2018 at 09:54, Julien Thierry <julien.thierry@....com> wrote:
>>
>>
>>
>> On 12/12/2018 18:10, Ard Biesheuvel wrote:
>>> On Wed, 12 Dec 2018 at 18:59, Julien Thierry <julien.thierry@....com> wrote:
>>>>
>>>>
>>>>
>>>> On 12/12/2018 17:27, Ard Biesheuvel wrote:
>>>>> On Wed, 12 Dec 2018 at 17:48, Julien Thierry <julien.thierry@....com> 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      |   5 +-
>>>>>>  arch/arm64/include/asm/irqflags.h | 123 +++++++++++++++++++++++++++++---------
>>>>>>  2 files changed, 99 insertions(+), 29 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
>>>>>> index 7ed3208..a9d3ebc 100644
>>>>>> --- a/arch/arm64/include/asm/efi.h
>>>>>> +++ b/arch/arm64/include/asm/efi.h
>>>>>> @@ -42,7 +42,10 @@
>>>>>>
>>>>>>  efi_status_t __efi_rt_asm_wrapper(void *, const char *, ...);
>>>>>>
>>>>>> -#define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
>>>>>> +#define ARCH_EFI_IRQ_FLAGS_MASK                                                \
>>>>>> +       (system_uses_irq_prio_masking() ?                               \
>>>>>> +               GIC_PRIO_IRQON :                                        \
>>>>>> +               (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT))
>>>>>>
>>>>>
>>>>> This mask is used to determine whether we return from a firmware call
>>>>> with a different value for the I flag than we entered it with. So
>>>>> instead of changing the mask, we should change the way we record DAIF,
>>>>> given that the firmware is still going to poke the I bit if it
>>>>> misbehaves, regardless of whether the OS happens to use priorities for
>>>>> interrupt masking.
>>>>>
>>>>
>>>> Thanks for pointing that out, so this change makes little sense...
>>>>
>>>> The annoying part is that the flag checking takes place in the arch
>>>> agnostic code.
>>>>
>>>> Would introducing some overriddable efi_get_flags() or efi_save_flags()
>>>> that default to local_save_flags() seem like an acceptable solution?
>>>>
>>>> This way I could override it for arm64 and still return the DAIF bits.
>>>>
>>>
>>> I don't follow the reasoning below about irqflags exactly, but is
>>> there any way we could simply but both PMR and DAIF in flags? We could
>>> even update the mask here to ensure that the firmware doesn't corrupt
>>> the PMR.
>>>
>>
>> So, that was the case in my previous versions of the series, and as you
>> said, that covered checking both DAIF bits and PMR on return from EFI
>> services. But Catalin suggested that irqflags could just use PMR when we
>> enable the priority masking feature. Catalin's suggestion does simplify
>> things, except for this part.
>>
>> However, it doesn't seem to far-fetched to me that the architecture
>> could have a more generic way to tell the EFI driver "this is the set of
>> stuff that I care about and you should return from runtime services with
>> this stuff in the same state as before" without the "set of stuff" being
>> limited to irqflags.
>>
>> But maybe this would be over-engineering just to deal with my use-case...
>>
> 
> No, that makes sense. As you said, you can just create a
> efi_get_irqflags() helper that defaults to what we are using now, and
> can be overridden to just return DAIF in our case.
> 

Good, thanks for the confirmation. I'll do that for the next version of
the series.

Thanks,

-- 
Julien Thierry