From: Hans de Goede <hdegoede@redhat.com>
To: "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc: Andy Lutomirski <luto@kernel.org>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Greg KH <gregkh@linuxfoundation.org>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
Peter Jones <pjones@redhat.com>,
dave@bewaar.me, Will Deacon <will.deacon@arm.com>,
Matt Fleming <matt@codeblueprint.co.uk>,
David Howells <dhowells@redhat.com>,
Mimi Zohar <zohar@linux.vnet.ibm.com>,
Josh Triplett <josh@joshtriplett.org>,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
Martin Fuzzey <mfuzzey@parkeon.com>,
Kalle Valo <kvalo@codeaurora.org>,
Arend Van Spriel <arend.vanspriel@broadcom.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
nbroeking@me.com, Bjorn Andersson <bjorn.andersson@linaro.org>,
duwe@suse.de, Kees Cook <keescook@chromium.org>,
X86 ML <x86@kernel.org>, linux-efi <linux-efi@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
LSM List <linux-security-module@vger.kernel.org>
Subject: Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
Date: Sun, 13 May 2018 12:05:39 +0100 [thread overview]
Message-ID: <306d98d9-b489-b98b-a0b3-27539aca167a@redhat.com> (raw)
In-Reply-To: <20180503223126.GE27853@wotan.suse.de>
Hi,
On 05/03/2018 11:31 PM, Luis R. Rodriguez wrote:
> On Wed, May 02, 2018 at 04:49:53PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 05/01/2018 09:29 PM, Andy Lutomirski wrote:
>>> On Sun, Apr 29, 2018 at 2:36 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> +The EFI embedded-fw code works by scanning all EFI_BOOT_SERVICES_CODE
>>> memory
>>>> +segments for an eight byte sequence matching prefix, if the prefix is
>>> found it
>>>> +then does a crc32 over length bytes and if that matches makes a copy of
>>> length
>>>> +bytes and adds that to its list with found firmwares.
>>>> +
>>>
>>> Eww, gross. Is there really no better way to do this?
>>
>> I'm afraid not.
>>
>>> Is the issue that
>>> the EFI code does not intend to pass the firmware to the OS but that it has
>>> a copy for its own purposes and that Linux is just going to hijack EFI's
>>> copy? If so, that's brilliant and terrible at the same time.
>>
>> Yes that is exactly the issue / what it happening here.
>>
>>>
>>>> + for (i = 0; i < size; i += 8) {
>>>> + if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
>>>> + continue;
>>>> +
>>>> + /* Seed with ~0, invert to match crc32 userspace utility
>>> */
>>>> + crc = ~crc32(~0, mem + i, desc->length);
>>>> + if (crc == desc->crc)
>>>> + break;
>>>> + }
>>>
>>> I hate to play the security card, but this stinks a bit. The kernel
>>> obviously needs to trust the EFI boot services code since the EFI boot
>>> services code is free to modify the kernel image. But your patch is not
>>> actually getting this firmware blob from the boot services code via any
>>> defined interface -- you're literally snarfing up the blob from a range of
>>> memory. I fully expect there to be any number of ways for untrustworthy
>>> entities to inject malicious blobs into this memory range on quite a few
>>> implementations. For example, there are probably unauthenticated EFI
>>> variables and even parts of USB sticks and such that get read into boot
>>> services memory, and I see no reason at all to expect that nothing in the
>>> so-called "boot services code" range is actually just plain old boot
>>> services *heap*.
>>>
>>> Fortunately, given your design, this is very easy to fix. Just replace
>>> CRC32 with SHA-256 or similar. If you find the crypto api too ugly for
>>> this purpose, I have patches that only need a small amount of dusting off
>>> to give an entirely reasonable SHA-256 API in the kernel.
>>
>> My main reason for going with crc32 is that the scanning happens before
>> the kernel is fully up and running (it happens just before the rest_init()
>> call in start_kernel() (from init/main.c) I'm open to using the
>> crypto api, but I was not sure if that is ready for use at that time.
>
> Not being sure is different than being certain. As Andy noted, if that does
> not work please poke Andy about the SHA-256 API he has which would enable
> its use in kernel.
>
> Right now this is just a crazy hack for *2* drivers. Its a lot of hacks for
> just that, so no need to rush this in just yet.
I agree that there is no rush to get this in. I will rebase this on top
of the "[PATCH v7 00/14] firmware_loader changes for v4.18" series you recently
send as well as try to address all the remarks made sofar. I'm not entirely
sure when I will get around to this.
Regards,
Hans
next prev parent reply other threads:[~2018-05-13 11:05 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-29 9:35 [PATCH v5 0/5] efi/firmware/platform-x86: Add EFI embedded fw support Hans de Goede
2018-04-29 9:35 ` [PATCH v5 1/5] efi: Export boot-services code and data as debugfs-blobs Hans de Goede
2018-04-29 9:35 ` [PATCH v5 2/5] efi: Add embedded peripheral firmware support Hans de Goede
2018-05-01 14:36 ` Mimi Zohar
2018-05-01 19:11 ` Hans de Goede
2018-05-01 19:27 ` Mimi Zohar
2018-05-03 22:23 ` Luis R. Rodriguez
2018-05-03 23:02 ` Mimi Zohar
2018-05-01 19:29 ` Andy Lutomirski
2018-05-01 20:06 ` Lukas Wunner
2018-05-02 14:49 ` Hans de Goede
2018-05-03 22:31 ` Luis R. Rodriguez
2018-05-03 22:35 ` Andy Lutomirski
2018-05-13 11:41 ` Hans de Goede
2018-05-13 11:05 ` Hans de Goede [this message]
2018-05-03 23:29 ` Luis R. Rodriguez
2018-05-04 5:54 ` Ard Biesheuvel
2018-05-08 17:12 ` Luis R. Rodriguez
2018-05-13 14:10 ` Hans de Goede
2018-05-04 5:56 ` Ard Biesheuvel
2018-05-13 11:03 ` Hans de Goede
2018-05-13 11:43 ` Ard Biesheuvel
2018-05-13 13:26 ` Hans de Goede
2018-04-29 9:35 ` [PATCH v5 3/5] platform/x86: Rename silead_dmi to touchscreen_dmi Hans de Goede
2018-04-29 9:35 ` [PATCH v5 4/5] platform/x86: touchscreen_dmi: Add EFI embedded firmware info support Hans de Goede
2018-04-29 9:35 ` [PATCH v5 5/5] platform/x86: touchscreen_dmi: Add info for the Chuwi Vi8 Plus tablet Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=306d98d9-b489-b98b-a0b3-27539aca167a@redhat.com \
--to=hdegoede@redhat.com \
--cc=ard.biesheuvel@linaro.org \
--cc=arend.vanspriel@broadcom.com \
--cc=bjorn.andersson@linaro.org \
--cc=dave@bewaar.me \
--cc=dhowells@redhat.com \
--cc=dmitry.torokhov@gmail.com \
--cc=duwe@suse.de \
--cc=gregkh@linuxfoundation.org \
--cc=hpa@zytor.com \
--cc=josh@joshtriplett.org \
--cc=keescook@chromium.org \
--cc=kvalo@codeaurora.org \
--cc=linux-efi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=luto@kernel.org \
--cc=matt@codeblueprint.co.uk \
--cc=mcgrof@kernel.org \
--cc=mfuzzey@parkeon.com \
--cc=mingo@redhat.com \
--cc=nbroeking@me.com \
--cc=pjones@redhat.com \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=will.deacon@arm.com \
--cc=x86@kernel.org \
--cc=zohar@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).