Linux-EFI Archive on lore.kernel.org
 help / color / Atom feed
From: Andy Lutomirski <luto@kernel.org>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Ard Biesheuvel <ardb@kernel.org>,
	Darren Hart <dvhart@infradead.org>,
	Andy Shevchenko <andy@infradead.org>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H . Peter Anvin" <hpa@zytor.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Peter Jones <pjones@redhat.com>, Dave Olsthoorn <dave@bewaar.me>,
	X86 ML <x86@kernel.org>,
	Platform Driver <platform-driver-x86@vger.kernel.org>,
	linux-efi <linux-efi@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
	"open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH v11 02/10] efi: Add embedded peripheral firmware support
Date: Sun, 12 Jan 2020 14:45:09 -0800
Message-ID: <CALCETrUz4gdVOH=5X+MkB56ST=DNcHKicRST1j1ff0kU1yXWzw@mail.gmail.com> (raw)
In-Reply-To: <20200111145703.533809-3-hdegoede@redhat.com>

On Sat, Jan 11, 2020 at 6:57 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Just like with PCI options ROMs, which we save in the setup_efi_pci*
> functions from arch/x86/boot/compressed/eboot.c, the EFI code / ROM itself
> sometimes may contain data which is useful/necessary for peripheral drivers
> to have access to.
>
> Specifically the EFI code may contain an embedded copy of firmware which
> needs to be (re)loaded into the peripheral. Normally such firmware would be
> part of linux-firmware, but in some cases this is not feasible, for 2
> reasons:
>
> 1) The firmware is customized for a specific use-case of the chipset / use
> with a specific hardware model, so we cannot have a single firmware file
> for the chipset. E.g. touchscreen controller firmwares are compiled
> specifically for the hardware model they are used with, as they are
> calibrated for a specific model digitizer.
>
> 2) Despite repeated attempts we have failed to get permission to
> redistribute the firmware. This is especially a problem with customized
> firmwares, these get created by the chip vendor for a specific ODM and the
> copyright may partially belong with the ODM, so the chip vendor cannot
> give a blanket permission to distribute these.
>
> This commit adds support for finding peripheral firmware embedded in the
> EFI code and makes the found firmware available through the new
> efi_get_embedded_fw() function.
>
> Support for loading these firmwares through the standard firmware loading
> mechanism is added in a follow-up commit in this patch-series.
>
> Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware near the end
> of start_kernel(), just before calling rest_init(), this is on purpose
> because the typical EFI_BOOT_SERVICES_CODE memory-segment is too large for
> early_memremap(), so the check must be done after mm_init(). This relies
> on EFI_BOOT_SERVICES_CODE not being free-ed until efi_free_boot_services()
> is called, which means that this will only work on x86 for now.
>

A couple general comments:

How does this interact with modules?  It looks like you're referencing
the dmi table from otherwise modular or potentially modular code in
early EFI code, which seems like it will either prevent modular builds
or will actually fail at compile time depending on config.  Perhaps
you should have a single collection of EFI firmware references in a
separate place in the kernel tree and reference *that* from the EFI
code.

In the event you have many DMI matches (e.g. if anyone ends up using
this in a case where the DMI match has to match very broad things),
you'll iterate over the EFI code and data multiple times and
performance will suck.  It would be much better to iterate once and
search for everything.

I suspect that a rolling hash would be better than the prefix you're
using, but this could be changed later, assuming someone can actually
find all the firmware needed.

