linux-kernel - Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKv+Gu-AWTQNG3ObSZt0cHV1YFb8UbYpkq-uVBqMW0wryfv_ig@mail.gmail.com>
Date:	Wed, 3 Feb 2016 10:57:24 +0100
From:	Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Matt Fleming <matt@...eblueprint.co.uk>,
	"H . Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andy Lutomirski <luto@...nel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services
 with interrupts enabled

On 3 February 2016 at 10:43, Ingo Molnar <mingo@...nel.org> wrote:
>
> * Matt Fleming <matt@...eblueprint.co.uk> wrote:
>
>> From: Ard Biesheuvel <ard.biesheuvel@...aro.org>
>>
>> The UEFI spec allows Runtime Services to be invoked with interrupts
>> enabled. The only reason we were disabling interrupts was to prevent
>> recursive calls into the services on the same CPU, which will lead to
>> deadlock. However, the only context where such invocations may occur
>> legally is from efi-pstore via efivars, and that code has been updated
>> to call a non-blocking alternative when invoked from a non-interruptible
>> context.
>>
>> So instead, update the ordinary, blocking UEFI Runtime Services wrappers
>> to execute with interrupts enabled. This aims to prevent excessive interrupt
>> latencies on uniprocessor platforms with slow variable stores.
>
> Well, those excessive latencies would affect SMP platforms as well, just that
> there are (usually) other CPUs free to do execution, right?
>

Correct.

> More fundamentally, this makes me nervous:
>
>  > The UEFI spec allows Runtime Services to be invoked with interrupts enabled.
>  > [...]
>
> So what really matters is not what the spec says, but how Windows executes UEFI
> firmware code in practice.
>
> If major versions of Windows calls UEFI firmware with interrupts disabled, then
> frankly I don't think we should interrupt them under Linux either, regardless of
> what the spec says ...
>
> Random firmware code getting interrupted by the OS changes timings and might have
> other side effects the firmware code might not expect - so the question is, does
> Windows already de facto allow the IRQ preemption of firmware calls?
>

Good question. I will try to find out.

> Also, this:
>
>> -     unsigned long flags;
>>       efi_status_t status;
>>
>> -     spin_lock_irqsave(&efi_runtime_lock, flags);
>> +     BUG_ON(in_irq());
>> +
>> +     spin_lock(&efi_runtime_lock);
>
> ... how does crashing the kernel help debuggability?
>
> Please use WARN_ON_ONCE() - or in fact, this assert is probably not needed at all,
> as lockdep will warn about IRQ unsafe lock usage.
>

Actually, reading back the original thread, Matt had already
identified this problem, and v2/v3 of this patch removed all of them
but one, so thanks for spotting that.

> I'd add comments to the efi_runtime_lock definition site explaining that this is
> never taken from IRQ contexts.
>

OK.