linux-kernel - Re: [tip:efi/core] efibc: Add EFI Bootloader Control module

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: <87lh3wejfi.fsf@jcompost-MOBL1.tl.intel.com>
Date:	Fri, 29 Apr 2016 13:36:01 +0200
From:	jeremy.compostella@...el.com (Compostella, Jeremy)
To:	Matt Fleming <matt@...eblueprint.co.uk>
Cc:	Ingo Molnar <mingo@...nel.org>, <stefan.stanacar@...el.com>,
	<peterz@...radead.org>, <linux-kernel@...r.kernel.org>,
	<tglx@...utronix.de>, <hpa@...or.com>, <bp@...en8.de>,
	<ard.biesheuvel@...aro.org>, <linux-tip-commits@...r.kernel.org>
Subject: Re: [tip:efi/core] efibc: Add EFI Bootloader Control module

Matt Fleming <matt@...eblueprint.co.uk> writes:

> On Fri, 29 Apr, at 11:53:56AM, Ingo Molnar wrote:
>> 
>> * tip-bot for Compostella, Jeremy <tipbot@...or.com> wrote:
>> 
>> > Commit-ID:  06f7d4a1618dbb086e738c93cd1ef416ab01027d
>> > Gitweb:     http://git.kernel.org/tip/06f7d4a1618dbb086e738c93cd1ef416ab01027d
>> > Author:     Compostella, Jeremy <jeremy.compostella@...el.com>
>> > AuthorDate: Mon, 25 Apr 2016 21:06:57 +0100
>> > Committer:  Ingo Molnar <mingo@...nel.org>
>> > CommitDate: Thu, 28 Apr 2016 11:34:02 +0200
>> > 
>> > efibc: Add EFI Bootloader Control module
>> > 
>> > This module installs a reboot callback, such that if reboot() is invoked
>> > with a string argument NNN, "NNN" is copied to the "LoaderEntryOneShot"
>> > EFI variable, to be read by the bootloader.
>> 
>> >  drivers/firmware/efi/Kconfig  |  15 +++++++
>> >  drivers/firmware/efi/Makefile |   1 +
>> >  drivers/firmware/efi/efibc.c  | 101 ++++++++++++++++++++++++++++++++++++++++++
>> >  include/linux/efi.h           |   4 ++
>> >  4 files changed, 121 insertions(+)
>> 
>> So this bloated things a bit on 32-bit x86 allyesconfig kernels, we now have this 
>> new warning:
>> 
>>   drivers/firmware/efi/efibc.c:53:1: warning: the frame size of 2256
>> bytes is larger than 1024 bytes [-Wframe-larger-than=]
>> 
>> 2K of stack use for a function is quite excessive, can we improve the stack 
>> footprint of this code?
>
> I'm waiting to hear from Jeremy on whether we can simply move the
> struct efivar_entry (which is the cause of the stack bloat) off the
> stack and into the .bss, because it only gets used from the reboot
> notifier call chain.
>
> But upon reading kernel_restart() I'm no longer sure it's guaranteed
> to be called only once, or even non-concurrently. It seems that if the
> user executes the reboot command and either the sysrq reboot code is
> invoked, or an error is encountered dm-verify-target driver or any
> other kernel_restart() caller is invoked we could race.
>
> Perhaps we should guard efi_reboot_notifier_call() with an atomic_t
> and exit if we've already invoked it?
>
> Alternatively, we could just kmalloc() the object ;)
I would go for this last proposal because it fixes the issue, it works
with the potential race condition you mentioned and it is a simple
solution.  Yet, it means that if we really run into a race condition,
the LoaderEntryOneShot value might be indeterminate.

Though, this is a very unlikely situation and if it happens I cannot
think of any good reason to prefer to store the first restart target
or the second.

Do you want me to make this change ?

Jérémy
-- 
One Emacs to rule them all