> +static int __init efi_check_md_for_embedded_firmware(
> +       efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
> +{
> +       const u64 prefix = *((u64 *)desc->prefix);
> +       struct sha256_state sctx;
> +       struct efi_embedded_fw *fw;
> +       u8 sha256[32];
> +       u64 i, size;
> +       void *map;
> +
> +       size = md->num_pages << EFI_PAGE_SHIFT;
> +       map = memremap(md->phys_addr, size, MEMREMAP_WB);
> +       if (!map) {
> +               pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
> +               return -ENOMEM;
> +       }
> +
> +       for (i = 0; (i + desc->length) <= size; i += 8) {
> +               u64 *mem = map + i;
> +
> +               if (*mem != prefix)
> +                       continue;

This is very ugly.  You're casting a pointer to a bunch of bytes to
u64* and then dereferencing it like it's an integer.  This has major
endian issues with are offset by the similar endianness issues when
you type-pun prefix to u64.  You should instead just memcmp the
pointer with the data.  This will get rid of all the type punning in
this function.  You will also fail to detect firmware that isn't
8-byte-aligned.

So perhaps just use memmem() to replace this whole mess?

> +
> +               sha256_init(&sctx);
> +               sha256_update(&sctx, map + i, desc->length);
> +               sha256_final(&sctx, sha256);
> +               if (memcmp(sha256, desc->sha256, 32) == 0)
> +                       break;
> +       }
> +       if ((i + desc->length) > size) {
> +               memunmap(map);
> +               return -ENOENT;
> +       }
> +
> +       pr_info("Found EFI embedded fw '%s'\n", desc->name);
> +

It might be nice to also log which EFI section it was in?

> +       fw = kmalloc(sizeof(*fw), GFP_KERNEL);
> +       if (!fw) {
> +               memunmap(map);
> +               return -ENOMEM;
> +       }
> +
> +       fw->data = kmemdup(map + i, desc->length, GFP_KERNEL);
> +       memunmap(map);
> +       if (!fw->data) {
> +               kfree(fw);
> +               return -ENOMEM;
> +       }
> +
> +       fw->name = desc->name;
> +       fw->length = desc->length;
> +       list_add(&fw->list, &efi_embedded_fw_list);
> +

If you actually copy the firmware name instead of just a pointer to
it, then you could potentially free the list of EFI firmwares.

Why are you copying the firmware into linear (kmemdup) memory here
just to copy it to vmalloc space down below...

> +       return 0;
> +}
> +
> +void __init efi_check_for_embedded_firmwares(void)
> +{
> +       const struct efi_embedded_fw_desc *fw_desc;
> +       const struct dmi_system_id *dmi_id;
> +       efi_memory_desc_t *md;
> +       int i, r;
> +
> +       for (i = 0; embedded_fw_table[i]; i++) {
> +               dmi_id = dmi_first_match(embedded_fw_table[i]);
> +               if (!dmi_id)
> +                       continue;
> +
> +               fw_desc = dmi_id->driver_data;
> +
> +               /*
> +                * In some drivers the struct driver_data contains may contain
> +                * other driver specific data after the fw_desc struct; and
> +                * the fw_desc struct itself may be empty, skip these.
> +                */
> +               if (!fw_desc->name)
> +                       continue;
> +
> +               for_each_efi_memory_desc(md) {
> +                       if (md->type != EFI_BOOT_SERVICES_CODE)
> +                               continue;
> +
> +                       r = efi_check_md_for_embedded_firmware(md, fw_desc);
> +                       if (r == 0)
> +                               break;
> +               }
> +       }
> +
> +       checked_for_fw = true;
> +}

Have you measured how long this takes on a typical system per matching DMI id?

> +
> +int efi_get_embedded_fw(const char *name, void **data, size_t *size)
> +{
> +       struct efi_embedded_fw *iter, *fw = NULL;
> +       void *buf = *data;
> +
> +       if (!checked_for_fw) {

WARN_ON_ONCE?  A stack dump would be quite nice here.

> +       buf = vmalloc(fw->length);
> +       if (!buf)
> +               return -ENOMEM;
> +
> +       memcpy(buf, fw->data, fw->length);
> +       *size = fw->length;
> +       *data = buf;

See above.  What's vmalloc() for?  Where's the vfree()?

BTW, it would be very nice to have a straightforward way
(/sys/kernel/debug/efi_firmware/[name]?) to dump all found firmwares.

  reply index

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-11 14:56 [PATCH v11 00/10] efi/firmware/platform-x86: Add EFI embedded fw support Hans de Goede
2020-01-11 14:56 ` [PATCH v11 01/10] efi: Export boot-services code and data as debugfs-blobs Hans de Goede
2020-01-11 14:56 ` [PATCH v11 02/10] efi: Add embedded peripheral firmware support Hans de Goede
2020-01-12 22:45   ` Andy Lutomirski [this message]
2020-01-14 12:25     ` Hans de Goede
2020-01-14 12:37       ` Andy Shevchenko
2020-01-17 20:06       ` Andy Lutomirski
2020-01-21 11:10         ` Hans de Goede
2020-01-11 14:56 ` [PATCH v11 03/10] firmware: Rename FW_OPT_NOFALLBACK to FW_OPT_NOFALLBACK_SYSFS Hans de Goede
2020-01-11 14:56 ` [PATCH v11 04/10] firmware: Add new platform fallback mechanism and firmware_request_platform() Hans de Goede
2020-01-12 22:54   ` Andy Lutomirski
2020-01-11 14:56 ` [PATCH v11 05/10] test_firmware: add support for firmware_request_platform Hans de Goede
2020-01-13 14:53   ` Luis Chamberlain
2020-01-13 15:22     ` Hans de Goede
2020-01-13 15:50       ` Luis Chamberlain
2020-01-11 14:56 ` [PATCH v11 06/10] selftests: firmware: Add firmware_request_platform tests Hans de Goede
2020-01-11 14:57 ` [PATCH v11 07/10] Input: silead - Switch to firmware_request_platform for retreiving the fw Hans de Goede
2020-01-11 14:57 ` [PATCH v11 08/10] Input: icn8505 " Hans de Goede
2020-01-11 14:57 ` [PATCH v11 09/10] platform/x86: touchscreen_dmi: Add EFI embedded firmware info support Hans de Goede
2020-01-11 14:57 ` [PATCH v11 10/10] platform/x86: touchscreen_dmi: Add info for the Chuwi Vi8 Plus tablet Hans de Goede

Reply instructions:

You may reply publically 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='CALCETrUz4gdVOH=5X+MkB56ST=DNcHKicRST1j1ff0kU1yXWzw@mail.gmail.com' \
    --to=luto@kernel.org \
    --cc=andy@infradead.org \
    --cc=ard.biesheuvel@linaro.org \
    --cc=ardb@kernel.org \
    --cc=bp@alien8.de \
    --cc=corbet@lwn.net \
    --cc=dave@bewaar.me \
    --cc=dmitry.torokhov@gmail.com \
    --cc=dvhart@infradead.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hdegoede@redhat.com \
    --cc=hpa@zytor.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mcgrof@kernel.org \
    --cc=mingo@redhat.com \
    --cc=pjones@redhat.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=rafael@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /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

Linux-EFI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-efi/0 linux-efi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-efi linux-efi/ https://lore.kernel.org/linux-efi \
		linux-efi@vger.kernel.org
	public-inbox-index linux-efi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-efi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git