[PATCH v5 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[PATCH v5 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

[Hans de Goede]

Hi All,

Here is v5 of my patch-set to add support for EFI embedded fw to the kernel.

Changes since v4:
-Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS

So I think this patch-set is getting close to ready for merging, which
brings us to the question of how to merge this, I think that patches 1
and 2 should probably both be merged through the same tree. Then an
unmutable branch should be created on that tree, merged into the
platform/x86 tree and then the last 3 patches can be merged through
that tree.

Ard has already indicated he is fine with the EFI bits going upstream
through another tree, so perhaps patches 1-2 can be merged through the
firmware-loader-tree and then do an unmutable branch on the
firmware-loader-tree for the platform/x86 tree to merge?

I don't think taking all 5 through 1 tree is a good idea because of
the file rename under platform/x86.


For the record here are the cover letters of the previous versions:

Changes since v3:
-Drop note in docs about EFI_FIRMWARE_VOLUME_PROTOCOL, it is not part of
 UEFI proper, so the EFI maintainers don't want us referring people to it
-Use new EFI_BOOT_SERVICES flag
-Put the new fw_get_efi_embedded_fw() function in its own fallback_efi.c
 file which only gets built when EFI_EMBEDDED_FIRMWARE is selected
-Define an empty stub for fw_get_efi_embedded_fw() in fallback.h hwen
 EFI_EMBEDDED_FIRMWARE is not selected, to avoid the need for #ifdefs
 in firmware_loader/main.c
-Properly call security_kernel_post_read_file() on the firmware returned
 by efi_get_embedded_fw() to make sure that we are allowed to use it

The 3 most prominent changes in v2 are:

1) Add documentation describing the EFI embedded firmware mechanism to:
   Documentation/driver-api/firmware/request_firmware.rst

2) Instead of having a single dmi_system_id array with its driver_data
   members pointing to efi_embedded_fw_desc structs, have the drivers which
   need EFI embedded-fw support export a dmi_system_id array and register
   that with the EFI embedded-fw code

   This series also includes the first driver to use this, in the form of
   the touchscreen_dmi code (formerly silead_dmi) from drivers/platfrom/x86

3) As discussed during the review of v1 we want to make the firmware_loader
   code fallback to EFI embedded-fw optional.  Rather the adding yet another
   firmware_request_foo variant for this, with the risk of later also needing
   firmware_request_foo_nowait, etc. variants I've decided to make the code
   check if the device has a "efi-embedded-firmware" device-property bool set.

   This also seemed better because the same driver may want to use the
   fallback on some systems, but not on others since e.g. not all (x86)
   systems with a silead touchscreen have their touchscreen firmware embedded
   in their EFI.

   Note that (as discussed) when the EFI fallback path is requested, the
   usermodehelper fallback path is skipped.

Here is the full changelog of patch 2/5 which is where most of the changes are:

Changes in v2:
-Rebased on driver-core/driver-core-next
-Add documentation describing the EFI embedded firmware mechanism to:
 Documentation/driver-api/firmware/request_firmware.rst
-Add a new EFI_EMBEDDED_FIRMWARE Kconfig bool and only build the embedded
 fw support if this is set. This is an invisible option which should be
 selected by drivers which need this
-Remove the efi_embedded_fw_desc and dmi_system_id-s for known devices
 from the efi-embedded-fw code, instead drivers using this are expected to
 export a dmi_system_id array, with each entries' driver_data pointing to a
 efi_embedded_fw_desc struct and register this with the efi-embedded-fw code
-Use kmemdup to make a copy instead of efi_mem_reserve()-ing the firmware,
 this avoids us messing with the EFI memmap and avoids the need to make
 changes to efi_mem_desc_lookup()
-Make the firmware-loader code only fallback to efi_get_embedded_fw() if the
 passed in device has the "efi-embedded-firmware" device-property bool set
-Skip usermodehelper fallback when "efi-embedded-firmware" device-property
 is set

Patches 3-5 are new and implement using the EFI embedded-fw mechanism for
Silead gslXXXX and Chipone icn8505 touchscreens on x86 devices.

Regards,

Hans

[PATCH v5 1/5] efi: Export boot-services code and data as debugfs-blobs

[Hans de Goede]

Sometimes it is useful to be able to dump the efi boot-services code and
data. This commit adds these as debugfs-blobs to /sys/kernel/debug/efi,
but only if efi=debug is passed on the kernel-commandline as this requires
not freeing those memory-regions, which costs 20+ MB of RAM.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
-Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS

Changes in v4:
-Add new EFI_BOOT_SERVICES flag and use it to determine if the boot-services
 memory segments are available (and thus if it makes sense to register the
 debugfs bits for them)

Changes in v2:
-Do not call pr_err on debugfs call failures
---
 arch/x86/platform/efi/efi.c    |  1 +
 arch/x86/platform/efi/quirks.c |  4 +++
 drivers/firmware/efi/efi.c     | 53 ++++++++++++++++++++++++++++++++++
 include/linux/efi.h            |  1 +
 4 files changed, 59 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 9061babfbc83..82bbbbf0836d 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -208,6 +208,7 @@ int __init efi_memblock_x86_reserve_range(void)
 	     efi.memmap.desc_version);
 
 	memblock_reserve(pmap, efi.memmap.nr_map * efi.memmap.desc_size);
+	set_bit(EFI_PRESERVE_BS_REGIONS, &efi.flags);
 
 	return 0;
 }
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 36c1f8b9f7e0..16bdb9e3b343 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -376,6 +376,10 @@ void __init efi_free_boot_services(void)
 	int num_entries = 0;
 	void *new, *new_md;
 
+	/* Keep all regions for /sys/kernel/debug/efi */
+	if (efi_enabled(EFI_DBG))
+		return;
+
 	for_each_efi_memory_desc(md) {
 		unsigned long long start = md->phys_addr;
 		unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 232f4915223b..1590e4d6a857 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -18,6 +18,7 @@
 #include <linux/kobject.h>
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/efi.h>
 #include <linux/of.h>
@@ -325,6 +326,55 @@ static __init int efivar_ssdt_load(void)
 static inline int efivar_ssdt_load(void) { return 0; }
 #endif
 
+#ifdef CONFIG_DEBUG_FS
+
+#define EFI_DEBUGFS_MAX_BLOBS 32
+
+static struct debugfs_blob_wrapper debugfs_blob[EFI_DEBUGFS_MAX_BLOBS];
+
+static void __init efi_debugfs_init(void)
+{
+	struct dentry *efi_debugfs;
+	efi_memory_desc_t *md;
+	char name[32];
+	int type_count[EFI_BOOT_SERVICES_DATA + 1] = {};
+	int i = 0;
+
+	efi_debugfs = debugfs_create_dir("efi", NULL);
+	if (IS_ERR_OR_NULL(efi_debugfs))
+		return;
+
+	for_each_efi_memory_desc(md) {
+		switch (md->type) {
+		case EFI_BOOT_SERVICES_CODE:
+			snprintf(name, sizeof(name), "boot_services_code%d",
+				 type_count[md->type]++);
+			break;
+		case EFI_BOOT_SERVICES_DATA:
+			snprintf(name, sizeof(name), "boot_services_data%d",
+				 type_count[md->type]++);
+			break;
+		default:
+			continue;
+		}
+
+		debugfs_blob[i].size = md->num_pages << EFI_PAGE_SHIFT;
+		debugfs_blob[i].data = memremap(md->phys_addr,
+						debugfs_blob[i].size,
+						MEMREMAP_WB);
+		if (!debugfs_blob[i].data)
+			continue;
+
+		debugfs_create_blob(name, 0400, efi_debugfs, &debugfs_blob[i]);
+		i++;
+		if (i == EFI_DEBUGFS_MAX_BLOBS)
+			break;
+	}
+}
+#else
+static inline void efi_debugfs_init(void) {}
+#endif
+
 /*
  * We register the efi subsystem with the firmware subsystem and the
  * efivars subsystem with the efi subsystem, if the system was booted with
@@ -369,6 +419,9 @@ static int __init efisubsys_init(void)
 		goto err_remove_group;
 	}
 
+	if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
+		efi_debugfs_init();
+
 	return 0;
 
 err_remove_group:
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f1b7d68ac460..791088360c1e 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1144,6 +1144,7 @@ extern int __init efi_setup_pcdp_console(char *);
 #define EFI_DBG			8	/* Print additional debug info at runtime */
 #define EFI_NX_PE_DATA		9	/* Can runtime data regions be mapped non-executable? */
 #define EFI_MEM_ATTR		10	/* Did firmware publish an EFI_MEMORY_ATTRIBUTES table? */
+#define EFI_PRESERVE_BS_REGIONS	11	/* Are EFI boot-services memory segments available? */
 
 #ifdef CONFIG_EFI
 /*
-- 
2.17.0

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

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 making this available to peripheral drivers through the
standard firmware loading mechanism.

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.

Reported-by: Dave Olsthoorn <dave@bewaar.me>
Suggested-by: Peter Jones <pjones@redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
-Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS

Changes in v4:
-Drop note in docs about EFI_FIRMWARE_VOLUME_PROTOCOL, it is not part of
 UEFI proper, so the EFI maintainers don't want us referring people to it
-Use new EFI_BOOT_SERVICES flag
-Put the new fw_get_efi_embedded_fw() function in its own fallback_efi.c
 file which only gets built when EFI_EMBEDDED_FIRMWARE is selected
-Define an empty stub for fw_get_efi_embedded_fw() in fallback.h hwen
 EFI_EMBEDDED_FIRMWARE is not selected, to avoid the need for #ifdefs
 in firmware_loader/main.c
-Properly call security_kernel_post_read_file() on the firmware returned
 by efi_get_embedded_fw() to make sure that we are allowed to use it

Changes in v3:
-Fix the docs using "efi-embedded-fw" as property name instead of
 "efi-embedded-firmware"

Changes in v2:
-Rebased on driver-core/driver-core-next
-Add documentation describing the EFI embedded firmware mechanism to:
 Documentation/driver-api/firmware/request_firmware.rst
-Add a new EFI_EMBEDDED_FIRMWARE Kconfig bool and only build the embedded
 fw support if this is set. This is an invisible option which should be
 selected by drivers which need this
-Remove the efi_embedded_fw_desc and dmi_system_id-s for known devices
 from the efi-embedded-fw code, instead drivers using this are expected to
 export a dmi_system_id array, with each entries' driver_data pointing to a
 efi_embedded_fw_desc struct and register this with the efi-embedded-fw code
-Use kmemdup to make a copy instead of efi_mem_reserve()-ing the firmware,
 this avoids us messing with the EFI memmap and avoids the need to make
 changes to efi_mem_desc_lookup()
-Make the firmware-loader code only fallback to efi_get_embedded_fw() if the
 passed in device has the "efi-embedded-firmware" device-property bool set
-Skip usermodehelper fallback when "efi-embedded-firmware" device-property
 is set
---
 .../driver-api/firmware/request_firmware.rst  |  66 ++++++++
 drivers/base/firmware_loader/Makefile         |   1 +
 drivers/base/firmware_loader/fallback.h       |  12 ++
 drivers/base/firmware_loader/fallback_efi.c   |  51 ++++++
 drivers/base/firmware_loader/main.c           |   2 +
 drivers/firmware/efi/Kconfig                  |   3 +
 drivers/firmware/efi/Makefile                 |   1 +
 drivers/firmware/efi/embedded-firmware.c      | 149 ++++++++++++++++++
 include/linux/efi.h                           |   6 +
 include/linux/efi_embedded_fw.h               |  25 +++
 init/main.c                                   |   3 +
 11 files changed, 319 insertions(+)
 create mode 100644 drivers/base/firmware_loader/fallback_efi.c
 create mode 100644 drivers/firmware/efi/embedded-firmware.c
 create mode 100644 include/linux/efi_embedded_fw.h

diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
index c8bddbdcfd10..560dfed76e38 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -73,3 +73,69 @@ If something went wrong firmware_request() returns non-zero and fw_entry
 is set to NULL. Once your driver is done with processing the firmware it
 can call call firmware_release(fw_entry) to release the firmware image
 and any related resource.
+
+EFI embedded firmware support
+=============================
+
+On some devices the system's EFI code / ROM may contain an embedded copy
+of firmware for some of the system's integrated peripheral devices and
+the peripheral's Linux device-driver needs to access this firmware.
+
+A device driver which needs this can describe the firmware it needs
+using an efi_embedded_fw_desc struct:
+
+.. kernel-doc:: include/linux/efi_embedded_fw.h
+   :functions: efi_embedded_fw_desc
+
+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.
+
+To avoid doing this somewhat expensive scan on all systems, dmi matching is
+used. Drivers are expected to export a dmi_system_id array, with each entries'
+driver_data pointing to an efi_embedded_fw_desc.
+
+To register this array with the efi-embedded-fw code, a driver needs to:
+
+1. Always be builtin to the kernel or store the dmi_system_id array in a
+   separate object file which always gets builtin.
+
+2. Add an extern declaration for the dmi_system_id array to
+   include/linux/efi_embedded_fw.h.
+
+3. Add the dmi_system_id array to the embedded_fw_table in
+   drivers/firmware/efi/embedded-firmware.c wrapped in a #ifdef testing that
+   the driver is being builtin.
+
+4. Add "select EFI_EMBEDDED_FIRMWARE if EFI_STUB" to its Kconfig entry.
+
+The request_firmware() function will always first try to load firmware with
+the specified name directly from the disk, so the EFI embedded-fw can always
+be overridden by placing a file under /lib/firmare.
+
+To make request_firmware() fallback to trying EFI embedded firmwares after this,
+the driver must set a boolean "efi-embedded-firmware" device-property on the
+device before passing it to request_firmware(). Note that this disables the
+usual usermodehelper fallback, so you may want to only set this on systems
+which match your dmi_system_id array.
+
+Once the device-property is set, the driver can use the regular
+request_firmware() function to get the firmware, using the name filled in
+in the efi_embedded_fw_desc.
+
+Note that:
+
+1. The code scanning for EFI embbedded-firmware runs near the end
+   of start_kernel(), just before calling rest_init(). For normal drivers and
+   subsystems using subsys_initcall() to register themselves this does not
+   matter. This means that code running earlier cannot use EFI
+   embbedded-firmware.
+
+2. ATM the EFI embedded-fw code assumes that firmwares always start at an offset
+   which is a multiple of 8 bytes, if this is not true for your case send in
+   a patch to fix this.
+
+3. ATM the EFI embedded-fw code only works on x86 because other archs free
+   EFI_BOOT_SERVICES_CODE before the EFI embedded-fw code gets a chance to
+   scan it.
diff --git a/drivers/base/firmware_loader/Makefile b/drivers/base/firmware_loader/Makefile
index a97eeb0be1d8..365a040995d3 100644
--- a/drivers/base/firmware_loader/Makefile
+++ b/drivers/base/firmware_loader/Makefile
@@ -5,3 +5,4 @@ obj-y			:= fallback_table.o
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
 firmware_class-objs := main.o
 firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o
+firmware_class-$(CONFIG_EFI_EMBEDDED_FIRMWARE) += fallback_efi.o
diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
index 8cfaa3299bb7..92f462415d25 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -66,4 +66,16 @@ static inline void unregister_sysfs_loader(void)
 }
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
+#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
+			   enum fw_opt *opt_flags, int ret);
+#else
+static inline int fw_get_efi_embedded_fw(struct device *dev,
+					 struct fw_priv *fw_priv,
+					 enum fw_opt *opt_flags, int ret)
+{
+	return ret;
+}
+#endif /* CONFIG_EFI_EMBEDDED_FIRMWARE */
+
 #endif /* __FIRMWARE_FALLBACK_H */
diff --git a/drivers/base/firmware_loader/fallback_efi.c b/drivers/base/firmware_loader/fallback_efi.c
new file mode 100644
index 000000000000..82ba82f48a79
--- /dev/null
+++ b/drivers/base/firmware_loader/fallback_efi.c
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/efi_embedded_fw.h>
+#include <linux/property.h>
+#include <linux/security.h>
+#include <linux/vmalloc.h>
+
+#include "fallback.h"
+#include "firmware.h"
+
+int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
+			   enum fw_opt *opt_flags, int ret)
+{
+	enum kernel_read_file_id id = READING_FIRMWARE;
+	size_t size, max = INT_MAX;
+	int rc;
+
+	if (!dev)
+		return ret;
+
+	if (!device_property_read_bool(dev, "efi-embedded-firmware"))
+		return ret;
+
+	*opt_flags |= FW_OPT_NO_WARN | FW_OPT_NOCACHE | FW_OPT_NOFALLBACK;
+
+	/* Already populated data member means we're loading into a buffer */
+	if (fw_priv->data) {
+		id = READING_FIRMWARE_PREALLOC_BUFFER;
+		max = fw_priv->allocated_size;
+	}
+
+	rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size, max);
+	if (rc) {
+		dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name);
+		return ret;
+	}
+
+	rc = security_kernel_post_read_file(NULL, fw_priv->data, size, id);
+	if (rc) {
+		if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+			vfree(fw_priv->data);
+			fw_priv->data = NULL;
+		}
+		return rc;
+	}
+
+	dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
+	fw_priv->size = size;
+	fw_state_done(fw_priv);
+	return 0;
+}
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index f009566acd35..23c5392eb59e 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -576,6 +576,8 @@ _firmware_request(const struct firmware **firmware_p, const char *name,
 		goto out;
 
 	ret = fw_get_filesystem_firmware(device, fw->priv);
+	if (ret)
+		ret = fw_get_efi_embedded_fw(device, fw->priv, &opt_flags, ret);
 	if (ret) {
 		if (!(opt_flags & FW_OPT_NO_WARN))
 			dev_warn(device,
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 3098410abad8..faefeff7fb34 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -164,6 +164,9 @@ config RESET_ATTACK_MITIGATION
 	  have been evicted, since otherwise it will trigger even on clean
 	  reboots.
 
+config EFI_EMBEDDED_FIRMWARE
+	bool
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index cb805374f4bc..dde12cea8aac 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
 obj-$(CONFIG_EFI_TEST)			+= test/
 obj-$(CONFIG_EFI_DEV_PATH_PARSER)	+= dev-path-parser.o
 obj-$(CONFIG_APPLE_PROPERTIES)		+= apple-properties.o
+obj-$(CONFIG_EFI_EMBEDDED_FIRMWARE)	+= embedded-firmware.o
 
 arm-obj-$(CONFIG_EFI)			:= arm-init.o arm-runtime.o
 obj-$(CONFIG_ARM)			+= $(arm-obj-y)
diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
new file mode 100644
index 000000000000..22a0f598b53d
--- /dev/null
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Support for extracting embedded firmware for peripherals from EFI code,
+ *
+ * Copyright (c) 2018 Hans de Goede <hdegoede@redhat.com>
+ */
+
+#include <linux/crc32.h>
+#include <linux/dmi.h>
+#include <linux/efi.h>
+#include <linux/efi_embedded_fw.h>
+#include <linux/io.h>
+#include <linux/types.h>
+#include <linux/vmalloc.h>
+
+struct embedded_fw {
+	struct list_head list;
+	const char *name;
+	void *data;
+	size_t length;
+};
+
+static LIST_HEAD(found_fw_list);
+
+static const struct dmi_system_id * const embedded_fw_table[] = {
+	NULL
+};
+
+/*
+ * Note the efi_check_for_embedded_firmwares() code currently makes the
+ * following 2 assumptions. This may needs to be revisited if embedded firmware
+ * is found where this is not true:
+ * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory segments
+ * 2) The firmware always starts at an offset which is a multiple of 8 bytes
+ */
+static int __init efi_check_md_for_embedded_firmware(
+	efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
+{
+	struct embedded_fw *fw;
+	u64 i, size;
+	u32 crc;
+	u8 *mem;
+
+	size = md->num_pages << EFI_PAGE_SHIFT;
+	mem = memremap(md->phys_addr, size, MEMREMAP_WB);
+	if (!mem) {
+		pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
+		return -ENOMEM;
+	}
+
+	size -= desc->length;
+	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;
+	}
+
+	memunmap(mem);
+
+	if (i >= size)
+		return -ENOENT;
+
+	pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name, desc->crc);
+
+	fw = kmalloc(sizeof(*fw), GFP_KERNEL);
+	if (!fw)
+		return -ENOMEM;
+
+	mem = memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
+	if (!mem) {
+		pr_err("Error mapping embedded firmware\n");
+		goto error_free_fw;
+	}
+	fw->data = kmemdup(mem, desc->length, GFP_KERNEL);
+	memunmap(mem);
+	if (!fw->data)
+		goto error_free_fw;
+
+	fw->name = desc->name;
+	fw->length = desc->length;
+	list_add(&fw->list, &found_fw_list);
+
+	return 0;
+
+error_free_fw:
+	kfree(fw);
+	return -ENOMEM;
+}
+
+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;
+		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;
+		}
+	}
+}
+
+int efi_get_embedded_fw(const char *name, void **data, size_t *size,
+			size_t msize)
+{
+	struct embedded_fw *iter, *fw = NULL;
+	void *buf = *data;
+
+	list_for_each_entry(iter, &found_fw_list, list) {
+		if (strcmp(name, iter->name) == 0) {
+			fw = iter;
+			break;
+		}
+	}
+
+	if (!fw)
+		return -ENOENT;
+
+	if (msize && msize < fw->length)
+		return -EFBIG;
+
+	if (!buf) {
+		buf = vmalloc(fw->length);
+		if (!buf)
+			return -ENOMEM;
+	}
+
+	memcpy(buf, fw->data, fw->length);
+	*size = fw->length;
+	*data = buf;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(efi_get_embedded_fw);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 791088360c1e..23e8a9c26ce2 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1575,6 +1575,12 @@ static inline void
 efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
 #endif
 
+#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+void efi_check_for_embedded_firmwares(void);
+#else
+static inline void efi_check_for_embedded_firmwares(void) { }
+#endif
+
 void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
 
 /*
diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
new file mode 100644
index 000000000000..0f7d4df3f57a
--- /dev/null
+++ b/include/linux/efi_embedded_fw.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_EFI_EMBEDDED_FW_H
+#define _LINUX_EFI_EMBEDDED_FW_H
+
+#include <linux/mod_devicetable.h>
+
+/**
+ * struct efi_embedded_fw_desc - This struct is used by the EFI embedded-fw
+ *                               code to search for embedded firmwares.
+ *
+ * @name:   Name to register the firmware with if found
+ * @prefix: First 8 bytes of the firmware
+ * @length: Length of the firmware in bytes including prefix
+ * @crc:    Inverted little endian Ethernet style CRC32, with 0xffffffff seed
+ */
+struct efi_embedded_fw_desc {
+	const char *name;
+	u8 prefix[8];
+	u32 length;
+	u32 crc;
+};
+
+int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t msize);
+
+#endif
diff --git a/init/main.c b/init/main.c
index b795aa341a3a..ab29775b35db 100644
--- a/init/main.c
+++ b/init/main.c
@@ -729,6 +729,9 @@ asmlinkage __visible void __init start_kernel(void)
 	arch_post_acpi_subsys_init();
 	sfi_init_late();
 
+	if (efi_enabled(EFI_PRESERVE_BS_REGIONS))
+		efi_check_for_embedded_firmwares();
+
 	if (efi_enabled(EFI_RUNTIME_SERVICES)) {
 		efi_free_boot_services();
 	}
-- 
2.17.0

[PATCH v5 3/5] platform/x86: Rename silead_dmi to touchscreen_dmi

[Hans de Goede]

Not only silead touchscreens need some extra info not available in the
ACPI tables to work properly. X86 devices with a Chipone ICN8505 chip also
need some DMI based extra configuration.

There is no reason to have separate dmi config code per touchscreen
controller vendor. This commit renames silead_dmi to a more generic
touchscreen_dmi name (and Kconfig option) in preparation of adding
info for tablets with an ICN8505 based touchscreen.

Note there are no functional changes all code changes are limited to
removing references to silead where these are no longer applicable.

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 MAINTAINERS                                   |  2 +-
 drivers/platform/x86/Kconfig                  | 16 ++---
 drivers/platform/x86/Makefile                 |  2 +-
 .../x86/{silead_dmi.c => touchscreen_dmi.c}   | 66 +++++++++----------
 4 files changed, 43 insertions(+), 43 deletions(-)
 rename drivers/platform/x86/{silead_dmi.c => touchscreen_dmi.c} (87%)

diff --git a/MAINTAINERS b/MAINTAINERS
index bee29977a3bd..5a215bcd6660 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12795,7 +12795,7 @@ L:	linux-input@vger.kernel.org
 L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
 F:	drivers/input/touchscreen/silead.c
-F:	drivers/platform/x86/silead_dmi.c
+F:	drivers/platform/x86/touchscreen_dmi.c
 
 SILICON MOTION SM712 FRAME BUFFER DRIVER
 M:	Sudip Mukherjee <sudipm.mukherjee@gmail.com>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 39d06dd1f63a..7ff206e5edfe 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1196,16 +1196,16 @@ config INTEL_TURBO_MAX_3
 	  This driver is only required when the system is not using Hardware
 	  P-States (HWP). In HWP mode, priority can be read from ACPI tables.
 
-config SILEAD_DMI
-	bool "Tablets with Silead touchscreens"
+config TOUCHSCREEN_DMI
+	bool "DMI based touchscreen configuration info"
 	depends on ACPI && DMI && I2C=y && TOUCHSCREEN_SILEAD
 	---help---
-	  Certain ACPI based tablets with Silead touchscreens do not have
-	  enough data in ACPI tables for the touchscreen driver to handle
-	  the touchscreen properly, as OEMs expected the data to be baked
-	  into the tablet model specific version of the driver shipped
-	  with the OS-image for the device. This option supplies the missing
-	  information. Enable this for x86 tablets with Silead touchscreens.
+	  Certain ACPI based tablets with e.g. Silead or Chipone touchscreens
+	  do not have enough data in ACPI tables for the touchscreen driver to
+	  handle the touchscreen properly, as OEMs expect the data to be baked
+	  into the tablet model specific version of the driver shipped with the
+	  the OS-image for the device. This option supplies the missing info.
+	  Enable this for x86 tablets with Silead or Chipone touchscreens.
 
 config INTEL_CHTDC_TI_PWRBTN
 	tristate "Intel Cherry Trail Dollar Cove TI power button driver"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 2ba6cb795338..8d9477114fb5 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -78,7 +78,7 @@ obj-$(CONFIG_INTEL_SMARTCONNECT)	+= intel-smartconnect.o
 obj-$(CONFIG_PVPANIC)           += pvpanic.o
 obj-$(CONFIG_ALIENWARE_WMI)	+= alienware-wmi.o
 obj-$(CONFIG_INTEL_PMC_IPC)	+= intel_pmc_ipc.o
-obj-$(CONFIG_SILEAD_DMI)	+= silead_dmi.o
+obj-$(CONFIG_TOUCHSCREEN_DMI)	+= touchscreen_dmi.o
 obj-$(CONFIG_SURFACE_PRO3_BUTTON)	+= surfacepro3_button.o
 obj-$(CONFIG_SURFACE_3_BUTTON)	+= surface3_button.o
 obj-$(CONFIG_INTEL_PUNIT_IPC)  += intel_punit_ipc.o
diff --git a/drivers/platform/x86/silead_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
similarity index 87%
rename from drivers/platform/x86/silead_dmi.c
rename to drivers/platform/x86/touchscreen_dmi.c
index 452aacabaa8e..87fc839b28f7 100644
--- a/drivers/platform/x86/silead_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -1,5 +1,5 @@
 /*
- * Silead touchscreen driver DMI based configuration code
+ * Touchscreen driver DMI based configuration code
  *
  * Copyright (c) 2017 Red Hat Inc.
  *
@@ -20,7 +20,7 @@
 #include <linux/property.h>
 #include <linux/string.h>
 
-struct silead_ts_dmi_data {
+struct ts_dmi_data {
 	const char *acpi_name;
 	const struct property_entry *properties;
 };
@@ -34,7 +34,7 @@ static const struct property_entry cube_iwork8_air_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data cube_iwork8_air_data = {
+static const struct ts_dmi_data cube_iwork8_air_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= cube_iwork8_air_props,
 };
@@ -48,7 +48,7 @@ static const struct property_entry jumper_ezpad_mini3_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data jumper_ezpad_mini3_data = {
+static const struct ts_dmi_data jumper_ezpad_mini3_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= jumper_ezpad_mini3_props,
 };
@@ -62,7 +62,7 @@ static const struct property_entry dexp_ursus_7w_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data dexp_ursus_7w_data = {
+static const struct ts_dmi_data dexp_ursus_7w_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= dexp_ursus_7w_props,
 };
@@ -77,7 +77,7 @@ static const struct property_entry surftab_twin_10_1_st10432_8_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data surftab_twin_10_1_st10432_8_data = {
+static const struct ts_dmi_data surftab_twin_10_1_st10432_8_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= surftab_twin_10_1_st10432_8_props,
 };
@@ -92,7 +92,7 @@ static const struct property_entry surftab_wintron70_st70416_6_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data surftab_wintron70_st70416_6_data = {
+static const struct ts_dmi_data surftab_wintron70_st70416_6_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= surftab_wintron70_st70416_6_props,
 };
@@ -107,7 +107,7 @@ static const struct property_entry gp_electronic_t701_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data gp_electronic_t701_data = {
+static const struct ts_dmi_data gp_electronic_t701_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= gp_electronic_t701_props,
 };
@@ -122,7 +122,7 @@ static const struct property_entry pipo_w2s_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data pipo_w2s_data = {
+static const struct ts_dmi_data pipo_w2s_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= pipo_w2s_props,
 };
@@ -137,7 +137,7 @@ static const struct property_entry pov_mobii_wintab_p800w_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data pov_mobii_wintab_p800w_data = {
+static const struct ts_dmi_data pov_mobii_wintab_p800w_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= pov_mobii_wintab_p800w_props,
 };
@@ -151,7 +151,7 @@ static const struct property_entry itworks_tw891_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data itworks_tw891_data = {
+static const struct ts_dmi_data itworks_tw891_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= itworks_tw891_props,
 };
@@ -165,7 +165,7 @@ static const struct property_entry chuwi_hi8_pro_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data chuwi_hi8_pro_data = {
+static const struct ts_dmi_data chuwi_hi8_pro_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= chuwi_hi8_pro_props,
 };
@@ -181,7 +181,7 @@ static const struct property_entry digma_citi_e200_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data digma_citi_e200_data = {
+static const struct ts_dmi_data digma_citi_e200_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= digma_citi_e200_props,
 };
@@ -198,7 +198,7 @@ static const struct property_entry onda_obook_20_plus_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data onda_obook_20_plus_data = {
+static const struct ts_dmi_data onda_obook_20_plus_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= onda_obook_20_plus_props,
 };
@@ -212,7 +212,7 @@ static const struct property_entry chuwi_hi8_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data chuwi_hi8_data = {
+static const struct ts_dmi_data chuwi_hi8_data = {
 	.acpi_name      = "MSSL0001:00",
 	.properties     = chuwi_hi8_props,
 };
@@ -227,7 +227,7 @@ static const struct property_entry chuwi_vi8_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data chuwi_vi8_data = {
+static const struct ts_dmi_data chuwi_vi8_data = {
 	.acpi_name      = "MSSL1680:00",
 	.properties     = chuwi_vi8_props,
 };
@@ -242,7 +242,7 @@ static const struct property_entry trekstor_primebook_c13_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data trekstor_primebook_c13_data = {
+static const struct ts_dmi_data trekstor_primebook_c13_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= trekstor_primebook_c13_props,
 };
@@ -258,7 +258,7 @@ static const struct property_entry teclast_x98plus2_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data teclast_x98plus2_data = {
+static const struct ts_dmi_data teclast_x98plus2_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= teclast_x98plus2_props,
 };
@@ -272,12 +272,12 @@ static const struct property_entry teclast_x3_plus_props[] = {
 	{ }
 };
 
-static const struct silead_ts_dmi_data teclast_x3_plus_data = {
+static const struct ts_dmi_data teclast_x3_plus_data = {
 	.acpi_name	= "MSSL1680:00",
 	.properties	= teclast_x3_plus_props,
 };
 
-static const struct dmi_system_id silead_ts_dmi_table[] = {
+static const struct dmi_system_id touchscreen_dmi_table[] = {
 	{
 		/* CUBE iwork8 Air */
 		.driver_data = (void *)&cube_iwork8_air_data,
@@ -466,22 +466,22 @@ static const struct dmi_system_id silead_ts_dmi_table[] = {
 	{ },
 };
 
-static const struct silead_ts_dmi_data *silead_ts_data;
+static const struct ts_dmi_data *ts_data;
 
-static void silead_ts_dmi_add_props(struct i2c_client *client)
+static void ts_dmi_add_props(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	int error;
 
 	if (has_acpi_companion(dev) &&
-	    !strncmp(silead_ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
-		error = device_add_properties(dev, silead_ts_data->properties);
+	    !strncmp(ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
+		error = device_add_properties(dev, ts_data->properties);
 		if (error)
 			dev_err(dev, "failed to add properties: %d\n", error);
 	}
 }
 
-static int silead_ts_dmi_notifier_call(struct notifier_block *nb,
+static int ts_dmi_notifier_call(struct notifier_block *nb,
 				       unsigned long action, void *data)
 {
 	struct device *dev = data;
@@ -491,7 +491,7 @@ static int silead_ts_dmi_notifier_call(struct notifier_block *nb,
 	case BUS_NOTIFY_ADD_DEVICE:
 		client = i2c_verify_client(dev);
 		if (client)
-			silead_ts_dmi_add_props(client);
+			ts_dmi_add_props(client);
 		break;
 
 	default:
@@ -501,22 +501,22 @@ static int silead_ts_dmi_notifier_call(struct notifier_block *nb,
 	return 0;
 }
 
-static struct notifier_block silead_ts_dmi_notifier = {
-	.notifier_call = silead_ts_dmi_notifier_call,
+static struct notifier_block ts_dmi_notifier = {
+	.notifier_call = ts_dmi_notifier_call,
 };
 
-static int __init silead_ts_dmi_init(void)
+static int __init ts_dmi_init(void)
 {
 	const struct dmi_system_id *dmi_id;
 	int error;
 
-	dmi_id = dmi_first_match(silead_ts_dmi_table);
+	dmi_id = dmi_first_match(touchscreen_dmi_table);
 	if (!dmi_id)
 		return 0; /* Not an error */
 
-	silead_ts_data = dmi_id->driver_data;
+	ts_data = dmi_id->driver_data;
 
-	error = bus_register_notifier(&i2c_bus_type, &silead_ts_dmi_notifier);
+	error = bus_register_notifier(&i2c_bus_type, &ts_dmi_notifier);
 	if (error)
 		pr_err("%s: failed to register i2c bus notifier: %d\n",
 			__func__, error);
@@ -529,4 +529,4 @@ static int __init silead_ts_dmi_init(void)
  * itself is ready (which happens at postcore initcall level), but before
  * ACPI starts enumerating devices (at subsys initcall level).
  */
-arch_initcall(silead_ts_dmi_init);
+arch_initcall(ts_dmi_init);
-- 
2.17.0

[PATCH v5 4/5] platform/x86: touchscreen_dmi: Add EFI embedded firmware info support

[Hans de Goede]

Sofar we have been unable to get permission from the vendors to put the
firmware for touchscreens listed in touchscreen_dmi in linux-firmware.

Some of the tablets with such a touchscreen have a touchscreen driver, and
thus a copy of the firmware, as part of their EFI code.

This commit adds the necessary info for the new EFI embedded-firmware code
to extract these firmwares, making the touchscreen work OOTB without the
user needing to manually add the firmware.

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/firmware/efi/embedded-firmware.c |  3 +++
 drivers/platform/x86/Kconfig             |  1 +
 drivers/platform/x86/touchscreen_dmi.c   | 26 +++++++++++++++++++++++-
 include/linux/efi_embedded_fw.h          |  2 ++
 4 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
index 22a0f598b53d..36a93a6938f0 100644
--- a/drivers/firmware/efi/embedded-firmware.c
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -23,6 +23,9 @@ struct embedded_fw {
 static LIST_HEAD(found_fw_list);
 
 static const struct dmi_system_id * const embedded_fw_table[] = {
+#ifdef CONFIG_TOUCHSCREEN_DMI
+	touchscreen_dmi_table,
+#endif
 	NULL
 };
 
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 7ff206e5edfe..cc4d95459190 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1199,6 +1199,7 @@ config INTEL_TURBO_MAX_3
 config TOUCHSCREEN_DMI
 	bool "DMI based touchscreen configuration info"
 	depends on ACPI && DMI && I2C=y && TOUCHSCREEN_SILEAD
+	select EFI_EMBEDDED_FIRMWARE if EFI
 	---help---
 	  Certain ACPI based tablets with e.g. Silead or Chipone touchscreens
 	  do not have enough data in ACPI tables for the touchscreen driver to
diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index 87fc839b28f7..6488cd50ba79 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -15,12 +15,15 @@
 #include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
+#include <linux/efi_embedded_fw.h>
 #include <linux/i2c.h>
 #include <linux/notifier.h>
 #include <linux/property.h>
 #include <linux/string.h>
 
 struct ts_dmi_data {
+	/* The EFI embedded-fw code expects this to be the first member! */
+	struct efi_embedded_fw_desc embedded_fw;
 	const char *acpi_name;
 	const struct property_entry *properties;
 };
@@ -31,10 +34,17 @@ static const struct property_entry cube_iwork8_air_props[] = {
 	PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
 	PROPERTY_ENTRY_STRING("firmware-name", "gsl3670-cube-iwork8-air.fw"),
 	PROPERTY_ENTRY_U32("silead,max-fingers", 10),
+	PROPERTY_ENTRY_BOOL("efi-embedded-firmware"),
 	{ }
 };
 
 static const struct ts_dmi_data cube_iwork8_air_data = {
+	.embedded_fw = {
+		.name	= "silead/gsl3670-cube-iwork8-air.fw",
+		.prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+		.length	= 38808,
+		.crc	= 0xfecde51f,
+	},
 	.acpi_name	= "MSSL1680:00",
 	.properties	= cube_iwork8_air_props,
 };
@@ -119,10 +129,17 @@ static const struct property_entry pipo_w2s_props[] = {
 	PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
 	PROPERTY_ENTRY_STRING("firmware-name",
 			      "gsl1680-pipo-w2s.fw"),
+	PROPERTY_ENTRY_BOOL("efi-embedded-firmware"),
 	{ }
 };
 
 static const struct ts_dmi_data pipo_w2s_data = {
+	.embedded_fw = {
+		.name	= "silead/gsl1680-pipo-w2s.fw",
+		.prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+		.length	= 39072,
+		.crc	= 0x28d5dc6c,
+	},
 	.acpi_name	= "MSSL1680:00",
 	.properties	= pipo_w2s_props,
 };
@@ -162,10 +179,17 @@ static const struct property_entry chuwi_hi8_pro_props[] = {
 	PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
 	PROPERTY_ENTRY_STRING("firmware-name", "gsl3680-chuwi-hi8-pro.fw"),
 	PROPERTY_ENTRY_BOOL("silead,home-button"),
+	PROPERTY_ENTRY_BOOL("efi-embedded-firmware"),
 	{ }
 };
 
 static const struct ts_dmi_data chuwi_hi8_pro_data = {
+	.embedded_fw = {
+		.name	= "silead/gsl3680-chuwi-hi8-pro.fw",
+		.prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+		.length	= 39864,
+		.crc	= 0xfe2bedba,
+	},
 	.acpi_name	= "MSSL1680:00",
 	.properties	= chuwi_hi8_pro_props,
 };
@@ -277,7 +301,7 @@ static const struct ts_dmi_data teclast_x3_plus_data = {
 	.properties	= teclast_x3_plus_props,
 };
 
-static const struct dmi_system_id touchscreen_dmi_table[] = {
+const struct dmi_system_id touchscreen_dmi_table[] = {
 	{
 		/* CUBE iwork8 Air */
 		.driver_data = (void *)&cube_iwork8_air_data,
diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
index 0f7d4df3f57a..4c8cfe38ec02 100644
--- a/include/linux/efi_embedded_fw.h
+++ b/include/linux/efi_embedded_fw.h
@@ -20,6 +20,8 @@ struct efi_embedded_fw_desc {
 	u32 crc;
 };
 
+extern const struct dmi_system_id touchscreen_dmi_table[];
+
 int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t msize);
 
 #endif
-- 
2.17.0

[PATCH v5 5/5] platform/x86: touchscreen_dmi: Add info for the Chuwi Vi8 Plus tablet

[Hans de Goede]

Add touchscreen info for the Chuwi Vi8 Plus tablet. This tablet uses a
Chipone ICN8505 touchscreen controller, with the firmware used by the
touchscreen embedded in the EFI firmware.

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/touchscreen_dmi.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index 6488cd50ba79..5fdb6fe878f4 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -301,6 +301,22 @@ static const struct ts_dmi_data teclast_x3_plus_data = {
 	.properties	= teclast_x3_plus_props,
 };
 
+static const struct property_entry efi_embedded_fw_props[] = {
+	PROPERTY_ENTRY_BOOL("efi-embedded-firmware"),
+	{ }
+};
+
+static const struct ts_dmi_data chuwi_vi8_plus_data = {
+	.embedded_fw = {
+		.name	= "chipone/icn8505-HAMP0002.fw",
+		.prefix = { 0xb0, 0x07, 0x00, 0x00, 0xe4, 0x07, 0x00, 0x00 },
+		.length	= 35012,
+		.crc	= 0x74dfd3fc,
+	},
+	.acpi_name	= "CHPN0001:00",
+	.properties	= efi_embedded_fw_props,
+};
+
 const struct dmi_system_id touchscreen_dmi_table[] = {
 	{
 		/* CUBE iwork8 Air */
@@ -487,6 +503,15 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "Y8W81"),
 		},
 	},
+	{
+		/* Chuwi Vi8 Plus (CWI506) */
+		.driver_data = (void *)&chuwi_vi8_plus_data,
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "D2D3_Vi8A1"),
+			DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
+		},
+	},
 	{ },
 };
 
-- 
2.17.0

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Mimi Zohar]

[Cc'ing linux-security]

On Sun, 2018-04-29 at 11:35 +0200, Hans de Goede wrote:
[...]
> diff --git a/drivers/base/firmware_loader/fallback_efi.c b/drivers/base/firmware_loader/fallback_efi.c
> new file mode 100644
> index 000000000000..82ba82f48a79
> --- /dev/null
> +++ b/drivers/base/firmware_loader/fallback_efi.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/efi_embedded_fw.h>
> +#include <linux/property.h>
> +#include <linux/security.h>
> +#include <linux/vmalloc.h>
> +
> +#include "fallback.h"
> +#include "firmware.h"
> +
> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
> +			   enum fw_opt *opt_flags, int ret)
> +{
> +	enum kernel_read_file_id id = READING_FIRMWARE;

Please define a new kernel_read_file_id for this (eg.
READING_FIRMWARE_EFI_EMBEDDED).

> +	size_t size, max = INT_MAX;
> +	int rc;
> +
> +	if (!dev)
> +		return ret;
> +
> +	if (!device_property_read_bool(dev, "efi-embedded-firmware"))
> +		return ret;

Instead of calling security_kernel_post_read_file(), either in
device_property_read_bool() or here call security_kernel_read_file().

The pre read call is for deciding whether to allow this call
independent of the firmware being loaded, whereas the post security
call is currently being used by IMA-appraisal for verifying a
signature.  There might be other LSMs using the post hook as well.  As
there is no kernel signature associated with this firmware, use the
security pre read_file hook.

thanks,

Mimi

> +
> +	*opt_flags |= FW_OPT_NO_WARN | FW_OPT_NOCACHE | FW_OPT_NOFALLBACK;
> +
> +	/* Already populated data member means we're loading into a buffer */
> +	if (fw_priv->data) {
> +		id = READING_FIRMWARE_PREALLOC_BUFFER;
> +		max = fw_priv->allocated_size;
> +	}
> +
> +	rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size, max);
> +	if (rc) {
> +		dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name);
> +		return ret;
> +	}
> +
> +	rc = security_kernel_post_read_file(NULL, fw_priv->data, size, id);
> +	if (rc) {
> +		if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> +			vfree(fw_priv->data);
> +			fw_priv->data = NULL;
> +		}
> +		return rc;
> +	}
> +
> +	dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
> +	fw_priv->size = size;
> +	fw_state_done(fw_priv);
> +	return 0;
> +}

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Mimi Zohar]

[Cc'ing linux-security]

On Sun, 2018-04-29 at 11:35 +0200, Hans de Goede wrote:
[...]
> diff --git a/drivers/base/firmware_loader/fallback_efi.c b/drivers/base/firmware_loader/fallback_efi.c
> new file mode 100644
> index 000000000000..82ba82f48a79
> --- /dev/null
> +++ b/drivers/base/firmware_loader/fallback_efi.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/efi_embedded_fw.h>
> +#include <linux/property.h>
> +#include <linux/security.h>
> +#include <linux/vmalloc.h>
> +
> +#include "fallback.h"
> +#include "firmware.h"
> +
> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
> +			   enum fw_opt *opt_flags, int ret)
> +{
> +	enum kernel_read_file_id id = READING_FIRMWARE;

Please define a new kernel_read_file_id for this (eg.
READING_FIRMWARE_EFI_EMBEDDED).

> +	size_t size, max = INT_MAX;
> +	int rc;
> +
> +	if (!dev)
> +		return ret;
> +
> +	if (!device_property_read_bool(dev, "efi-embedded-firmware"))
> +		return ret;

Instead of calling security_kernel_post_read_file(), either in
device_property_read_bool() or here call security_kernel_read_file().

The pre read call is for deciding whether to allow this call
independent of the firmware being loaded, whereas the post security
call is currently being used by IMA-appraisal for verifying a
signature. ?There might be other LSMs using the post hook as well. ?As
there is no kernel signature associated with this firmware, use the
security pre read_file hook.

thanks,

Mimi

> +
> +	*opt_flags |= FW_OPT_NO_WARN | FW_OPT_NOCACHE | FW_OPT_NOFALLBACK;
> +
> +	/* Already populated data member means we're loading into a buffer */
> +	if (fw_priv->data) {
> +		id = READING_FIRMWARE_PREALLOC_BUFFER;
> +		max = fw_priv->allocated_size;
> +	}
> +
> +	rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size, max);
> +	if (rc) {
> +		dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name);
> +		return ret;
> +	}
> +
> +	rc = security_kernel_post_read_file(NULL, fw_priv->data, size, id);
> +	if (rc) {
> +		if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> +			vfree(fw_priv->data);
> +			fw_priv->data = NULL;
> +		}
> +		return rc;
> +	}
> +
> +	dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
> +	fw_priv->size = size;
> +	fw_state_done(fw_priv);
> +	return 0;
> +}

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

Hi,

On 01-05-18 16:36, Mimi Zohar wrote:
> [Cc'ing linux-security]
> 
> On Sun, 2018-04-29 at 11:35 +0200, Hans de Goede wrote:
> [...]
>> diff --git a/drivers/base/firmware_loader/fallback_efi.c b/drivers/base/firmware_loader/fallback_efi.c
>> new file mode 100644
>> index 000000000000..82ba82f48a79
>> --- /dev/null
>> +++ b/drivers/base/firmware_loader/fallback_efi.c
>> @@ -0,0 +1,51 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/efi_embedded_fw.h>
>> +#include <linux/property.h>
>> +#include <linux/security.h>
>> +#include <linux/vmalloc.h>
>> +
>> +#include "fallback.h"
>> +#include "firmware.h"
>> +
>> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
>> +			   enum fw_opt *opt_flags, int ret)
>> +{
>> +	enum kernel_read_file_id id = READING_FIRMWARE;
> 
> Please define a new kernel_read_file_id for this (eg.
> READING_FIRMWARE_EFI_EMBEDDED).

Are you sure, I wonder how useful it is to add a new
kernel_read_file_id every time a new way to get firmware
comes up?

I especially wonder about the sense in adding a new id
given that the quite old FIRMWARE_PREALLOC_BUFFER is
still not supported / checked properly by the security code.

Anyways I can add a new id if you want me to, what about
when fw_get_efi_embedded_fw is reading into a driver allocated
buffer, do you want a separate EADING_FIRMWARE_EFI_EMBEDDED_PREALLOC_BUFFER
for that ?




> 
>> +	size_t size, max = INT_MAX;
>> +	int rc;
>> +
>> +	if (!dev)
>> +		return ret;
>> +
>> +	if (!device_property_read_bool(dev, "efi-embedded-firmware"))
>> +		return ret;
> 
> Instead of calling security_kernel_post_read_file(), either in
> device_property_read_bool() or here call security_kernel_read_file().
> 
> The pre read call is for deciding whether to allow this call
> independent of the firmware being loaded, whereas the post security
> call is currently being used by IMA-appraisal for verifying a
> signature.  There might be other LSMs using the post hook as well.  As
> there is no kernel signature associated with this firmware, use the
> security pre read_file hook.

Only the pre hook?  I believe the post-hook should still be called too,
right? So that we've hashes of all loaded firmwares in the IMA core.

Regards,

Hans


> 
> thanks,
> 
> Mimi
> 
>> +
>> +	*opt_flags |= FW_OPT_NO_WARN | FW_OPT_NOCACHE | FW_OPT_NOFALLBACK;
>> +
>> +	/* Already populated data member means we're loading into a buffer */
>> +	if (fw_priv->data) {
>> +		id = READING_FIRMWARE_PREALLOC_BUFFER;
>> +		max = fw_priv->allocated_size;
>> +	}
>> +
>> +	rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size, max);
>> +	if (rc) {
>> +		dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name);
>> +		return ret;
>> +	}
>> +
>> +	rc = security_kernel_post_read_file(NULL, fw_priv->data, size, id);
>> +	if (rc) {
>> +		if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
>> +			vfree(fw_priv->data);
>> +			fw_priv->data = NULL;
>> +		}
>> +		return rc;
>> +	}
>> +
>> +	dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
>> +	fw_priv->size = size;
>> +	fw_state_done(fw_priv);
>> +	return 0;
>> +}
> 

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

Hi,

On 01-05-18 16:36, Mimi Zohar wrote:
> [Cc'ing linux-security]
> 
> On Sun, 2018-04-29 at 11:35 +0200, Hans de Goede wrote:
> [...]
>> diff --git a/drivers/base/firmware_loader/fallback_efi.c b/drivers/base/firmware_loader/fallback_efi.c
>> new file mode 100644
>> index 000000000000..82ba82f48a79
>> --- /dev/null
>> +++ b/drivers/base/firmware_loader/fallback_efi.c
>> @@ -0,0 +1,51 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/efi_embedded_fw.h>
>> +#include <linux/property.h>
>> +#include <linux/security.h>
>> +#include <linux/vmalloc.h>
>> +
>> +#include "fallback.h"
>> +#include "firmware.h"
>> +
>> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
>> +			   enum fw_opt *opt_flags, int ret)
>> +{
>> +	enum kernel_read_file_id id = READING_FIRMWARE;
> 
> Please define a new kernel_read_file_id for this (eg.
> READING_FIRMWARE_EFI_EMBEDDED).

Are you sure, I wonder how useful it is to add a new
kernel_read_file_id every time a new way to get firmware
comes up?

I especially wonder about the sense in adding a new id
given that the quite old FIRMWARE_PREALLOC_BUFFER is
still not supported / checked properly by the security code.

Anyways I can add a new id if you want me to, what about
when fw_get_efi_embedded_fw is reading into a driver allocated
buffer, do you want a separate EADING_FIRMWARE_EFI_EMBEDDED_PREALLOC_BUFFER
for that ?




> 
>> +	size_t size, max = INT_MAX;
>> +	int rc;
>> +
>> +	if (!dev)
>> +		return ret;
>> +
>> +	if (!device_property_read_bool(dev, "efi-embedded-firmware"))
>> +		return ret;
> 
> Instead of calling security_kernel_post_read_file(), either in
> device_property_read_bool() or here call security_kernel_read_file().
> 
> The pre read call is for deciding whether to allow this call
> independent of the firmware being loaded, whereas the post security
> call is currently being used by IMA-appraisal for verifying a
> signature. ?There might be other LSMs using the post hook as well. ?As
> there is no kernel signature associated with this firmware, use the
> security pre read_file hook.

Only the pre hook?  I believe the post-hook should still be called too,
right? So that we've hashes of all loaded firmwares in the IMA core.

Regards,

Hans


> 
> thanks,
> 
> Mimi
> 
>> +
>> +	*opt_flags |= FW_OPT_NO_WARN | FW_OPT_NOCACHE | FW_OPT_NOFALLBACK;
>> +
>> +	/* Already populated data member means we're loading into a buffer */
>> +	if (fw_priv->data) {
>> +		id = READING_FIRMWARE_PREALLOC_BUFFER;
>> +		max = fw_priv->allocated_size;
>> +	}
>> +
>> +	rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size, max);
>> +	if (rc) {
>> +		dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name);
>> +		return ret;
>> +	}
>> +
>> +	rc = security_kernel_post_read_file(NULL, fw_priv->data, size, id);
>> +	if (rc) {
>> +		if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
>> +			vfree(fw_priv->data);
>> +			fw_priv->data = NULL;
>> +		}
>> +		return rc;
>> +	}
>> +
>> +	dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
>> +	fw_priv->size = size;
>> +	fw_state_done(fw_priv);
>> +	return 0;
>> +}
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Mimi Zohar]

On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote:
> Hi,
> 
> On 01-05-18 16:36, Mimi Zohar wrote:
> > [Cc'ing linux-security]
> > 
> > On Sun, 2018-04-29 at 11:35 +0200, Hans de Goede wrote:
> > [...]
> >> diff --git a/drivers/base/firmware_loader/fallback_efi.c b/drivers/base/firmware_loader/fallback_efi.c
> >> new file mode 100644
> >> index 000000000000..82ba82f48a79
> >> --- /dev/null
> >> +++ b/drivers/base/firmware_loader/fallback_efi.c
> >> @@ -0,0 +1,51 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +#include <linux/efi_embedded_fw.h>
> >> +#include <linux/property.h>
> >> +#include <linux/security.h>
> >> +#include <linux/vmalloc.h>
> >> +
> >> +#include "fallback.h"
> >> +#include "firmware.h"
> >> +
> >> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
> >> +			   enum fw_opt *opt_flags, int ret)
> >> +{
> >> +	enum kernel_read_file_id id = READING_FIRMWARE;
> > 
> > Please define a new kernel_read_file_id for this (eg.
> > READING_FIRMWARE_EFI_EMBEDDED).
> 
> Are you sure, I wonder how useful it is to add a new
> kernel_read_file_id every time a new way to get firmware
> comes up?
> 
> I especially wonder about the sense in adding a new id
> given that the quite old FIRMWARE_PREALLOC_BUFFER is
> still not supported / checked properly by the security code.

I posted patches earlier today[1], which address this.  Patch 5/6 just
makes it equivalent to READING_FIRMWARE.  Patch 6/6 questions whether
the device has access to the pre-allocated buffer *before* the
signature has been verified.

[1] kernsec.org/pipermail/linux-security-module-archive/2018-May/006639.html

> 
> Anyways I can add a new id if you want me to, what about
> when fw_get_efi_embedded_fw is reading into a driver allocated
> buffer, do you want a separate EADING_FIRMWARE_EFI_EMBEDDED_PREALLOC_BUFFER
> for that ?

Without the kernel being able to verify the firmware's signature, I'm
not sure it makes much of a difference.

> 
> > 
> >> +	size_t size, max = INT_MAX;
> >> +	int rc;
> >> +
> >> +	if (!dev)
> >> +		return ret;
> >> +
> >> +	if (!device_property_read_bool(dev, "efi-embedded-firmware"))
> >> +		return ret;
> > 
> > Instead of calling security_kernel_post_read_file(), either in
> > device_property_read_bool() or here call security_kernel_read_file().
> > 
> > The pre read call is for deciding whether to allow this call
> > independent of the firmware being loaded, whereas the post security
> > call is currently being used by IMA-appraisal for verifying a
> > signature.  There might be other LSMs using the post hook as well.  As
> > there is no kernel signature associated with this firmware, use the
> > security pre read_file hook.
> 
> Only the pre hook?  I believe the post-hook should still be called too,
> right? So that we've hashes of all loaded firmwares in the IMA core.

Good catch!  Right, if IMA-measurement is enabled, then we would want
to add the measurement.

Mimi

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Mimi Zohar]

On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote:
> Hi,
> 
> On 01-05-18 16:36, Mimi Zohar wrote:
> > [Cc'ing linux-security]
> > 
> > On Sun, 2018-04-29 at 11:35 +0200, Hans de Goede wrote:
> > [...]
> >> diff --git a/drivers/base/firmware_loader/fallback_efi.c b/drivers/base/firmware_loader/fallback_efi.c
> >> new file mode 100644
> >> index 000000000000..82ba82f48a79
> >> --- /dev/null
> >> +++ b/drivers/base/firmware_loader/fallback_efi.c
> >> @@ -0,0 +1,51 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +#include <linux/efi_embedded_fw.h>
> >> +#include <linux/property.h>
> >> +#include <linux/security.h>
> >> +#include <linux/vmalloc.h>
> >> +
> >> +#include "fallback.h"
> >> +#include "firmware.h"
> >> +
> >> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
> >> +			   enum fw_opt *opt_flags, int ret)
> >> +{
> >> +	enum kernel_read_file_id id = READING_FIRMWARE;
> > 
> > Please define a new kernel_read_file_id for this (eg.
> > READING_FIRMWARE_EFI_EMBEDDED).
> 
> Are you sure, I wonder how useful it is to add a new
> kernel_read_file_id every time a new way to get firmware
> comes up?
> 
> I especially wonder about the sense in adding a new id
> given that the quite old FIRMWARE_PREALLOC_BUFFER is
> still not supported / checked properly by the security code.

I posted patches earlier today[1], which address this. ?Patch 5/6 just
makes it equivalent to READING_FIRMWARE. ?Patch 6/6 questions whether
the device has access to the pre-allocated buffer *before* the
signature has been verified.

[1] kernsec.org/pipermail/linux-security-module-archive/2018-May/006639.html

> 
> Anyways I can add a new id if you want me to, what about
> when fw_get_efi_embedded_fw is reading into a driver allocated
> buffer, do you want a separate EADING_FIRMWARE_EFI_EMBEDDED_PREALLOC_BUFFER
> for that ?

Without the kernel being able to verify the firmware's signature, I'm
not sure it makes much of a difference.

> 
> > 
> >> +	size_t size, max = INT_MAX;
> >> +	int rc;
> >> +
> >> +	if (!dev)
> >> +		return ret;
> >> +
> >> +	if (!device_property_read_bool(dev, "efi-embedded-firmware"))
> >> +		return ret;
> > 
> > Instead of calling security_kernel_post_read_file(), either in
> > device_property_read_bool() or here call security_kernel_read_file().
> > 
> > The pre read call is for deciding whether to allow this call
> > independent of the firmware being loaded, whereas the post security
> > call is currently being used by IMA-appraisal for verifying a
> > signature. ?There might be other LSMs using the post hook as well. ?As
> > there is no kernel signature associated with this firmware, use the
> > security pre read_file hook.
> 
> Only the pre hook?  I believe the post-hook should still be called too,
> right? So that we've hashes of all loaded firmwares in the IMA core.

Good catch! ?Right, if IMA-measurement is enabled, then we would want
to add the measurement.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Andy Lutomirski]

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?  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.

> +       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.

(To be clear, I don't love my own suggestion here.  What I'd *really* like
to see is a better interface and no attempt to match the data to some
built-in hash at all.  In particular, there are plenty of devices for which
the driver wants access to a genuinely device-specific blob.  For example,
I'm typing this email while connected to a router that is running ath10k
and is using a calibration blob awkwardly snarfed out of flash somewhere.
It would be really nice if there was a way to pull a blob out of EFI space
that is marked, by EFI, as belonging to a particular device.  Then the
firmware could just pass it over without any particular verification.  But
since your code is literally scanning a wide swath of physical memory for
something that looks like a valid blob, I think you need to use a
cryptographically strong concept of validity.)

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Andy Lutomirski]

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?  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.

> +       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.

(To be clear, I don't love my own suggestion here.  What I'd *really* like
to see is a better interface and no attempt to match the data to some
built-in hash at all.  In particular, there are plenty of devices for which
the driver wants access to a genuinely device-specific blob.  For example,
I'm typing this email while connected to a router that is running ath10k
and is using a calibration blob awkwardly snarfed out of flash somewhere.
It would be really nice if there was a way to pull a blob out of EFI space
that is marked, by EFI, as belonging to a particular device.  Then the
firmware could just pass it over without any particular verification.  But
since your code is literally scanning a wide swath of physical memory for
something that looks like a valid blob, I think you need to use a
cryptographically strong concept of validity.)

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Andy Lutomirski]

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?  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.

> +       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.

(To be clear, I don't love my own suggestion here.  What I'd *really* like
to see is a better interface and no attempt to match the data to some
built-in hash at all.  In particular, there are plenty of devices for which
the driver wants access to a genuinely device-specific blob.  For example,
I'm typing this email while connected to a router that is running ath10k
and is using a calibration blob awkwardly snarfed out of flash somewhere.
It would be really nice if there was a way to pull a blob out of EFI space
that is marked, by EFI, as belonging to a particular device.  Then the
firmware could just pass it over without any particular verification.  But
since your code is literally scanning a wide swath of physical memory for
something that looks like a valid blob, I think you need to use a
cryptographically strong concept of validity.)
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Lukas Wunner]

On Tue, May 01, 2018 at 07:29:19PM +0000, Andy Lutomirski wrote:
> On Sun, Apr 29, 2018 at 2:36 AM Hans de Goede <hdegoede@redhat.com> wrote:
> > +       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.
[snip]
> It would be really nice if there was a way to pull a blob out of EFI space
> that is marked, by EFI, as belonging to a particular device.

Upthread I suggested to read the firmware from the EFI Firmware Volume.
>From my point of view that would fulfill your requirements:
https://lkml.org/lkml/2018/4/3/568

In the case of Hans' HID device, the firmware is embedded as a binary
array in the EFI driver for the device.  So the kernel would read the
driver from the Firmware Volume, but it would still have to extract
the firmware from the driver, either by scanning for the 8 byte magic
or by hardcoding the offset and length in the kernel.

An attacker would have to modify the Firmware Volume as opposed to
just modifying a particular space in memory.

My suggestion was dismissed by Hans and Peter Jones on the grounds of
causing additional work without perceived benefit, given that a working
(albeit admitted to be hackish) solution exists.

Moreover Ard criticized that the EFI Firmware Volume Protocol is not
part of the UEFI spec.

I agree with you completely BTW.

Thanks,

Lukas

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Lukas Wunner]

On Tue, May 01, 2018 at 07:29:19PM +0000, Andy Lutomirski wrote:
> On Sun, Apr 29, 2018 at 2:36 AM Hans de Goede <hdegoede@redhat.com> wrote:
> > +       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.
[snip]
> It would be really nice if there was a way to pull a blob out of EFI space
> that is marked, by EFI, as belonging to a particular device.

Upthread I suggested to read the firmware from the EFI Firmware Volume.
>From my point of view that would fulfill your requirements:
https://lkml.org/lkml/2018/4/3/568

In the case of Hans' HID device, the firmware is embedded as a binary
array in the EFI driver for the device.  So the kernel would read the
driver from the Firmware Volume, but it would still have to extract
the firmware from the driver, either by scanning for the 8 byte magic
or by hardcoding the offset and length in the kernel.

An attacker would have to modify the Firmware Volume as opposed to
just modifying a particular space in memory.

My suggestion was dismissed by Hans and Peter Jones on the grounds of
causing additional work without perceived benefit, given that a working
(albeit admitted to be hackish) solution exists.

Moreover Ard criticized that the EFI Firmware Volume Protocol is not
part of the UEFI spec.

I agree with you completely BTW.

Thanks,

Lukas

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Lukas Wunner]

On Tue, May 01, 2018 at 07:29:19PM +0000, Andy Lutomirski wrote:
> On Sun, Apr 29, 2018 at 2:36 AM Hans de Goede <hdegoede@redhat.com> wrote:
> > +       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.
[snip]
> It would be really nice if there was a way to pull a blob out of EFI space
> that is marked, by EFI, as belonging to a particular device.

Upthread I suggested to read the firmware from the EFI Firmware Volume.
>From my point of view that would fulfill your requirements:
https://lkml.org/lkml/2018/4/3/568

In the case of Hans' HID device, the firmware is embedded as a binary
array in the EFI driver for the device.  So the kernel would read the
driver from the Firmware Volume, but it would still have to extract
the firmware from the driver, either by scanning for the 8 byte magic
or by hardcoding the offset and length in the kernel.

An attacker would have to modify the Firmware Volume as opposed to
just modifying a particular space in memory.

My suggestion was dismissed by Hans and Peter Jones on the grounds of
causing additional work without perceived benefit, given that a working
(albeit admitted to be hackish) solution exists.

Moreover Ard criticized that the EFI Firmware Volume Protocol is not
part of the UEFI spec.

I agree with you completely BTW.

Thanks,

Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

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.

> (To be clear, I don't love my own suggestion here.  What I'd *really* like
> to see is a better interface and no attempt to match the data to some
> built-in hash at all.  In particular, there are plenty of devices for which
> the driver wants access to a genuinely device-specific blob.  For example,
> I'm typing this email while connected to a router that is running ath10k
> and is using a calibration blob awkwardly snarfed out of flash somewhere.
> It would be really nice if there was a way to pull a blob out of EFI space
> that is marked, by EFI, as belonging to a particular device.  Then the
> firmware could just pass it over without any particular verification.  But
> since your code is literally scanning a wide swath of physical memory for
> something that looks like a valid blob, I think you need to use a
> cryptographically strong concept of validity.)

Yes ideally this would not be needed at all and/or use a well defined
interface, but alas we don't live in an ideal world :)

Regards,

Hans

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

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.

> (To be clear, I don't love my own suggestion here.  What I'd *really* like
> to see is a better interface and no attempt to match the data to some
> built-in hash at all.  In particular, there are plenty of devices for which
> the driver wants access to a genuinely device-specific blob.  For example,
> I'm typing this email while connected to a router that is running ath10k
> and is using a calibration blob awkwardly snarfed out of flash somewhere.
> It would be really nice if there was a way to pull a blob out of EFI space
> that is marked, by EFI, as belonging to a particular device.  Then the
> firmware could just pass it over without any particular verification.  But
> since your code is literally scanning a wide swath of physical memory for
> something that looks like a valid blob, I think you need to use a
> cryptographically strong concept of validity.)

Yes ideally this would not be needed at all and/or use a well defined
interface, but alas we don't live in an ideal world :)

Regards,

Hans

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

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.

> (To be clear, I don't love my own suggestion here.  What I'd *really* like
> to see is a better interface and no attempt to match the data to some
> built-in hash at all.  In particular, there are plenty of devices for which
> the driver wants access to a genuinely device-specific blob.  For example,
> I'm typing this email while connected to a router that is running ath10k
> and is using a calibration blob awkwardly snarfed out of flash somewhere.
> It would be really nice if there was a way to pull a blob out of EFI space
> that is marked, by EFI, as belonging to a particular device.  Then the
> firmware could just pass it over without any particular verification.  But
> since your code is literally scanning a wide swath of physical memory for
> something that looks like a valid blob, I think you need to use a
> cryptographically strong concept of validity.)

Yes ideally this would not be needed at all and/or use a well defined
interface, but alas we don't live in an ideal world :)

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

On Tue, May 01, 2018 at 03:27:27PM -0400, Mimi Zohar wrote:
> On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote:
> > Only the pre hook?  I believe the post-hook should still be called too,
> > right? So that we've hashes of all loaded firmwares in the IMA core.
> 
> Good catch!  Right, if IMA-measurement is enabled, then we would want
> to add the measurement.

Mimi, just a heads up, we only use the post hook for the syfs fallback
mechanism, ie, we don't even use the post hook for direct fs lookup.
Do we want that there?

  Luis

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

On Tue, May 01, 2018 at 03:27:27PM -0400, Mimi Zohar wrote:
> On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote:
> > Only the pre hook?  I believe the post-hook should still be called too,
> > right? So that we've hashes of all loaded firmwares in the IMA core.
> 
> Good catch!  Right, if IMA-measurement is enabled, then we would want
> to add the measurement.

Mimi, just a heads up, we only use the post hook for the syfs fallback
mechanism, ie, we don't even use the post hook for direct fs lookup.
Do we want that there?

  Luis

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

On Tue, May 01, 2018 at 03:27:27PM -0400, Mimi Zohar wrote:
> On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote:
> > Only the pre hook?  I believe the post-hook should still be called too,
> > right? So that we've hashes of all loaded firmwares in the IMA core.
> 
> Good catch! ?Right, if IMA-measurement is enabled, then we would want
> to add the measurement.

Mimi, just a heads up, we only use the post hook for the syfs fallback
mechanism, ie, we don't even use the post hook for direct fs lookup.
Do we want that there?

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

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. It seems unclear if we're
all happy with this yet as well.

  Luis

> > (To be clear, I don't love my own suggestion here.  What I'd *really* like
> > to see is a better interface and no attempt to match the data to some
> > built-in hash at all.  In particular, there are plenty of devices for which
> > the driver wants access to a genuinely device-specific blob.  For example,
> > I'm typing this email while connected to a router that is running ath10k
> > and is using a calibration blob awkwardly snarfed out of flash somewhere.
> > It would be really nice if there was a way to pull a blob out of EFI space
> > that is marked, by EFI, as belonging to a particular device.  Then the
> > firmware could just pass it over without any particular verification.  But
> > since your code is literally scanning a wide swath of physical memory for
> > something that looks like a valid blob, I think you need to use a
> > cryptographically strong concept of validity.)
> 
> Yes ideally this would not be needed at all and/or use a well defined
> interface, but alas we don't live in an ideal world :)
> 
> Regards,
> 
> Hans
> 

-- 
Do not panic

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

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. It seems unclear if we're
all happy with this yet as well.

  Luis

> > (To be clear, I don't love my own suggestion here.  What I'd *really* like
> > to see is a better interface and no attempt to match the data to some
> > built-in hash at all.  In particular, there are plenty of devices for which
> > the driver wants access to a genuinely device-specific blob.  For example,
> > I'm typing this email while connected to a router that is running ath10k
> > and is using a calibration blob awkwardly snarfed out of flash somewhere.
> > It would be really nice if there was a way to pull a blob out of EFI space
> > that is marked, by EFI, as belonging to a particular device.  Then the
> > firmware could just pass it over without any particular verification.  But
> > since your code is literally scanning a wide swath of physical memory for
> > something that looks like a valid blob, I think you need to use a
> > cryptographically strong concept of validity.)
> 
> Yes ideally this would not be needed at all and/or use a well defined
> interface, but alas we don't live in an ideal world :)
> 
> Regards,
> 
> Hans
> 

-- 
Do not panic

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

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. It seems unclear if we're
all happy with this yet as well.

  Luis

> > (To be clear, I don't love my own suggestion here.  What I'd *really* like
> > to see is a better interface and no attempt to match the data to some
> > built-in hash at all.  In particular, there are plenty of devices for which
> > the driver wants access to a genuinely device-specific blob.  For example,
> > I'm typing this email while connected to a router that is running ath10k
> > and is using a calibration blob awkwardly snarfed out of flash somewhere.
> > It would be really nice if there was a way to pull a blob out of EFI space
> > that is marked, by EFI, as belonging to a particular device.  Then the
> > firmware could just pass it over without any particular verification.  But
> > since your code is literally scanning a wide swath of physical memory for
> > something that looks like a valid blob, I think you need to use a
> > cryptographically strong concept of validity.)
> 
> Yes ideally this would not be needed at all and/or use a well defined
> interface, but alas we don't live in an ideal world :)
> 
> Regards,
> 
> Hans
> 

-- 
Do not panic
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Andy Lutomirski]

On Thu, May 3, 2018 at 3:31 PM Luis R. Rodriguez <mcgrof@kernel.org> 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.

Nah, don't use the cryptoapi for this.  You'll probably regret it for any
number of reasons.  My code is here:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=crypto/sha256_bpf&id=e9e12f056f2abed50a30b762db9185799f5864e6

and its two parents.  It needs a little bit of dusting and it needs
checking that all combinations of modular and non-modular builds work.  Ard
probably has further comments.


> 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. It seems unclear if we're
> all happy with this yet as well.

Fair enough.

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Andy Lutomirski]

On Thu, May 3, 2018 at 3:31 PM Luis R. Rodriguez <mcgrof@kernel.org> 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.

Nah, don't use the cryptoapi for this.  You'll probably regret it for any
number of reasons.  My code is here:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=crypto/sha256_bpf&id=e9e12f056f2abed50a30b762db9185799f5864e6

and its two parents.  It needs a little bit of dusting and it needs
checking that all combinations of modular and non-modular builds work.  Ard
probably has further comments.


> 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. It seems unclear if we're
> all happy with this yet as well.

Fair enough.

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Andy Lutomirski]

On Thu, May 3, 2018 at 3:31 PM Luis R. Rodriguez <mcgrof@kernel.org> 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.

Nah, don't use the cryptoapi for this.  You'll probably regret it for any
number of reasons.  My code is here:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=crypto/sha256_bpf&id=e9e12f056f2abed50a30b762db9185799f5864e6

and its two parents.  It needs a little bit of dusting and it needs
checking that all combinations of modular and non-modular builds work.  Ard
probably has further comments.


> 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. It seems unclear if we're
> all happy with this yet as well.

Fair enough.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Mimi Zohar]

On Thu, 2018-05-03 at 22:23 +0000, Luis R. Rodriguez wrote:
> On Tue, May 01, 2018 at 03:27:27PM -0400, Mimi Zohar wrote:
> > On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote:
> > > Only the pre hook?  I believe the post-hook should still be called too,
> > > right? So that we've hashes of all loaded firmwares in the IMA core.
> > 
> > Good catch!  Right, if IMA-measurement is enabled, then we would want
> > to add the measurement.
> 
> Mimi, just a heads up, we only use the post hook for the syfs fallback
> mechanism, ie, we don't even use the post hook for direct fs lookup.
> Do we want that there?

The direct fs lookup calls kernel_read_file_from_path(), which calls
the security_kernel_read_file() and security_kernel_post_read_file()
hooks.  So there is no need to add a direct call to either of these
security calls.

Mimi



  

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Mimi Zohar]

On Thu, 2018-05-03 at 22:23 +0000, Luis R. Rodriguez wrote:
> On Tue, May 01, 2018 at 03:27:27PM -0400, Mimi Zohar wrote:
> > On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote:
> > > Only the pre hook?  I believe the post-hook should still be called too,
> > > right? So that we've hashes of all loaded firmwares in the IMA core.
> > 
> > Good catch!  Right, if IMA-measurement is enabled, then we would want
> > to add the measurement.
> 
> Mimi, just a heads up, we only use the post hook for the syfs fallback
> mechanism, ie, we don't even use the post hook for direct fs lookup.
> Do we want that there?

The direct fs lookup calls kernel_read_file_from_path(), which calls
the security_kernel_read_file() and security_kernel_post_read_file()
hooks.  So there is no need to add a direct call to either of these
security calls.

Mimi



  

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Mimi Zohar]

On Thu, 2018-05-03 at 22:23 +0000, Luis R. Rodriguez wrote:
> On Tue, May 01, 2018 at 03:27:27PM -0400, Mimi Zohar wrote:
> > On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote:
> > > Only the pre hook?  I believe the post-hook should still be called too,
> > > right? So that we've hashes of all loaded firmwares in the IMA core.
> > 
> > Good catch! ?Right, if IMA-measurement is enabled, then we would want
> > to add the measurement.
> 
> Mimi, just a heads up, we only use the post hook for the syfs fallback
> mechanism, ie, we don't even use the post hook for direct fs lookup.
> Do we want that there?

The direct fs lookup calls kernel_read_file_from_path(), which calls
the security_kernel_read_file() and security_kernel_post_read_file()
hooks. ?So there is no need to add a direct call to either of these
security calls.

Mimi



??

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

Please Cc andresx7@gmail.com  on future patches.

On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede 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.

Interesting. Mimi you may want to look into that for appraisal gap coverage.

> 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:

This and other reasons have been scattered through mailing lists, and
documentation, our Kconfig does not reflect which of the reasons are
currently addressed as how. Our goal for your commit log would be to
document the precise reason you can't use any of the existing interfaces
exposed. Let's see if some of the reasons you state cannot be used
with the existing APIs.

> 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.

In such cases the firmware may be stashed in a separate partition of a custom
device and firmwared could be used with a uevent and the fallback mechanism,
enabling userspace to do whatever it needs.

However in your case the firmware has been found on EFI.

> 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.

Indeed, we don't have a good way to deal with this now except the above
noted firmwared use case, and that still ponies up the redistribution problem
up to a system integrator somehow.

But also -- your solution is super custom to only two drivers, does not
follow a standard of any sort either, and I fear of setting any wrong
precedents which will be hard to later not support.

Just look at the fallback mechanism and how hairy it got, I've put some
love into it recently but it was a pain to even comprehend. Once we add
this interface we are going to have to support it for a while so I want
to be sure we get the architecture right. Specially if were going to enable
*other* vendors to start using such interface.

Is your goal to enable or encourage other vendors in similar predicament to
use the same strategy?

There is nothing wrong in setting de-facto standards for Linux, we do this
all the time, but if we are going to do it I want to be sure we document
this well and provide proper justifications in the commit log and
documentation.

> This commit adds support for finding peripheral firmware embedded in the
> EFI code 

Please specify this is through a custom scheme.... as it reads now it seems
to read this is a sort of standard.

> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
> index c8bddbdcfd10..560dfed76e38 100644
> --- a/Documentation/driver-api/firmware/request_firmware.rst
> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns non-zero and fw_entry
>  is set to NULL. Once your driver is done with processing the firmware it
>  can call call firmware_release(fw_entry) to release the firmware image
>  and any related resource.
> +
> +EFI embedded firmware support
> +=============================

This is a new fallback mechanism, please see:

Documentation/driver-api/firmware/fallback-mechanisms.rst

Refer to the section "Types of fallback mechanisms", augument the list there
and then move the section "Firmware sysfs loading facility" to a new file, and
then add a new file for your own.

> +
> +On some devices the system's EFI code / ROM may contain an embedded copy
> +of firmware for some of the system's integrated peripheral devices and
> +the peripheral's Linux device-driver needs to access this firmware.

You in no way indicate this is a just an invented scheme, a custom solution and
nothing standard.  I realize Ard criticized that the EFI Firmware Volume Protocol
is not part of the UEFI spec -- however it is a bit more widely used right?
Why can't Linux support it instead?

Could your new scheme grow to support the EFI Firmware Volume Protocol rather
easily if someone wants to wrap up their sleeves? If so what likely would change?

Ensure that the documentation answers the question then, when would folks
use the EFI Firmware Volume Protocol? Why can't Linux support it? Why and would
use this alternative scheme?

Granted, we seem to have reverse engineered it, but we can certainly improve
upon it, and if we're going to make a new de-facto standard for Linux best
we cover our bases for our justifications.

> +
> +A device driver which needs this can describe the firmware it needs
> +using an efi_embedded_fw_desc struct:
> +
> +.. kernel-doc:: include/linux/efi_embedded_fw.h
> +   :functions: efi_embedded_fw_desc
> +
> +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.
> +
> +To avoid doing this somewhat expensive scan on all systems,

Well we should also only enable this for systems that enable these drivers
*and* enable this kernel feature. Ie, if the drivers are enabled but
the feature is disabled nothing new should happen.

I'd like this feature disabled by default for now.

>  dmi matching is
> +used. Drivers are expected to export a dmi_system_id array, with each entries'
> +driver_data pointing to an efi_embedded_fw_desc.
> +
> +To register this array with the efi-embedded-fw code, a driver needs to:
> +
> +1. Always be builtin to the kernel or store the dmi_system_id array in a
> +   separate object file which always gets builtin.
> +
> +2. Add an extern declaration for the dmi_system_id array to
> +   include/linux/efi_embedded_fw.h.
> +
> +3. Add the dmi_system_id array to the embedded_fw_table in
> +   drivers/firmware/efi/embedded-firmware.c wrapped in a #ifdef testing that
> +   the driver is being builtin.
> +
> +4. Add "select EFI_EMBEDDED_FIRMWARE if EFI_STUB" to its Kconfig entry.
> +
> +The request_firmware() function will always first try to load firmware with
> +the specified name directly from the disk, so the EFI embedded-fw can always
> +be overridden by placing a file under /lib/firmare.
> +
> +To make request_firmware() fallback to trying 

This sounds funny, best to just start off describing that this is just a new
fallback mechanism.

> EFI embedded firmwares after this,
> +the driver must set a boolean "efi-embedded-firmware" device-property on the
> +device before passing it to request_firmware()

Mention that this is for struct platform_device's and what you'd use is
PROPERTY_ENTRY_BOOL(). Provide an example.

Anyway, I'm in no way a fan of this being the only requirement. It makes
no sense to call into EFI or check a device property if the driver didn't
even  mean it to happen.

As I have been asking for a while now, please also add a new
firmware_request_efi() which if called would enable a new internal
firmware_loader flag FW_OPT_EFI which you'd kdoc'ify and add to
drivers/base/firmware_loader/firmware.h.
Then only if *this* internal flag is set would we call fw_get_efi_embedded_fw()
(I'm also asking you to rename this to firmware_fallback_efi() below).

> . Note that this disables the
> +usual usermodehelper fallback, so you may want to only set this on systems
> +which match your dmi_system_id array.

You'll need both the device property and for the driver to use this new
firmware_request_efi() call.

> +
> +Once the device-property is set, the driver can use the regular
> +request_firmware() function to get the firmware, using the name filled in
> +in the efi_embedded_fw_desc.

And nuke this.

> +
> +Note that:
> +
> +1. The code scanning for EFI embbedded-firmware runs near the end
> +   of start_kernel(), just before calling rest_init(). For normal drivers and
> +   subsystems using subsys_initcall() to register themselves this does not
> +   matter. This means that code running earlier cannot use EFI
> +   embbedded-firmware.
> +
> +2. ATM the EFI embedded-fw code assumes that firmwares always start at an offset
> +   which is a multiple of 8 bytes, if this is not true for your case send in
> +   a patch to fix this.
> +
> +3. ATM the EFI embedded-fw code only works on x86 because other archs free
> +   EFI_BOOT_SERVICES_CODE before the EFI embedded-fw code gets a chance to
> +   scan it.
> diff --git a/drivers/base/firmware_loader/Makefile b/drivers/base/firmware_loader/Makefile
> index a97eeb0be1d8..365a040995d3 100644
> --- a/drivers/base/firmware_loader/Makefile
> +++ b/drivers/base/firmware_loader/Makefile
> @@ -5,3 +5,4 @@ obj-y			:= fallback_table.o
>  obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
>  firmware_class-objs := main.o
>  firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o
> +firmware_class-$(CONFIG_EFI_EMBEDDED_FIRMWARE) += fallback_efi.o

Please add a CONFIG_FW_LOADER_FALLBACK_EFI which will be properly
documented on a Kconfig which then if selected would select
CONFIG_EFI_EMBEDDED_FIRMWARE which enables the efi_check_for_embedded_firmwares().
Note you'd be expanding CONFIG_FW_LOADER_FALLBACK_EFI with the new call too.


> diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
> index 8cfaa3299bb7..92f462415d25 100644
> --- a/drivers/base/firmware_loader/fallback.h
> +++ b/drivers/base/firmware_loader/fallback.h
> @@ -66,4 +66,16 @@ static inline void unregister_sysfs_loader(void)
>  }
>  #endif /* CONFIG_FW_LOADER_USER_HELPER */
>  
> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
> +			   enum fw_opt *opt_flags, int ret);
> +#else
> +static inline int fw_get_efi_embedded_fw(struct device *dev,
> +					 struct fw_priv *fw_priv,
> +					 enum fw_opt *opt_flags, int ret)
> +{
> +	return ret;
> +}
> +#endif /* CONFIG_EFI_EMBEDDED_FIRMWARE */
> +
>  #endif /* __FIRMWARE_FALLBACK_H */
> diff --git a/drivers/base/firmware_loader/fallback_efi.c b/drivers/base/firmware_loader/fallback_efi.c
> new file mode 100644
> index 000000000000..82ba82f48a79
> --- /dev/null
> +++ b/drivers/base/firmware_loader/fallback_efi.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/efi_embedded_fw.h>
> +#include <linux/property.h>
> +#include <linux/security.h>
> +#include <linux/vmalloc.h>
> +
> +#include "fallback.h"
> +#include "firmware.h"
> +
> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
> +			   enum fw_opt *opt_flags, int ret)
> +{

Please rename to firmware_fallback_efi() and kdocify it above. I'll soon post a
patch which does the same for firmware_fallback_sysfs(). Andres's
pending patch renames fw_sysfs_fallback() to firmware_sysfs_fallback()
but in retrospect it should be firmware_fallback_sysfs() -- I'll fix this.

> +	enum kernel_read_file_id id = READING_FIRMWARE;
> +	size_t size, max = INT_MAX;
> +	int rc;
> +
> +	if (!dev)
> +		return ret;
> +
> +	if (!device_property_read_bool(dev, "efi-embedded-firmware"))
> +		return ret;
> +
> +	*opt_flags |= FW_OPT_NO_WARN | FW_OPT_NOCACHE | FW_OPT_NOFALLBACK;
> +
> +	/* Already populated data member means we're loading into a buffer */
> +	if (fw_priv->data) {
> +		id = READING_FIRMWARE_PREALLOC_BUFFER;
> +		max = fw_priv->allocated_size;
> +	}
> +
> +	rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size, max);
> +	if (rc) {
> +		dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name);
> +		return ret;
> +	}
> +
> +	rc = security_kernel_post_read_file(NULL, fw_priv->data, size, id);
> +	if (rc) {
> +		if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> +			vfree(fw_priv->data);
> +			fw_priv->data = NULL;
> +		}
> +		return rc;
> +	}
> +
> +	dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
> +	fw_priv->size = size;
> +	fw_state_done(fw_priv);
> +	return 0;
> +}
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index f009566acd35..23c5392eb59e 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -576,6 +576,8 @@ _firmware_request(const struct firmware **firmware_p, const char *name,
>  		goto out;
>  
>  	ret = fw_get_filesystem_firmware(device, fw->priv);
> +	if (ret)
> +		ret = fw_get_efi_embedded_fw(device, fw->priv, &opt_flags, ret);

This is going to get complex. If you can come up with a nicer scheme for dealing
with fallback that'd be appreciated. If you this is the most legible you can
come up with I understand though, I can have a look later.

  Luis

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

Please Cc andresx7@gmail.com  on future patches.

On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede 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.

Interesting. Mimi you may want to look into that for appraisal gap coverage.

> 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:

This and other reasons have been scattered through mailing lists, and
documentation, our Kconfig does not reflect which of the reasons are
currently addressed as how. Our goal for your commit log would be to
document the precise reason you can't use any of the existing interfaces
exposed. Let's see if some of the reasons you state cannot be used
with the existing APIs.

> 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.

In such cases the firmware may be stashed in a separate partition of a custom
device and firmwared could be used with a uevent and the fallback mechanism,
enabling userspace to do whatever it needs.

However in your case the firmware has been found on EFI.

> 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.

Indeed, we don't have a good way to deal with this now except the above
noted firmwared use case, and that still ponies up the redistribution problem
up to a system integrator somehow.

But also -- your solution is super custom to only two drivers, does not
follow a standard of any sort either, and I fear of setting any wrong
precedents which will be hard to later not support.

Just look at the fallback mechanism and how hairy it got, I've put some
love into it recently but it was a pain to even comprehend. Once we add
this interface we are going to have to support it for a while so I want
to be sure we get the architecture right. Specially if were going to enable
*other* vendors to start using such interface.

Is your goal to enable or encourage other vendors in similar predicament to
use the same strategy?

There is nothing wrong in setting de-facto standards for Linux, we do this
all the time, but if we are going to do it I want to be sure we document
this well and provide proper justifications in the commit log and
documentation.

> This commit adds support for finding peripheral firmware embedded in the
> EFI code 

Please specify this is through a custom scheme.... as it reads now it seems
to read this is a sort of standard.

> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
> index c8bddbdcfd10..560dfed76e38 100644
> --- a/Documentation/driver-api/firmware/request_firmware.rst
> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns non-zero and fw_entry
>  is set to NULL. Once your driver is done with processing the firmware it
>  can call call firmware_release(fw_entry) to release the firmware image
>  and any related resource.
> +
> +EFI embedded firmware support
> +=============================

This is a new fallback mechanism, please see:

Documentation/driver-api/firmware/fallback-mechanisms.rst

Refer to the section "Types of fallback mechanisms", augument the list there
and then move the section "Firmware sysfs loading facility" to a new file, and
then add a new file for your own.

> +
> +On some devices the system's EFI code / ROM may contain an embedded copy
> +of firmware for some of the system's integrated peripheral devices and
> +the peripheral's Linux device-driver needs to access this firmware.

You in no way indicate this is a just an invented scheme, a custom solution and
nothing standard.  I realize Ard criticized that the EFI Firmware Volume Protocol
is not part of the UEFI spec -- however it is a bit more widely used right?
Why can't Linux support it instead?

Could your new scheme grow to support the EFI Firmware Volume Protocol rather
easily if someone wants to wrap up their sleeves? If so what likely would change?

Ensure that the documentation answers the question then, when would folks
use the EFI Firmware Volume Protocol? Why can't Linux support it? Why and would
use this alternative scheme?

Granted, we seem to have reverse engineered it, but we can certainly improve
upon it, and if we're going to make a new de-facto standard for Linux best
we cover our bases for our justifications.

> +
> +A device driver which needs this can describe the firmware it needs
> +using an efi_embedded_fw_desc struct:
> +
> +.. kernel-doc:: include/linux/efi_embedded_fw.h
> +   :functions: efi_embedded_fw_desc
> +
> +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.
> +
> +To avoid doing this somewhat expensive scan on all systems,

Well we should also only enable this for systems that enable these drivers
*and* enable this kernel feature. Ie, if the drivers are enabled but
the feature is disabled nothing new should happen.

I'd like this feature disabled by default for now.

>  dmi matching is
> +used. Drivers are expected to export a dmi_system_id array, with each entries'
> +driver_data pointing to an efi_embedded_fw_desc.
> +
> +To register this array with the efi-embedded-fw code, a driver needs to:
> +
> +1. Always be builtin to the kernel or store the dmi_system_id array in a
> +   separate object file which always gets builtin.
> +
> +2. Add an extern declaration for the dmi_system_id array to
> +   include/linux/efi_embedded_fw.h.
> +
> +3. Add the dmi_system_id array to the embedded_fw_table in
> +   drivers/firmware/efi/embedded-firmware.c wrapped in a #ifdef testing that
> +   the driver is being builtin.
> +
> +4. Add "select EFI_EMBEDDED_FIRMWARE if EFI_STUB" to its Kconfig entry.
> +
> +The request_firmware() function will always first try to load firmware with
> +the specified name directly from the disk, so the EFI embedded-fw can always
> +be overridden by placing a file under /lib/firmare.
> +
> +To make request_firmware() fallback to trying 

This sounds funny, best to just start off describing that this is just a new
fallback mechanism.

> EFI embedded firmwares after this,
> +the driver must set a boolean "efi-embedded-firmware" device-property on the
> +device before passing it to request_firmware()

Mention that this is for struct platform_device's and what you'd use is
PROPERTY_ENTRY_BOOL(). Provide an example.

Anyway, I'm in no way a fan of this being the only requirement. It makes
no sense to call into EFI or check a device property if the driver didn't
even  mean it to happen.

As I have been asking for a while now, please also add a new
firmware_request_efi() which if called would enable a new internal
firmware_loader flag FW_OPT_EFI which you'd kdoc'ify and add to
drivers/base/firmware_loader/firmware.h.
Then only if *this* internal flag is set would we call fw_get_efi_embedded_fw()
(I'm also asking you to rename this to firmware_fallback_efi() below).

> . Note that this disables the
> +usual usermodehelper fallback, so you may want to only set this on systems
> +which match your dmi_system_id array.

You'll need both the device property and for the driver to use this new
firmware_request_efi() call.

> +
> +Once the device-property is set, the driver can use the regular
> +request_firmware() function to get the firmware, using the name filled in
> +in the efi_embedded_fw_desc.

And nuke this.

> +
> +Note that:
> +
> +1. The code scanning for EFI embbedded-firmware runs near the end
> +   of start_kernel(), just before calling rest_init(). For normal drivers and
> +   subsystems using subsys_initcall() to register themselves this does not
> +   matter. This means that code running earlier cannot use EFI
> +   embbedded-firmware.
> +
> +2. ATM the EFI embedded-fw code assumes that firmwares always start at an offset
> +   which is a multiple of 8 bytes, if this is not true for your case send in
> +   a patch to fix this.
> +
> +3. ATM the EFI embedded-fw code only works on x86 because other archs free
> +   EFI_BOOT_SERVICES_CODE before the EFI embedded-fw code gets a chance to
> +   scan it.
> diff --git a/drivers/base/firmware_loader/Makefile b/drivers/base/firmware_loader/Makefile
> index a97eeb0be1d8..365a040995d3 100644
> --- a/drivers/base/firmware_loader/Makefile
> +++ b/drivers/base/firmware_loader/Makefile
> @@ -5,3 +5,4 @@ obj-y			:= fallback_table.o
>  obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
>  firmware_class-objs := main.o
>  firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o
> +firmware_class-$(CONFIG_EFI_EMBEDDED_FIRMWARE) += fallback_efi.o

Please add a CONFIG_FW_LOADER_FALLBACK_EFI which will be properly
documented on a Kconfig which then if selected would select
CONFIG_EFI_EMBEDDED_FIRMWARE which enables the efi_check_for_embedded_firmwares().
Note you'd be expanding CONFIG_FW_LOADER_FALLBACK_EFI with the new call too.


> diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
> index 8cfaa3299bb7..92f462415d25 100644
> --- a/drivers/base/firmware_loader/fallback.h
> +++ b/drivers/base/firmware_loader/fallback.h
> @@ -66,4 +66,16 @@ static inline void unregister_sysfs_loader(void)
>  }
>  #endif /* CONFIG_FW_LOADER_USER_HELPER */
>  
> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
> +			   enum fw_opt *opt_flags, int ret);
> +#else
> +static inline int fw_get_efi_embedded_fw(struct device *dev,
> +					 struct fw_priv *fw_priv,
> +					 enum fw_opt *opt_flags, int ret)
> +{
> +	return ret;
> +}
> +#endif /* CONFIG_EFI_EMBEDDED_FIRMWARE */
> +
>  #endif /* __FIRMWARE_FALLBACK_H */
> diff --git a/drivers/base/firmware_loader/fallback_efi.c b/drivers/base/firmware_loader/fallback_efi.c
> new file mode 100644
> index 000000000000..82ba82f48a79
> --- /dev/null
> +++ b/drivers/base/firmware_loader/fallback_efi.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/efi_embedded_fw.h>
> +#include <linux/property.h>
> +#include <linux/security.h>
> +#include <linux/vmalloc.h>
> +
> +#include "fallback.h"
> +#include "firmware.h"
> +
> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
> +			   enum fw_opt *opt_flags, int ret)
> +{

Please rename to firmware_fallback_efi() and kdocify it above. I'll soon post a
patch which does the same for firmware_fallback_sysfs(). Andres's
pending patch renames fw_sysfs_fallback() to firmware_sysfs_fallback()
but in retrospect it should be firmware_fallback_sysfs() -- I'll fix this.

> +	enum kernel_read_file_id id = READING_FIRMWARE;
> +	size_t size, max = INT_MAX;
> +	int rc;
> +
> +	if (!dev)
> +		return ret;
> +
> +	if (!device_property_read_bool(dev, "efi-embedded-firmware"))
> +		return ret;
> +
> +	*opt_flags |= FW_OPT_NO_WARN | FW_OPT_NOCACHE | FW_OPT_NOFALLBACK;
> +
> +	/* Already populated data member means we're loading into a buffer */
> +	if (fw_priv->data) {
> +		id = READING_FIRMWARE_PREALLOC_BUFFER;
> +		max = fw_priv->allocated_size;
> +	}
> +
> +	rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size, max);
> +	if (rc) {
> +		dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name);
> +		return ret;
> +	}
> +
> +	rc = security_kernel_post_read_file(NULL, fw_priv->data, size, id);
> +	if (rc) {
> +		if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> +			vfree(fw_priv->data);
> +			fw_priv->data = NULL;
> +		}
> +		return rc;
> +	}
> +
> +	dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
> +	fw_priv->size = size;
> +	fw_state_done(fw_priv);
> +	return 0;
> +}
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index f009566acd35..23c5392eb59e 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -576,6 +576,8 @@ _firmware_request(const struct firmware **firmware_p, const char *name,
>  		goto out;
>  
>  	ret = fw_get_filesystem_firmware(device, fw->priv);
> +	if (ret)
> +		ret = fw_get_efi_embedded_fw(device, fw->priv, &opt_flags, ret);

This is going to get complex. If you can come up with a nicer scheme for dealing
with fallback that'd be appreciated. If you this is the most legible you can
come up with I understand though, I can have a look later.

  Luis

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Ard Biesheuvel]

On 4 May 2018 at 01:29, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede wrote:
[...]
>> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
>> index c8bddbdcfd10..560dfed76e38 100644
>> --- a/Documentation/driver-api/firmware/request_firmware.rst
>> +++ b/Documentation/driver-api/firmware/request_firmware.rst
>> @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns non-zero and fw_entry
>>  is set to NULL. Once your driver is done with processing the firmware it
>>  can call call firmware_release(fw_entry) to release the firmware image
>>  and any related resource.
>> +
>> +EFI embedded firmware support
>> +=============================
>
> This is a new fallback mechanism, please see:
>
> Documentation/driver-api/firmware/fallback-mechanisms.rst
>
> Refer to the section "Types of fallback mechanisms", augument the list there
> and then move the section "Firmware sysfs loading facility" to a new file, and
> then add a new file for your own.
>
>> +
>> +On some devices the system's EFI code / ROM may contain an embedded copy
>> +of firmware for some of the system's integrated peripheral devices and
>> +the peripheral's Linux device-driver needs to access this firmware.
>
> You in no way indicate this is a just an invented scheme, a custom solution and
> nothing standard.  I realize Ard criticized that the EFI Firmware Volume Protocol
> is not part of the UEFI spec -- however it is a bit more widely used right?
> Why can't Linux support it instead?
>

Most implementations of UEFI are based on PI, and so it is likely that
the protocols are available. However, the PI spec does not cover
firmware blobs, and so it is undefined whether such blobs are self
contained (i.e., in separate files in the firmware volume), statically
linked into the driver or maybe even encrypted or otherwise
encapsulated, and the actual loadable image only lives in memory.

Hans's case is the second one, i.e., the firmware is at an arbitrary
offset in the driver image. Using the FV protocol in this case would
result in a mix of both approaches: look up the driver file by GUID
[which could change btw between different versions of the system
firmware, although this is unlikely] and then still use the prefix/crc
based approach to sift through the image itself.

But my main objection is simply that from the UEFI forum point of
view, there is a clear distinction between the OS visible interfaces
in the UEFI spec and the internal interfaces in the PI spec (which for
instance are not subject to the same rules when it comes to backward
compatibility), and so I think we should not depend on PI at all. This
is all the more important considering that we are trying to encourage
the creation of other implementations of UEFI that are not based on PI
(e.g., uboot for arm64 implements the required UEFI interfaces for
booting the kernel via GRUB), and adding dependencies on PI protocols
makes that a moving target.

So in my view, we either take a ad-hoc approach which works for the
few platforms we expect to support, in which case Hans's approach is
sufficient, or we architect it properly, in which case we shouldn't
depend on PI because it does not belong in a properly architected
OS<->firmware exchange.

-- 
Ard.

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Ard Biesheuvel]

On 4 May 2018 at 01:29, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede wrote:
[...]
>> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
>> index c8bddbdcfd10..560dfed76e38 100644
>> --- a/Documentation/driver-api/firmware/request_firmware.rst
>> +++ b/Documentation/driver-api/firmware/request_firmware.rst
>> @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns non-zero and fw_entry
>>  is set to NULL. Once your driver is done with processing the firmware it
>>  can call call firmware_release(fw_entry) to release the firmware image
>>  and any related resource.
>> +
>> +EFI embedded firmware support
>> +=============================
>
> This is a new fallback mechanism, please see:
>
> Documentation/driver-api/firmware/fallback-mechanisms.rst
>
> Refer to the section "Types of fallback mechanisms", augument the list there
> and then move the section "Firmware sysfs loading facility" to a new file, and
> then add a new file for your own.
>
>> +
>> +On some devices the system's EFI code / ROM may contain an embedded copy
>> +of firmware for some of the system's integrated peripheral devices and
>> +the peripheral's Linux device-driver needs to access this firmware.
>
> You in no way indicate this is a just an invented scheme, a custom solution and
> nothing standard.  I realize Ard criticized that the EFI Firmware Volume Protocol
> is not part of the UEFI spec -- however it is a bit more widely used right?
> Why can't Linux support it instead?
>

Most implementations of UEFI are based on PI, and so it is likely that
the protocols are available. However, the PI spec does not cover
firmware blobs, and so it is undefined whether such blobs are self
contained (i.e., in separate files in the firmware volume), statically
linked into the driver or maybe even encrypted or otherwise
encapsulated, and the actual loadable image only lives in memory.

Hans's case is the second one, i.e., the firmware is at an arbitrary
offset in the driver image. Using the FV protocol in this case would
result in a mix of both approaches: look up the driver file by GUID
[which could change btw between different versions of the system
firmware, although this is unlikely] and then still use the prefix/crc
based approach to sift through the image itself.

But my main objection is simply that from the UEFI forum point of
view, there is a clear distinction between the OS visible interfaces
in the UEFI spec and the internal interfaces in the PI spec (which for
instance are not subject to the same rules when it comes to backward
compatibility), and so I think we should not depend on PI at all. This
is all the more important considering that we are trying to encourage
the creation of other implementations of UEFI that are not based on PI
(e.g., uboot for arm64 implements the required UEFI interfaces for
booting the kernel via GRUB), and adding dependencies on PI protocols
makes that a moving target.

So in my view, we either take a ad-hoc approach which works for the
few platforms we expect to support, in which case Hans's approach is
sufficient, or we architect it properly, in which case we shouldn't
depend on PI because it does not belong in a properly architected
OS<->firmware exchange.

-- 
Ard.

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Ard Biesheuvel]

Hi Hans,

One comment below, which I missed in review before.

On 29 April 2018 at 11:35, 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 making this available to peripheral drivers through the
> standard firmware loading mechanism.
>
> 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.
>
> Reported-by: Dave Olsthoorn <dave@bewaar.me>
> Suggested-by: Peter Jones <pjones@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
[...]
> diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
> new file mode 100644
> index 000000000000..22a0f598b53d
> --- /dev/null
> +++ b/drivers/firmware/efi/embedded-firmware.c
> @@ -0,0 +1,149 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Support for extracting embedded firmware for peripherals from EFI code,
> + *
> + * Copyright (c) 2018 Hans de Goede <hdegoede@redhat.com>
> + */
> +
> +#include <linux/crc32.h>
> +#include <linux/dmi.h>
> +#include <linux/efi.h>
> +#include <linux/efi_embedded_fw.h>
> +#include <linux/io.h>
> +#include <linux/types.h>
> +#include <linux/vmalloc.h>
> +
> +struct embedded_fw {
> +       struct list_head list;
> +       const char *name;
> +       void *data;
> +       size_t length;
> +};
> +
> +static LIST_HEAD(found_fw_list);
> +
> +static const struct dmi_system_id * const embedded_fw_table[] = {
> +       NULL
> +};
> +
> +/*
> + * Note the efi_check_for_embedded_firmwares() code currently makes the
> + * following 2 assumptions. This may needs to be revisited if embedded firmware
> + * is found where this is not true:
> + * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory segments
> + * 2) The firmware always starts at an offset which is a multiple of 8 bytes
> + */
> +static int __init efi_check_md_for_embedded_firmware(
> +       efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
> +{
> +       struct embedded_fw *fw;
> +       u64 i, size;
> +       u32 crc;
> +       u8 *mem;
> +
> +       size = md->num_pages << EFI_PAGE_SHIFT;
> +       mem = memremap(md->phys_addr, size, MEMREMAP_WB);
> +       if (!mem) {
> +               pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
> +               return -ENOMEM;
> +       }
> +
> +       size -= desc->length;
> +       for (i = 0; i < size; i += 8) {
> +               if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
> +                       continue;
> +

Please use the proper APIs here to cast u8* to u64*, i.e., either use
get_unaligned64() or use memcmp()

> +               /* Seed with ~0, invert to match crc32 userspace utility */
> +               crc = ~crc32(~0, mem + i, desc->length);
> +               if (crc == desc->crc)
> +                       break;
> +       }
> +
> +       memunmap(mem);
> +
> +       if (i >= size)
> +               return -ENOENT;
> +
> +       pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name, desc->crc);
> +
> +       fw = kmalloc(sizeof(*fw), GFP_KERNEL);
> +       if (!fw)
> +               return -ENOMEM;
> +
> +       mem = memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
> +       if (!mem) {
> +               pr_err("Error mapping embedded firmware\n");
> +               goto error_free_fw;
> +       }
> +       fw->data = kmemdup(mem, desc->length, GFP_KERNEL);
> +       memunmap(mem);
> +       if (!fw->data)
> +               goto error_free_fw;
> +
> +       fw->name = desc->name;
> +       fw->length = desc->length;
> +       list_add(&fw->list, &found_fw_list);
> +
> +       return 0;
> +
> +error_free_fw:
> +       kfree(fw);
> +       return -ENOMEM;
> +}
> +
> +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;
> +               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;
> +               }
> +       }
> +}
> +
> +int efi_get_embedded_fw(const char *name, void **data, size_t *size,
> +                       size_t msize)
> +{
> +       struct embedded_fw *iter, *fw = NULL;
> +       void *buf = *data;
> +
> +       list_for_each_entry(iter, &found_fw_list, list) {
> +               if (strcmp(name, iter->name) == 0) {
> +                       fw = iter;
> +                       break;
> +               }
> +       }
> +
> +       if (!fw)
> +               return -ENOENT;
> +
> +       if (msize && msize < fw->length)
> +               return -EFBIG;
> +
> +       if (!buf) {
> +               buf = vmalloc(fw->length);
> +               if (!buf)
> +                       return -ENOMEM;
> +       }
> +
> +       memcpy(buf, fw->data, fw->length);
> +       *size = fw->length;
> +       *data = buf;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(efi_get_embedded_fw);
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 791088360c1e..23e8a9c26ce2 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1575,6 +1575,12 @@ static inline void
>  efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
>  #endif
>
> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
> +void efi_check_for_embedded_firmwares(void);
> +#else
> +static inline void efi_check_for_embedded_firmwares(void) { }
> +#endif
> +
>  void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
>
>  /*
> diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
> new file mode 100644
> index 000000000000..0f7d4df3f57a
> --- /dev/null
> +++ b/include/linux/efi_embedded_fw.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_EFI_EMBEDDED_FW_H
> +#define _LINUX_EFI_EMBEDDED_FW_H
> +
> +#include <linux/mod_devicetable.h>
> +
> +/**
> + * struct efi_embedded_fw_desc - This struct is used by the EFI embedded-fw
> + *                               code to search for embedded firmwares.
> + *
> + * @name:   Name to register the firmware with if found
> + * @prefix: First 8 bytes of the firmware
> + * @length: Length of the firmware in bytes including prefix
> + * @crc:    Inverted little endian Ethernet style CRC32, with 0xffffffff seed
> + */
> +struct efi_embedded_fw_desc {
> +       const char *name;
> +       u8 prefix[8];
> +       u32 length;
> +       u32 crc;
> +};
> +
> +int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t msize);
> +
> +#endif
> diff --git a/init/main.c b/init/main.c
> index b795aa341a3a..ab29775b35db 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -729,6 +729,9 @@ asmlinkage __visible void __init start_kernel(void)
>         arch_post_acpi_subsys_init();
>         sfi_init_late();
>
> +       if (efi_enabled(EFI_PRESERVE_BS_REGIONS))
> +               efi_check_for_embedded_firmwares();
> +
>         if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>                 efi_free_boot_services();
>         }
> --
> 2.17.0
>

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Ard Biesheuvel]

Hi Hans,

One comment below, which I missed in review before.

On 29 April 2018 at 11:35, 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 making this available to peripheral drivers through the
> standard firmware loading mechanism.
>
> 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.
>
> Reported-by: Dave Olsthoorn <dave@bewaar.me>
> Suggested-by: Peter Jones <pjones@redhat.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
[...]
> diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
> new file mode 100644
> index 000000000000..22a0f598b53d
> --- /dev/null
> +++ b/drivers/firmware/efi/embedded-firmware.c
> @@ -0,0 +1,149 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Support for extracting embedded firmware for peripherals from EFI code,
> + *
> + * Copyright (c) 2018 Hans de Goede <hdegoede@redhat.com>
> + */
> +
> +#include <linux/crc32.h>
> +#include <linux/dmi.h>
> +#include <linux/efi.h>
> +#include <linux/efi_embedded_fw.h>
> +#include <linux/io.h>
> +#include <linux/types.h>
> +#include <linux/vmalloc.h>
> +
> +struct embedded_fw {
> +       struct list_head list;
> +       const char *name;
> +       void *data;
> +       size_t length;
> +};
> +
> +static LIST_HEAD(found_fw_list);
> +
> +static const struct dmi_system_id * const embedded_fw_table[] = {
> +       NULL
> +};
> +
> +/*
> + * Note the efi_check_for_embedded_firmwares() code currently makes the
> + * following 2 assumptions. This may needs to be revisited if embedded firmware
> + * is found where this is not true:
> + * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory segments
> + * 2) The firmware always starts at an offset which is a multiple of 8 bytes
> + */
> +static int __init efi_check_md_for_embedded_firmware(
> +       efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
> +{
> +       struct embedded_fw *fw;
> +       u64 i, size;
> +       u32 crc;
> +       u8 *mem;
> +
> +       size = md->num_pages << EFI_PAGE_SHIFT;
> +       mem = memremap(md->phys_addr, size, MEMREMAP_WB);
> +       if (!mem) {
> +               pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
> +               return -ENOMEM;
> +       }
> +
> +       size -= desc->length;
> +       for (i = 0; i < size; i += 8) {
> +               if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
> +                       continue;
> +

Please use the proper APIs here to cast u8* to u64*, i.e., either use
get_unaligned64() or use memcmp()

> +               /* Seed with ~0, invert to match crc32 userspace utility */
> +               crc = ~crc32(~0, mem + i, desc->length);
> +               if (crc == desc->crc)
> +                       break;
> +       }
> +
> +       memunmap(mem);
> +
> +       if (i >= size)
> +               return -ENOENT;
> +
> +       pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name, desc->crc);
> +
> +       fw = kmalloc(sizeof(*fw), GFP_KERNEL);
> +       if (!fw)
> +               return -ENOMEM;
> +
> +       mem = memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
> +       if (!mem) {
> +               pr_err("Error mapping embedded firmware\n");
> +               goto error_free_fw;
> +       }
> +       fw->data = kmemdup(mem, desc->length, GFP_KERNEL);
> +       memunmap(mem);
> +       if (!fw->data)
> +               goto error_free_fw;
> +
> +       fw->name = desc->name;
> +       fw->length = desc->length;
> +       list_add(&fw->list, &found_fw_list);
> +
> +       return 0;
> +
> +error_free_fw:
> +       kfree(fw);
> +       return -ENOMEM;
> +}
> +
> +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;
> +               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;
> +               }
> +       }
> +}
> +
> +int efi_get_embedded_fw(const char *name, void **data, size_t *size,
> +                       size_t msize)
> +{
> +       struct embedded_fw *iter, *fw = NULL;
> +       void *buf = *data;
> +
> +       list_for_each_entry(iter, &found_fw_list, list) {
> +               if (strcmp(name, iter->name) == 0) {
> +                       fw = iter;
> +                       break;
> +               }
> +       }
> +
> +       if (!fw)
> +               return -ENOENT;
> +
> +       if (msize && msize < fw->length)
> +               return -EFBIG;
> +
> +       if (!buf) {
> +               buf = vmalloc(fw->length);
> +               if (!buf)
> +                       return -ENOMEM;
> +       }
> +
> +       memcpy(buf, fw->data, fw->length);
> +       *size = fw->length;
> +       *data = buf;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(efi_get_embedded_fw);
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 791088360c1e..23e8a9c26ce2 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1575,6 +1575,12 @@ static inline void
>  efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
>  #endif
>
> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
> +void efi_check_for_embedded_firmwares(void);
> +#else
> +static inline void efi_check_for_embedded_firmwares(void) { }
> +#endif
> +
>  void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
>
>  /*
> diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
> new file mode 100644
> index 000000000000..0f7d4df3f57a
> --- /dev/null
> +++ b/include/linux/efi_embedded_fw.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_EFI_EMBEDDED_FW_H
> +#define _LINUX_EFI_EMBEDDED_FW_H
> +
> +#include <linux/mod_devicetable.h>
> +
> +/**
> + * struct efi_embedded_fw_desc - This struct is used by the EFI embedded-fw
> + *                               code to search for embedded firmwares.
> + *
> + * @name:   Name to register the firmware with if found
> + * @prefix: First 8 bytes of the firmware
> + * @length: Length of the firmware in bytes including prefix
> + * @crc:    Inverted little endian Ethernet style CRC32, with 0xffffffff seed
> + */
> +struct efi_embedded_fw_desc {
> +       const char *name;
> +       u8 prefix[8];
> +       u32 length;
> +       u32 crc;
> +};
> +
> +int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t msize);
> +
> +#endif
> diff --git a/init/main.c b/init/main.c
> index b795aa341a3a..ab29775b35db 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -729,6 +729,9 @@ asmlinkage __visible void __init start_kernel(void)
>         arch_post_acpi_subsys_init();
>         sfi_init_late();
>
> +       if (efi_enabled(EFI_PRESERVE_BS_REGIONS))
> +               efi_check_for_embedded_firmwares();
> +
>         if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>                 efi_free_boot_services();
>         }
> --
> 2.17.0
>

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

On Fri, May 04, 2018 at 07:54:28AM +0200, Ard Biesheuvel wrote:
> On 4 May 2018 at 01:29, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede wrote:
> [...]
> >> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
> >> index c8bddbdcfd10..560dfed76e38 100644
> >> --- a/Documentation/driver-api/firmware/request_firmware.rst
> >> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> >> @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns non-zero and fw_entry
> >>  is set to NULL. Once your driver is done with processing the firmware it
> >>  can call call firmware_release(fw_entry) to release the firmware image
> >>  and any related resource.
> >> +
> >> +EFI embedded firmware support
> >> +=============================
> >
> > This is a new fallback mechanism, please see:
> >
> > Documentation/driver-api/firmware/fallback-mechanisms.rst
> >
> > Refer to the section "Types of fallback mechanisms", augument the list there
> > and then move the section "Firmware sysfs loading facility" to a new file, and
> > then add a new file for your own.
> >
> >> +
> >> +On some devices the system's EFI code / ROM may contain an embedded copy
> >> +of firmware for some of the system's integrated peripheral devices and
> >> +the peripheral's Linux device-driver needs to access this firmware.
> >
> > You in no way indicate this is a just an invented scheme, a custom solution and
> > nothing standard.  I realize Ard criticized that the EFI Firmware Volume Protocol
> > is not part of the UEFI spec -- however it is a bit more widely used right?
> > Why can't Linux support it instead?
> >
> 
> Most implementations of UEFI are based on PI,

That seems to be the UEFI Platform Initialization specification:

http://www.uefi.org/sites/default/files/resources/PI_Spec_1_6.pdf

> and so it is likely that
> the protocols are available. However, the PI spec does not cover
> firmware blobs,

Indeed, I cannot find anything about it on the PI Spec, but I *can* easily
find a few documents referring to the Firmware Volume Protocol:

http://wiki.phoenix.com/wiki/index.php/EFI_FIRMWARE_VOLUME_PROTOCOL

But this has no references at all...

I see stupid patents over some of this and authentication mechanisms for it:

https://patents.google.com/patent/US20170098084

> and so it is undefined whether such blobs are self
> contained (i.e., in separate files in the firmware volume), statically
> linked into the driver or maybe even encrypted or otherwise
> encapsulated, and the actual loadable image only lives in memory.

Got it, thanks this helps! There are two things then:

 1) The "EFI Firmware Volume Protocol" ("FV" for short in your descriptions
    below), and whether to support it or not in the future and recommend it
    for future use cases.

 b) Han's EFI scraper to help support 2 drivers, and whether or not to
    recommend it for future use cases.

> Hans's case is the second one, i.e., the firmware is at an arbitrary
> offset in the driver image. Using the FV protocol in this case would
> result in a mix of both approaches: look up the driver file by GUID
> [which could change btw between different versions of the system
> firmware, although this is unlikely] and then still use the prefix/crc
> based approach to sift through the image itself.

Got it. And to be clear its a reversed engineered solution to what
two vendors decided to do.

> But my main objection is simply that from the UEFI forum point of
> view, there is a clear distinction between the OS visible interfaces
> in the UEFI spec and the internal interfaces in the PI spec (which for
> instance are not subject to the same rules when it comes to backward
> compatibility), and so I think we should not depend on PI at all.

Ah I see.

> This
> is all the more important considering that we are trying to encourage
> the creation of other implementations of UEFI that are not based on PI
> (e.g., uboot for arm64 implements the required UEFI interfaces for
> booting the kernel via GRUB), and adding dependencies on PI protocols
> makes that a moving target.

Got it!

> So in my view, we either take a ad-hoc approach which works for the
> few platforms we expect to support, in which case Hans's approach is
> sufficient, 

Modulo it needs some work for ARM as it only works for x86 right now ;)

> or we architect it properly, in which case we shouldn't
> depend on PI because it does not belong in a properly architected
> OS<->firmware exchange.

OK, it sounds to me like we have room to then implement our own de-facto
standard for letting vendors stuff firmware into EFI as we in the Linux
community see fit.

We can start out by supporting existing drivers, but also consider customizing
this in the future for our own needs, so long as we document it and set
expectations well.

So we need to support what Hans is implementing for two reasons then:

a) The FV Protocol cannot be used to support the two drivers he's
   trying to provide support for -- I believe Hans tried and it didn't work,
   Hans, correct me if I'm wrong?

b) The FV Protocol relies on *internal* interfaces of PI spec, and since:
    1) The PI spec does not define firmware at all
    2) The internal interfaces of PI Spec does not guarantee any backward
       compatibility
   Any implementation details in FV may be subject to change, and may vary
   system to system. Supporting the FV Protocol would be difficult as it
   purposely ambiguous.

If accurate, Hans, can you capture this in your documentation somehow?

And.. we should review Hans' changes in light of not only supporting 2
drivers but also helping advance the framework to properly expand it
in the future as we see fit. And ensuring backward compatibility, should
we change or enhance the interface.

In that case I believe that, other than enhanced documentation justifying
these new changes over supporting the FV protocol, the remaining sticking
points to this patch series was the stronger hashing algorithms used for
which Andy already provided feedback and recommendations for.

  Luis

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Luis R. Rodriguez]

On Fri, May 04, 2018 at 07:54:28AM +0200, Ard Biesheuvel wrote:
> On 4 May 2018 at 01:29, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede wrote:
> [...]
> >> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
> >> index c8bddbdcfd10..560dfed76e38 100644
> >> --- a/Documentation/driver-api/firmware/request_firmware.rst
> >> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> >> @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns non-zero and fw_entry
> >>  is set to NULL. Once your driver is done with processing the firmware it
> >>  can call call firmware_release(fw_entry) to release the firmware image
> >>  and any related resource.
> >> +
> >> +EFI embedded firmware support
> >> +=============================
> >
> > This is a new fallback mechanism, please see:
> >
> > Documentation/driver-api/firmware/fallback-mechanisms.rst
> >
> > Refer to the section "Types of fallback mechanisms", augument the list there
> > and then move the section "Firmware sysfs loading facility" to a new file, and
> > then add a new file for your own.
> >
> >> +
> >> +On some devices the system's EFI code / ROM may contain an embedded copy
> >> +of firmware for some of the system's integrated peripheral devices and
> >> +the peripheral's Linux device-driver needs to access this firmware.
> >
> > You in no way indicate this is a just an invented scheme, a custom solution and
> > nothing standard.  I realize Ard criticized that the EFI Firmware Volume Protocol
> > is not part of the UEFI spec -- however it is a bit more widely used right?
> > Why can't Linux support it instead?
> >
> 
> Most implementations of UEFI are based on PI,

That seems to be the UEFI Platform Initialization specification:

http://www.uefi.org/sites/default/files/resources/PI_Spec_1_6.pdf

> and so it is likely that
> the protocols are available. However, the PI spec does not cover
> firmware blobs,

Indeed, I cannot find anything about it on the PI Spec, but I *can* easily
find a few documents referring to the Firmware Volume Protocol:

http://wiki.phoenix.com/wiki/index.php/EFI_FIRMWARE_VOLUME_PROTOCOL

But this has no references at all...

I see stupid patents over some of this and authentication mechanisms for it:

https://patents.google.com/patent/US20170098084

> and so it is undefined whether such blobs are self
> contained (i.e., in separate files in the firmware volume), statically
> linked into the driver or maybe even encrypted or otherwise
> encapsulated, and the actual loadable image only lives in memory.

Got it, thanks this helps! There are two things then:

 1) The "EFI Firmware Volume Protocol" ("FV" for short in your descriptions
    below), and whether to support it or not in the future and recommend it
    for future use cases.

 b) Han's EFI scraper to help support 2 drivers, and whether or not to
    recommend it for future use cases.

> Hans's case is the second one, i.e., the firmware is at an arbitrary
> offset in the driver image. Using the FV protocol in this case would
> result in a mix of both approaches: look up the driver file by GUID
> [which could change btw between different versions of the system
> firmware, although this is unlikely] and then still use the prefix/crc
> based approach to sift through the image itself.

Got it. And to be clear its a reversed engineered solution to what
two vendors decided to do.

> But my main objection is simply that from the UEFI forum point of
> view, there is a clear distinction between the OS visible interfaces
> in the UEFI spec and the internal interfaces in the PI spec (which for
> instance are not subject to the same rules when it comes to backward
> compatibility), and so I think we should not depend on PI at all.

Ah I see.

> This
> is all the more important considering that we are trying to encourage
> the creation of other implementations of UEFI that are not based on PI
> (e.g., uboot for arm64 implements the required UEFI interfaces for
> booting the kernel via GRUB), and adding dependencies on PI protocols
> makes that a moving target.

Got it!

> So in my view, we either take a ad-hoc approach which works for the
> few platforms we expect to support, in which case Hans's approach is
> sufficient, 

Modulo it needs some work for ARM as it only works for x86 right now ;)

> or we architect it properly, in which case we shouldn't
> depend on PI because it does not belong in a properly architected
> OS<->firmware exchange.

OK, it sounds to me like we have room to then implement our own de-facto
standard for letting vendors stuff firmware into EFI as we in the Linux
community see fit.

We can start out by supporting existing drivers, but also consider customizing
this in the future for our own needs, so long as we document it and set
expectations well.

So we need to support what Hans is implementing for two reasons then:

a) The FV Protocol cannot be used to support the two drivers he's
   trying to provide support for -- I believe Hans tried and it didn't work,
   Hans, correct me if I'm wrong?

b) The FV Protocol relies on *internal* interfaces of PI spec, and since:
    1) The PI spec does not define firmware at all
    2) The internal interfaces of PI Spec does not guarantee any backward
       compatibility
   Any implementation details in FV may be subject to change, and may vary
   system to system. Supporting the FV Protocol would be difficult as it
   purposely ambiguous.

If accurate, Hans, can you capture this in your documentation somehow?

And.. we should review Hans' changes in light of not only supporting 2
drivers but also helping advance the framework to properly expand it
in the future as we see fit. And ensuring backward compatibility, should
we change or enhance the interface.

In that case I believe that, other than enhanced documentation justifying
these new changes over supporting the FV protocol, the remaining sticking
points to this patch series was the stronger hashing algorithms used for
which Andy already provided feedback and recommendations for.

  Luis

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

Hi,

On 05/04/2018 06:56 AM, Ard Biesheuvel wrote:
> Hi Hans,
> 
> One comment below, which I missed in review before.
> 
> On 29 April 2018 at 11:35, 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 making this available to peripheral drivers through the
>> standard firmware loading mechanism.
>>
>> 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.
>>
>> Reported-by: Dave Olsthoorn <dave@bewaar.me>
>> Suggested-by: Peter Jones <pjones@redhat.com>
>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
> [...]
>> diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
>> new file mode 100644
>> index 000000000000..22a0f598b53d
>> --- /dev/null
>> +++ b/drivers/firmware/efi/embedded-firmware.c
>> @@ -0,0 +1,149 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Support for extracting embedded firmware for peripherals from EFI code,
>> + *
>> + * Copyright (c) 2018 Hans de Goede <hdegoede@redhat.com>
>> + */
>> +
>> +#include <linux/crc32.h>
>> +#include <linux/dmi.h>
>> +#include <linux/efi.h>
>> +#include <linux/efi_embedded_fw.h>
>> +#include <linux/io.h>
>> +#include <linux/types.h>
>> +#include <linux/vmalloc.h>
>> +
>> +struct embedded_fw {
>> +       struct list_head list;
>> +       const char *name;
>> +       void *data;
>> +       size_t length;
>> +};
>> +
>> +static LIST_HEAD(found_fw_list);
>> +
>> +static const struct dmi_system_id * const embedded_fw_table[] = {
>> +       NULL
>> +};
>> +
>> +/*
>> + * Note the efi_check_for_embedded_firmwares() code currently makes the
>> + * following 2 assumptions. This may needs to be revisited if embedded firmware
>> + * is found where this is not true:
>> + * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory segments
>> + * 2) The firmware always starts at an offset which is a multiple of 8 bytes
>> + */
>> +static int __init efi_check_md_for_embedded_firmware(
>> +       efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
>> +{
>> +       struct embedded_fw *fw;
>> +       u64 i, size;
>> +       u32 crc;
>> +       u8 *mem;
>> +
>> +       size = md->num_pages << EFI_PAGE_SHIFT;
>> +       mem = memremap(md->phys_addr, size, MEMREMAP_WB);
>> +       if (!mem) {
>> +               pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
>> +               return -ENOMEM;
>> +       }
>> +
>> +       size -= desc->length;
>> +       for (i = 0; i < size; i += 8) {
>> +               if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
>> +                       continue;
>> +
> 
> Please use the proper APIs here to cast u8* to u64*, i.e., either use
> get_unaligned64() or use memcmp()

But we know the memory addresses are 64 bit aligned, so using
get_unaligned64 seems wrong, and I'm not sure if the compiler is
smart enough to optimize a memcmp to the single 64 bit integer comparison
we want done here.

Regards,

Hans

> 
>> +               /* Seed with ~0, invert to match crc32 userspace utility */
>> +               crc = ~crc32(~0, mem + i, desc->length);
>> +               if (crc == desc->crc)
>> +                       break;
>> +       }
>> +
>> +       memunmap(mem);
>> +
>> +       if (i >= size)
>> +               return -ENOENT;
>> +
>> +       pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name, desc->crc);
>> +
>> +       fw = kmalloc(sizeof(*fw), GFP_KERNEL);
>> +       if (!fw)
>> +               return -ENOMEM;
>> +
>> +       mem = memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
>> +       if (!mem) {
>> +               pr_err("Error mapping embedded firmware\n");
>> +               goto error_free_fw;
>> +       }
>> +       fw->data = kmemdup(mem, desc->length, GFP_KERNEL);
>> +       memunmap(mem);
>> +       if (!fw->data)
>> +               goto error_free_fw;
>> +
>> +       fw->name = desc->name;
>> +       fw->length = desc->length;
>> +       list_add(&fw->list, &found_fw_list);
>> +
>> +       return 0;
>> +
>> +error_free_fw:
>> +       kfree(fw);
>> +       return -ENOMEM;
>> +}
>> +
>> +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;
>> +               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;
>> +               }
>> +       }
>> +}
>> +
>> +int efi_get_embedded_fw(const char *name, void **data, size_t *size,
>> +                       size_t msize)
>> +{
>> +       struct embedded_fw *iter, *fw = NULL;
>> +       void *buf = *data;
>> +
>> +       list_for_each_entry(iter, &found_fw_list, list) {
>> +               if (strcmp(name, iter->name) == 0) {
>> +                       fw = iter;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       if (!fw)
>> +               return -ENOENT;
>> +
>> +       if (msize && msize < fw->length)
>> +               return -EFBIG;
>> +
>> +       if (!buf) {
>> +               buf = vmalloc(fw->length);
>> +               if (!buf)
>> +                       return -ENOMEM;
>> +       }
>> +
>> +       memcpy(buf, fw->data, fw->length);
>> +       *size = fw->length;
>> +       *data = buf;
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(efi_get_embedded_fw);
>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>> index 791088360c1e..23e8a9c26ce2 100644
>> --- a/include/linux/efi.h
>> +++ b/include/linux/efi.h
>> @@ -1575,6 +1575,12 @@ static inline void
>>   efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
>>   #endif
>>
>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
>> +void efi_check_for_embedded_firmwares(void);
>> +#else
>> +static inline void efi_check_for_embedded_firmwares(void) { }
>> +#endif
>> +
>>   void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
>>
>>   /*
>> diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
>> new file mode 100644
>> index 000000000000..0f7d4df3f57a
>> --- /dev/null
>> +++ b/include/linux/efi_embedded_fw.h
>> @@ -0,0 +1,25 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _LINUX_EFI_EMBEDDED_FW_H
>> +#define _LINUX_EFI_EMBEDDED_FW_H
>> +
>> +#include <linux/mod_devicetable.h>
>> +
>> +/**
>> + * struct efi_embedded_fw_desc - This struct is used by the EFI embedded-fw
>> + *                               code to search for embedded firmwares.
>> + *
>> + * @name:   Name to register the firmware with if found
>> + * @prefix: First 8 bytes of the firmware
>> + * @length: Length of the firmware in bytes including prefix
>> + * @crc:    Inverted little endian Ethernet style CRC32, with 0xffffffff seed
>> + */
>> +struct efi_embedded_fw_desc {
>> +       const char *name;
>> +       u8 prefix[8];
>> +       u32 length;
>> +       u32 crc;
>> +};
>> +
>> +int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t msize);
>> +
>> +#endif
>> diff --git a/init/main.c b/init/main.c
>> index b795aa341a3a..ab29775b35db 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -729,6 +729,9 @@ asmlinkage __visible void __init start_kernel(void)
>>          arch_post_acpi_subsys_init();
>>          sfi_init_late();
>>
>> +       if (efi_enabled(EFI_PRESERVE_BS_REGIONS))
>> +               efi_check_for_embedded_firmwares();
>> +
>>          if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>>                  efi_free_boot_services();
>>          }
>> --
>> 2.17.0
>>

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

Hi,

On 05/04/2018 06:56 AM, Ard Biesheuvel wrote:
> Hi Hans,
> 
> One comment below, which I missed in review before.
> 
> On 29 April 2018 at 11:35, 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 making this available to peripheral drivers through the
>> standard firmware loading mechanism.
>>
>> 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.
>>
>> Reported-by: Dave Olsthoorn <dave@bewaar.me>
>> Suggested-by: Peter Jones <pjones@redhat.com>
>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
> [...]
>> diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
>> new file mode 100644
>> index 000000000000..22a0f598b53d
>> --- /dev/null
>> +++ b/drivers/firmware/efi/embedded-firmware.c
>> @@ -0,0 +1,149 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Support for extracting embedded firmware for peripherals from EFI code,
>> + *
>> + * Copyright (c) 2018 Hans de Goede <hdegoede@redhat.com>
>> + */
>> +
>> +#include <linux/crc32.h>
>> +#include <linux/dmi.h>
>> +#include <linux/efi.h>
>> +#include <linux/efi_embedded_fw.h>
>> +#include <linux/io.h>
>> +#include <linux/types.h>
>> +#include <linux/vmalloc.h>
>> +
>> +struct embedded_fw {
>> +       struct list_head list;
>> +       const char *name;
>> +       void *data;
>> +       size_t length;
>> +};
>> +
>> +static LIST_HEAD(found_fw_list);
>> +
>> +static const struct dmi_system_id * const embedded_fw_table[] = {
>> +       NULL
>> +};
>> +
>> +/*
>> + * Note the efi_check_for_embedded_firmwares() code currently makes the
>> + * following 2 assumptions. This may needs to be revisited if embedded firmware
>> + * is found where this is not true:
>> + * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory segments
>> + * 2) The firmware always starts at an offset which is a multiple of 8 bytes
>> + */
>> +static int __init efi_check_md_for_embedded_firmware(
>> +       efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
>> +{
>> +       struct embedded_fw *fw;
>> +       u64 i, size;
>> +       u32 crc;
>> +       u8 *mem;
>> +
>> +       size = md->num_pages << EFI_PAGE_SHIFT;
>> +       mem = memremap(md->phys_addr, size, MEMREMAP_WB);
>> +       if (!mem) {
>> +               pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
>> +               return -ENOMEM;
>> +       }
>> +
>> +       size -= desc->length;
>> +       for (i = 0; i < size; i += 8) {
>> +               if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
>> +                       continue;
>> +
> 
> Please use the proper APIs here to cast u8* to u64*, i.e., either use
> get_unaligned64() or use memcmp()

But we know the memory addresses are 64 bit aligned, so using
get_unaligned64 seems wrong, and I'm not sure if the compiler is
smart enough to optimize a memcmp to the single 64 bit integer comparison
we want done here.

Regards,

Hans

> 
>> +               /* Seed with ~0, invert to match crc32 userspace utility */
>> +               crc = ~crc32(~0, mem + i, desc->length);
>> +               if (crc == desc->crc)
>> +                       break;
>> +       }
>> +
>> +       memunmap(mem);
>> +
>> +       if (i >= size)
>> +               return -ENOENT;
>> +
>> +       pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name, desc->crc);
>> +
>> +       fw = kmalloc(sizeof(*fw), GFP_KERNEL);
>> +       if (!fw)
>> +               return -ENOMEM;
>> +
>> +       mem = memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
>> +       if (!mem) {
>> +               pr_err("Error mapping embedded firmware\n");
>> +               goto error_free_fw;
>> +       }
>> +       fw->data = kmemdup(mem, desc->length, GFP_KERNEL);
>> +       memunmap(mem);
>> +       if (!fw->data)
>> +               goto error_free_fw;
>> +
>> +       fw->name = desc->name;
>> +       fw->length = desc->length;
>> +       list_add(&fw->list, &found_fw_list);
>> +
>> +       return 0;
>> +
>> +error_free_fw:
>> +       kfree(fw);
>> +       return -ENOMEM;
>> +}
>> +
>> +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;
>> +               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;
>> +               }
>> +       }
>> +}
>> +
>> +int efi_get_embedded_fw(const char *name, void **data, size_t *size,
>> +                       size_t msize)
>> +{
>> +       struct embedded_fw *iter, *fw = NULL;
>> +       void *buf = *data;
>> +
>> +       list_for_each_entry(iter, &found_fw_list, list) {
>> +               if (strcmp(name, iter->name) == 0) {
>> +                       fw = iter;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       if (!fw)
>> +               return -ENOENT;
>> +
>> +       if (msize && msize < fw->length)
>> +               return -EFBIG;
>> +
>> +       if (!buf) {
>> +               buf = vmalloc(fw->length);
>> +               if (!buf)
>> +                       return -ENOMEM;
>> +       }
>> +
>> +       memcpy(buf, fw->data, fw->length);
>> +       *size = fw->length;
>> +       *data = buf;
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(efi_get_embedded_fw);
>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>> index 791088360c1e..23e8a9c26ce2 100644
>> --- a/include/linux/efi.h
>> +++ b/include/linux/efi.h
>> @@ -1575,6 +1575,12 @@ static inline void
>>   efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
>>   #endif
>>
>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
>> +void efi_check_for_embedded_firmwares(void);
>> +#else
>> +static inline void efi_check_for_embedded_firmwares(void) { }
>> +#endif
>> +
>>   void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
>>
>>   /*
>> diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
>> new file mode 100644
>> index 000000000000..0f7d4df3f57a
>> --- /dev/null
>> +++ b/include/linux/efi_embedded_fw.h
>> @@ -0,0 +1,25 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _LINUX_EFI_EMBEDDED_FW_H
>> +#define _LINUX_EFI_EMBEDDED_FW_H
>> +
>> +#include <linux/mod_devicetable.h>
>> +
>> +/**
>> + * struct efi_embedded_fw_desc - This struct is used by the EFI embedded-fw
>> + *                               code to search for embedded firmwares.
>> + *
>> + * @name:   Name to register the firmware with if found
>> + * @prefix: First 8 bytes of the firmware
>> + * @length: Length of the firmware in bytes including prefix
>> + * @crc:    Inverted little endian Ethernet style CRC32, with 0xffffffff seed
>> + */
>> +struct efi_embedded_fw_desc {
>> +       const char *name;
>> +       u8 prefix[8];
>> +       u32 length;
>> +       u32 crc;
>> +};
>> +
>> +int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t msize);
>> +
>> +#endif
>> diff --git a/init/main.c b/init/main.c
>> index b795aa341a3a..ab29775b35db 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -729,6 +729,9 @@ asmlinkage __visible void __init start_kernel(void)
>>          arch_post_acpi_subsys_init();
>>          sfi_init_late();
>>
>> +       if (efi_enabled(EFI_PRESERVE_BS_REGIONS))
>> +               efi_check_for_embedded_firmwares();
>> +
>>          if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>>                  efi_free_boot_services();
>>          }
>> --
>> 2.17.0
>>

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

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

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

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

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

Hi,

On 05/03/2018 11:35 PM, Andy Lutomirski wrote:
> On Thu, May 3, 2018 at 3:31 PM Luis R. Rodriguez <mcgrof@kernel.org> 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.
> 
> Nah, don't use the cryptoapi for this.  You'll probably regret it for any
> number of reasons.  My code is here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=crypto/sha256_bpf&id=e9e12f056f2abed50a30b762db9185799f5864e6
> 
> and its two parents.  It needs a little bit of dusting and it needs
> checking that all combinations of modular and non-modular builds work.  Ard
> probably has further comments.

Looks good, I've cherry picked this into my personal tree and will make
the next version of the EFI embedded-firmware patches use SHA256.

As Luis already mentioned geting the EFI embedded-firmware patches
upstream is not something urgent, so it is probably best to just
wait for you to push these upstream I guess?

Regards,

Hans

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

Hi,

On 05/03/2018 11:35 PM, Andy Lutomirski wrote:
> On Thu, May 3, 2018 at 3:31 PM Luis R. Rodriguez <mcgrof@kernel.org> 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.
> 
> Nah, don't use the cryptoapi for this.  You'll probably regret it for any
> number of reasons.  My code is here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=crypto/sha256_bpf&id=e9e12f056f2abed50a30b762db9185799f5864e6
> 
> and its two parents.  It needs a little bit of dusting and it needs
> checking that all combinations of modular and non-modular builds work.  Ard
> probably has further comments.

Looks good, I've cherry picked this into my personal tree and will make
the next version of the EFI embedded-firmware patches use SHA256.

As Luis already mentioned geting the EFI embedded-firmware patches
upstream is not something urgent, so it is probably best to just
wait for you to push these upstream I guess?

Regards,

Hans

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

Hi,

On 05/03/2018 11:35 PM, Andy Lutomirski wrote:
> On Thu, May 3, 2018 at 3:31 PM Luis R. Rodriguez <mcgrof@kernel.org> 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.
> 
> Nah, don't use the cryptoapi for this.  You'll probably regret it for any
> number of reasons.  My code is here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=crypto/sha256_bpf&id=e9e12f056f2abed50a30b762db9185799f5864e6
> 
> and its two parents.  It needs a little bit of dusting and it needs
> checking that all combinations of modular and non-modular builds work.  Ard
> probably has further comments.

Looks good, I've cherry picked this into my personal tree and will make
the next version of the EFI embedded-firmware patches use SHA256.

As Luis already mentioned geting the EFI embedded-firmware patches
upstream is not something urgent, so it is probably best to just
wait for you to push these upstream I guess?

Regards,

Hans

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Ard Biesheuvel]

On 13 May 2018 at 13:03, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 05/04/2018 06:56 AM, Ard Biesheuvel wrote:
>>
>> Hi Hans,
>>
>> One comment below, which I missed in review before.
>>
>> On 29 April 2018 at 11:35, 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 making this available to peripheral drivers through the
>>> standard firmware loading mechanism.
>>>
>>> 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.
>>>
>>> Reported-by: Dave Olsthoorn <dave@bewaar.me>
>>> Suggested-by: Peter Jones <pjones@redhat.com>
>>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>
>> [...]
>>>
>>> diff --git a/drivers/firmware/efi/embedded-firmware.c
>>> b/drivers/firmware/efi/embedded-firmware.c
>>> new file mode 100644
>>> index 000000000000..22a0f598b53d
>>> --- /dev/null
>>> +++ b/drivers/firmware/efi/embedded-firmware.c
>>> @@ -0,0 +1,149 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Support for extracting embedded firmware for peripherals from EFI
>>> code,
>>> + *
>>> + * Copyright (c) 2018 Hans de Goede <hdegoede@redhat.com>
>>> + */
>>> +
>>> +#include <linux/crc32.h>
>>> +#include <linux/dmi.h>
>>> +#include <linux/efi.h>
>>> +#include <linux/efi_embedded_fw.h>
>>> +#include <linux/io.h>
>>> +#include <linux/types.h>
>>> +#include <linux/vmalloc.h>
>>> +
>>> +struct embedded_fw {
>>> +       struct list_head list;
>>> +       const char *name;
>>> +       void *data;
>>> +       size_t length;
>>> +};
>>> +
>>> +static LIST_HEAD(found_fw_list);
>>> +
>>> +static const struct dmi_system_id * const embedded_fw_table[] = {
>>> +       NULL
>>> +};
>>> +
>>> +/*
>>> + * Note the efi_check_for_embedded_firmwares() code currently makes the
>>> + * following 2 assumptions. This may needs to be revisited if embedded
>>> firmware
>>> + * is found where this is not true:
>>> + * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory
>>> segments
>>> + * 2) The firmware always starts at an offset which is a multiple of 8
>>> bytes
>>> + */
>>> +static int __init efi_check_md_for_embedded_firmware(
>>> +       efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
>>> +{
>>> +       struct embedded_fw *fw;
>>> +       u64 i, size;
>>> +       u32 crc;
>>> +       u8 *mem;
>>> +
>>> +       size = md->num_pages << EFI_PAGE_SHIFT;
>>> +       mem = memremap(md->phys_addr, size, MEMREMAP_WB);
>>> +       if (!mem) {
>>> +               pr_err("Error mapping EFI mem at %#llx\n",
>>> md->phys_addr);
>>> +               return -ENOMEM;
>>> +       }
>>> +
>>> +       size -= desc->length;
>>> +       for (i = 0; i < size; i += 8) {
>>> +               if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
>>> +                       continue;
>>> +
>>
>>
>> Please use the proper APIs here to cast u8* to u64*, i.e., either use
>> get_unaligned64() or use memcmp()
>
>
> But we know the memory addresses are 64 bit aligned, so using
> get_unaligned64 seems wrong, and I'm not sure if the compiler is
> smart enough to optimize a memcmp to the single 64 bit integer comparison
> we want done here.
>

Fair enough. The memory regions are indeed guaranteed to be 4k aligned.

So in that case, please make mem a u64* and cast the other way where needed.

>>
>>> +               /* Seed with ~0, invert to match crc32 userspace utility
>>> */
>>> +               crc = ~crc32(~0, mem + i, desc->length);
>>> +               if (crc == desc->crc)
>>> +                       break;
>>> +       }
>>> +
>>> +       memunmap(mem);
>>> +
>>> +       if (i >= size)
>>> +               return -ENOENT;
>>> +
>>> +       pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name,
>>> desc->crc);
>>> +
>>> +       fw = kmalloc(sizeof(*fw), GFP_KERNEL);
>>> +       if (!fw)
>>> +               return -ENOMEM;
>>> +
>>> +       mem = memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
>>> +       if (!mem) {
>>> +               pr_err("Error mapping embedded firmware\n");
>>> +               goto error_free_fw;
>>> +       }
>>> +       fw->data = kmemdup(mem, desc->length, GFP_KERNEL);
>>> +       memunmap(mem);
>>> +       if (!fw->data)
>>> +               goto error_free_fw;
>>> +
>>> +       fw->name = desc->name;
>>> +       fw->length = desc->length;
>>> +       list_add(&fw->list, &found_fw_list);
>>> +
>>> +       return 0;
>>> +
>>> +error_free_fw:
>>> +       kfree(fw);
>>> +       return -ENOMEM;
>>> +}
>>> +
>>> +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;
>>> +               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;
>>> +               }
>>> +       }
>>> +}
>>> +
>>> +int efi_get_embedded_fw(const char *name, void **data, size_t *size,
>>> +                       size_t msize)
>>> +{
>>> +       struct embedded_fw *iter, *fw = NULL;
>>> +       void *buf = *data;
>>> +
>>> +       list_for_each_entry(iter, &found_fw_list, list) {
>>> +               if (strcmp(name, iter->name) == 0) {
>>> +                       fw = iter;
>>> +                       break;
>>> +               }
>>> +       }
>>> +
>>> +       if (!fw)
>>> +               return -ENOENT;
>>> +
>>> +       if (msize && msize < fw->length)
>>> +               return -EFBIG;
>>> +
>>> +       if (!buf) {
>>> +               buf = vmalloc(fw->length);
>>> +               if (!buf)
>>> +                       return -ENOMEM;
>>> +       }
>>> +
>>> +       memcpy(buf, fw->data, fw->length);
>>> +       *size = fw->length;
>>> +       *data = buf;
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(efi_get_embedded_fw);
>>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>>> index 791088360c1e..23e8a9c26ce2 100644
>>> --- a/include/linux/efi.h
>>> +++ b/include/linux/efi.h
>>> @@ -1575,6 +1575,12 @@ static inline void
>>>   efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) {
>>> }
>>>   #endif
>>>
>>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
>>> +void efi_check_for_embedded_firmwares(void);
>>> +#else
>>> +static inline void efi_check_for_embedded_firmwares(void) { }
>>> +#endif
>>> +
>>>   void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
>>>
>>>   /*
>>> diff --git a/include/linux/efi_embedded_fw.h
>>> b/include/linux/efi_embedded_fw.h
>>> new file mode 100644
>>> index 000000000000..0f7d4df3f57a
>>> --- /dev/null
>>> +++ b/include/linux/efi_embedded_fw.h
>>> @@ -0,0 +1,25 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef _LINUX_EFI_EMBEDDED_FW_H
>>> +#define _LINUX_EFI_EMBEDDED_FW_H
>>> +
>>> +#include <linux/mod_devicetable.h>
>>> +
>>> +/**
>>> + * struct efi_embedded_fw_desc - This struct is used by the EFI
>>> embedded-fw
>>> + *                               code to search for embedded firmwares.
>>> + *
>>> + * @name:   Name to register the firmware with if found
>>> + * @prefix: First 8 bytes of the firmware
>>> + * @length: Length of the firmware in bytes including prefix
>>> + * @crc:    Inverted little endian Ethernet style CRC32, with 0xffffffff
>>> seed
>>> + */
>>> +struct efi_embedded_fw_desc {
>>> +       const char *name;
>>> +       u8 prefix[8];
>>> +       u32 length;
>>> +       u32 crc;
>>> +};
>>> +
>>> +int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t
>>> msize);
>>> +
>>> +#endif
>>> diff --git a/init/main.c b/init/main.c
>>> index b795aa341a3a..ab29775b35db 100644
>>> --- a/init/main.c
>>> +++ b/init/main.c
>>> @@ -729,6 +729,9 @@ asmlinkage __visible void __init start_kernel(void)
>>>          arch_post_acpi_subsys_init();
>>>          sfi_init_late();
>>>
>>> +       if (efi_enabled(EFI_PRESERVE_BS_REGIONS))
>>> +               efi_check_for_embedded_firmwares();
>>> +
>>>          if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>>>                  efi_free_boot_services();
>>>          }
>>> --
>>> 2.17.0
>>>
>

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Ard Biesheuvel]

On 13 May 2018 at 13:03, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 05/04/2018 06:56 AM, Ard Biesheuvel wrote:
>>
>> Hi Hans,
>>
>> One comment below, which I missed in review before.
>>
>> On 29 April 2018 at 11:35, 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 making this available to peripheral drivers through the
>>> standard firmware loading mechanism.
>>>
>>> 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.
>>>
>>> Reported-by: Dave Olsthoorn <dave@bewaar.me>
>>> Suggested-by: Peter Jones <pjones@redhat.com>
>>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>
>> [...]
>>>
>>> diff --git a/drivers/firmware/efi/embedded-firmware.c
>>> b/drivers/firmware/efi/embedded-firmware.c
>>> new file mode 100644
>>> index 000000000000..22a0f598b53d
>>> --- /dev/null
>>> +++ b/drivers/firmware/efi/embedded-firmware.c
>>> @@ -0,0 +1,149 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Support for extracting embedded firmware for peripherals from EFI
>>> code,
>>> + *
>>> + * Copyright (c) 2018 Hans de Goede <hdegoede@redhat.com>
>>> + */
>>> +
>>> +#include <linux/crc32.h>
>>> +#include <linux/dmi.h>
>>> +#include <linux/efi.h>
>>> +#include <linux/efi_embedded_fw.h>
>>> +#include <linux/io.h>
>>> +#include <linux/types.h>
>>> +#include <linux/vmalloc.h>
>>> +
>>> +struct embedded_fw {
>>> +       struct list_head list;
>>> +       const char *name;
>>> +       void *data;
>>> +       size_t length;
>>> +};
>>> +
>>> +static LIST_HEAD(found_fw_list);
>>> +
>>> +static const struct dmi_system_id * const embedded_fw_table[] = {
>>> +       NULL
>>> +};
>>> +
>>> +/*
>>> + * Note the efi_check_for_embedded_firmwares() code currently makes the
>>> + * following 2 assumptions. This may needs to be revisited if embedded
>>> firmware
>>> + * is found where this is not true:
>>> + * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory
>>> segments
>>> + * 2) The firmware always starts at an offset which is a multiple of 8
>>> bytes
>>> + */
>>> +static int __init efi_check_md_for_embedded_firmware(
>>> +       efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
>>> +{
>>> +       struct embedded_fw *fw;
>>> +       u64 i, size;
>>> +       u32 crc;
>>> +       u8 *mem;
>>> +
>>> +       size = md->num_pages << EFI_PAGE_SHIFT;
>>> +       mem = memremap(md->phys_addr, size, MEMREMAP_WB);
>>> +       if (!mem) {
>>> +               pr_err("Error mapping EFI mem at %#llx\n",
>>> md->phys_addr);
>>> +               return -ENOMEM;
>>> +       }
>>> +
>>> +       size -= desc->length;
>>> +       for (i = 0; i < size; i += 8) {
>>> +               if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
>>> +                       continue;
>>> +
>>
>>
>> Please use the proper APIs here to cast u8* to u64*, i.e., either use
>> get_unaligned64() or use memcmp()
>
>
> But we know the memory addresses are 64 bit aligned, so using
> get_unaligned64 seems wrong, and I'm not sure if the compiler is
> smart enough to optimize a memcmp to the single 64 bit integer comparison
> we want done here.
>

Fair enough. The memory regions are indeed guaranteed to be 4k aligned.

So in that case, please make mem a u64* and cast the other way where needed.

>>
>>> +               /* Seed with ~0, invert to match crc32 userspace utility
>>> */
>>> +               crc = ~crc32(~0, mem + i, desc->length);
>>> +               if (crc == desc->crc)
>>> +                       break;
>>> +       }
>>> +
>>> +       memunmap(mem);
>>> +
>>> +       if (i >= size)
>>> +               return -ENOENT;
>>> +
>>> +       pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name,
>>> desc->crc);
>>> +
>>> +       fw = kmalloc(sizeof(*fw), GFP_KERNEL);
>>> +       if (!fw)
>>> +               return -ENOMEM;
>>> +
>>> +       mem = memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
>>> +       if (!mem) {
>>> +               pr_err("Error mapping embedded firmware\n");
>>> +               goto error_free_fw;
>>> +       }
>>> +       fw->data = kmemdup(mem, desc->length, GFP_KERNEL);
>>> +       memunmap(mem);
>>> +       if (!fw->data)
>>> +               goto error_free_fw;
>>> +
>>> +       fw->name = desc->name;
>>> +       fw->length = desc->length;
>>> +       list_add(&fw->list, &found_fw_list);
>>> +
>>> +       return 0;
>>> +
>>> +error_free_fw:
>>> +       kfree(fw);
>>> +       return -ENOMEM;
>>> +}
>>> +
>>> +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;
>>> +               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;
>>> +               }
>>> +       }
>>> +}
>>> +
>>> +int efi_get_embedded_fw(const char *name, void **data, size_t *size,
>>> +                       size_t msize)
>>> +{
>>> +       struct embedded_fw *iter, *fw = NULL;
>>> +       void *buf = *data;
>>> +
>>> +       list_for_each_entry(iter, &found_fw_list, list) {
>>> +               if (strcmp(name, iter->name) == 0) {
>>> +                       fw = iter;
>>> +                       break;
>>> +               }
>>> +       }
>>> +
>>> +       if (!fw)
>>> +               return -ENOENT;
>>> +
>>> +       if (msize && msize < fw->length)
>>> +               return -EFBIG;
>>> +
>>> +       if (!buf) {
>>> +               buf = vmalloc(fw->length);
>>> +               if (!buf)
>>> +                       return -ENOMEM;
>>> +       }
>>> +
>>> +       memcpy(buf, fw->data, fw->length);
>>> +       *size = fw->length;
>>> +       *data = buf;
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(efi_get_embedded_fw);
>>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>>> index 791088360c1e..23e8a9c26ce2 100644
>>> --- a/include/linux/efi.h
>>> +++ b/include/linux/efi.h
>>> @@ -1575,6 +1575,12 @@ static inline void
>>>   efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) {
>>> }
>>>   #endif
>>>
>>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
>>> +void efi_check_for_embedded_firmwares(void);
>>> +#else
>>> +static inline void efi_check_for_embedded_firmwares(void) { }
>>> +#endif
>>> +
>>>   void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
>>>
>>>   /*
>>> diff --git a/include/linux/efi_embedded_fw.h
>>> b/include/linux/efi_embedded_fw.h
>>> new file mode 100644
>>> index 000000000000..0f7d4df3f57a
>>> --- /dev/null
>>> +++ b/include/linux/efi_embedded_fw.h
>>> @@ -0,0 +1,25 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef _LINUX_EFI_EMBEDDED_FW_H
>>> +#define _LINUX_EFI_EMBEDDED_FW_H
>>> +
>>> +#include <linux/mod_devicetable.h>
>>> +
>>> +/**
>>> + * struct efi_embedded_fw_desc - This struct is used by the EFI
>>> embedded-fw
>>> + *                               code to search for embedded firmwares.
>>> + *
>>> + * @name:   Name to register the firmware with if found
>>> + * @prefix: First 8 bytes of the firmware
>>> + * @length: Length of the firmware in bytes including prefix
>>> + * @crc:    Inverted little endian Ethernet style CRC32, with 0xffffffff
>>> seed
>>> + */
>>> +struct efi_embedded_fw_desc {
>>> +       const char *name;
>>> +       u8 prefix[8];
>>> +       u32 length;
>>> +       u32 crc;
>>> +};
>>> +
>>> +int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t
>>> msize);
>>> +
>>> +#endif
>>> diff --git a/init/main.c b/init/main.c
>>> index b795aa341a3a..ab29775b35db 100644
>>> --- a/init/main.c
>>> +++ b/init/main.c
>>> @@ -729,6 +729,9 @@ asmlinkage __visible void __init start_kernel(void)
>>>          arch_post_acpi_subsys_init();
>>>          sfi_init_late();
>>>
>>> +       if (efi_enabled(EFI_PRESERVE_BS_REGIONS))
>>> +               efi_check_for_embedded_firmwares();
>>> +
>>>          if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>>>                  efi_free_boot_services();
>>>          }
>>> --
>>> 2.17.0
>>>
>

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

Hi,

On 05/13/2018 12:43 PM, Ard Biesheuvel wrote:
> On 13 May 2018 at 13:03, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 05/04/2018 06:56 AM, Ard Biesheuvel wrote:
>>>
>>> Hi Hans,
>>>
>>> One comment below, which I missed in review before.
>>>
>>> On 29 April 2018 at 11:35, 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 making this available to peripheral drivers through the
>>>> standard firmware loading mechanism.
>>>>
>>>> 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.
>>>>
>>>> Reported-by: Dave Olsthoorn <dave@bewaar.me>
>>>> Suggested-by: Peter Jones <pjones@redhat.com>
>>>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>
>>> [...]
>>>>
>>>> diff --git a/drivers/firmware/efi/embedded-firmware.c
>>>> b/drivers/firmware/efi/embedded-firmware.c
>>>> new file mode 100644
>>>> index 000000000000..22a0f598b53d
>>>> --- /dev/null
>>>> +++ b/drivers/firmware/efi/embedded-firmware.c
>>>> @@ -0,0 +1,149 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Support for extracting embedded firmware for peripherals from EFI
>>>> code,
>>>> + *
>>>> + * Copyright (c) 2018 Hans de Goede <hdegoede@redhat.com>
>>>> + */
>>>> +
>>>> +#include <linux/crc32.h>
>>>> +#include <linux/dmi.h>
>>>> +#include <linux/efi.h>
>>>> +#include <linux/efi_embedded_fw.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/types.h>
>>>> +#include <linux/vmalloc.h>
>>>> +
>>>> +struct embedded_fw {
>>>> +       struct list_head list;
>>>> +       const char *name;
>>>> +       void *data;
>>>> +       size_t length;
>>>> +};
>>>> +
>>>> +static LIST_HEAD(found_fw_list);
>>>> +
>>>> +static const struct dmi_system_id * const embedded_fw_table[] = {
>>>> +       NULL
>>>> +};
>>>> +
>>>> +/*
>>>> + * Note the efi_check_for_embedded_firmwares() code currently makes the
>>>> + * following 2 assumptions. This may needs to be revisited if embedded
>>>> firmware
>>>> + * is found where this is not true:
>>>> + * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory
>>>> segments
>>>> + * 2) The firmware always starts at an offset which is a multiple of 8
>>>> bytes
>>>> + */
>>>> +static int __init efi_check_md_for_embedded_firmware(
>>>> +       efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
>>>> +{
>>>> +       struct embedded_fw *fw;
>>>> +       u64 i, size;
>>>> +       u32 crc;
>>>> +       u8 *mem;
>>>> +
>>>> +       size = md->num_pages << EFI_PAGE_SHIFT;
>>>> +       mem = memremap(md->phys_addr, size, MEMREMAP_WB);
>>>> +       if (!mem) {
>>>> +               pr_err("Error mapping EFI mem at %#llx\n",
>>>> md->phys_addr);
>>>> +               return -ENOMEM;
>>>> +       }
>>>> +
>>>> +       size -= desc->length;
>>>> +       for (i = 0; i < size; i += 8) {
>>>> +               if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
>>>> +                       continue;
>>>> +
>>>
>>>
>>> Please use the proper APIs here to cast u8* to u64*, i.e., either use
>>> get_unaligned64() or use memcmp()
>>
>>
>> But we know the memory addresses are 64 bit aligned, so using
>> get_unaligned64 seems wrong, and I'm not sure if the compiler is
>> smart enough to optimize a memcmp to the single 64 bit integer comparison
>> we want done here.
>>
> 
> Fair enough. The memory regions are indeed guaranteed to be 4k aligned.
> 
> So in that case, please make mem a u64* and cast the other way where needed.

Ok, I've reworked the code to get rid of the compares in the if condition.

Regards,

Hans



> 
>>>
>>>> +               /* Seed with ~0, invert to match crc32 userspace utility
>>>> */
>>>> +               crc = ~crc32(~0, mem + i, desc->length);
>>>> +               if (crc == desc->crc)
>>>> +                       break;
>>>> +       }
>>>> +
>>>> +       memunmap(mem);
>>>> +
>>>> +       if (i >= size)
>>>> +               return -ENOENT;
>>>> +
>>>> +       pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name,
>>>> desc->crc);
>>>> +
>>>> +       fw = kmalloc(sizeof(*fw), GFP_KERNEL);
>>>> +       if (!fw)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       mem = memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
>>>> +       if (!mem) {
>>>> +               pr_err("Error mapping embedded firmware\n");
>>>> +               goto error_free_fw;
>>>> +       }
>>>> +       fw->data = kmemdup(mem, desc->length, GFP_KERNEL);
>>>> +       memunmap(mem);
>>>> +       if (!fw->data)
>>>> +               goto error_free_fw;
>>>> +
>>>> +       fw->name = desc->name;
>>>> +       fw->length = desc->length;
>>>> +       list_add(&fw->list, &found_fw_list);
>>>> +
>>>> +       return 0;
>>>> +
>>>> +error_free_fw:
>>>> +       kfree(fw);
>>>> +       return -ENOMEM;
>>>> +}
>>>> +
>>>> +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;
>>>> +               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;
>>>> +               }
>>>> +       }
>>>> +}
>>>> +
>>>> +int efi_get_embedded_fw(const char *name, void **data, size_t *size,
>>>> +                       size_t msize)
>>>> +{
>>>> +       struct embedded_fw *iter, *fw = NULL;
>>>> +       void *buf = *data;
>>>> +
>>>> +       list_for_each_entry(iter, &found_fw_list, list) {
>>>> +               if (strcmp(name, iter->name) == 0) {
>>>> +                       fw = iter;
>>>> +                       break;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       if (!fw)
>>>> +               return -ENOENT;
>>>> +
>>>> +       if (msize && msize < fw->length)
>>>> +               return -EFBIG;
>>>> +
>>>> +       if (!buf) {
>>>> +               buf = vmalloc(fw->length);
>>>> +               if (!buf)
>>>> +                       return -ENOMEM;
>>>> +       }
>>>> +
>>>> +       memcpy(buf, fw->data, fw->length);
>>>> +       *size = fw->length;
>>>> +       *data = buf;
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(efi_get_embedded_fw);
>>>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>>>> index 791088360c1e..23e8a9c26ce2 100644
>>>> --- a/include/linux/efi.h
>>>> +++ b/include/linux/efi.h
>>>> @@ -1575,6 +1575,12 @@ static inline void
>>>>    efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) {
>>>> }
>>>>    #endif
>>>>
>>>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
>>>> +void efi_check_for_embedded_firmwares(void);
>>>> +#else
>>>> +static inline void efi_check_for_embedded_firmwares(void) { }
>>>> +#endif
>>>> +
>>>>    void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
>>>>
>>>>    /*
>>>> diff --git a/include/linux/efi_embedded_fw.h
>>>> b/include/linux/efi_embedded_fw.h
>>>> new file mode 100644
>>>> index 000000000000..0f7d4df3f57a
>>>> --- /dev/null
>>>> +++ b/include/linux/efi_embedded_fw.h
>>>> @@ -0,0 +1,25 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +#ifndef _LINUX_EFI_EMBEDDED_FW_H
>>>> +#define _LINUX_EFI_EMBEDDED_FW_H
>>>> +
>>>> +#include <linux/mod_devicetable.h>
>>>> +
>>>> +/**
>>>> + * struct efi_embedded_fw_desc - This struct is used by the EFI
>>>> embedded-fw
>>>> + *                               code to search for embedded firmwares.
>>>> + *
>>>> + * @name:   Name to register the firmware with if found
>>>> + * @prefix: First 8 bytes of the firmware
>>>> + * @length: Length of the firmware in bytes including prefix
>>>> + * @crc:    Inverted little endian Ethernet style CRC32, with 0xffffffff
>>>> seed
>>>> + */
>>>> +struct efi_embedded_fw_desc {
>>>> +       const char *name;
>>>> +       u8 prefix[8];
>>>> +       u32 length;
>>>> +       u32 crc;
>>>> +};
>>>> +
>>>> +int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t
>>>> msize);
>>>> +
>>>> +#endif
>>>> diff --git a/init/main.c b/init/main.c
>>>> index b795aa341a3a..ab29775b35db 100644
>>>> --- a/init/main.c
>>>> +++ b/init/main.c
>>>> @@ -729,6 +729,9 @@ asmlinkage __visible void __init start_kernel(void)
>>>>           arch_post_acpi_subsys_init();
>>>>           sfi_init_late();
>>>>
>>>> +       if (efi_enabled(EFI_PRESERVE_BS_REGIONS))
>>>> +               efi_check_for_embedded_firmwares();
>>>> +
>>>>           if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>>>>                   efi_free_boot_services();
>>>>           }
>>>> --
>>>> 2.17.0
>>>>
>>

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

Hi,

On 05/13/2018 12:43 PM, Ard Biesheuvel wrote:
> On 13 May 2018 at 13:03, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 05/04/2018 06:56 AM, Ard Biesheuvel wrote:
>>>
>>> Hi Hans,
>>>
>>> One comment below, which I missed in review before.
>>>
>>> On 29 April 2018 at 11:35, 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 making this available to peripheral drivers through the
>>>> standard firmware loading mechanism.
>>>>
>>>> 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.
>>>>
>>>> Reported-by: Dave Olsthoorn <dave@bewaar.me>
>>>> Suggested-by: Peter Jones <pjones@redhat.com>
>>>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>
>>> [...]
>>>>
>>>> diff --git a/drivers/firmware/efi/embedded-firmware.c
>>>> b/drivers/firmware/efi/embedded-firmware.c
>>>> new file mode 100644
>>>> index 000000000000..22a0f598b53d
>>>> --- /dev/null
>>>> +++ b/drivers/firmware/efi/embedded-firmware.c
>>>> @@ -0,0 +1,149 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Support for extracting embedded firmware for peripherals from EFI
>>>> code,
>>>> + *
>>>> + * Copyright (c) 2018 Hans de Goede <hdegoede@redhat.com>
>>>> + */
>>>> +
>>>> +#include <linux/crc32.h>
>>>> +#include <linux/dmi.h>
>>>> +#include <linux/efi.h>
>>>> +#include <linux/efi_embedded_fw.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/types.h>
>>>> +#include <linux/vmalloc.h>
>>>> +
>>>> +struct embedded_fw {
>>>> +       struct list_head list;
>>>> +       const char *name;
>>>> +       void *data;
>>>> +       size_t length;
>>>> +};
>>>> +
>>>> +static LIST_HEAD(found_fw_list);
>>>> +
>>>> +static const struct dmi_system_id * const embedded_fw_table[] = {
>>>> +       NULL
>>>> +};
>>>> +
>>>> +/*
>>>> + * Note the efi_check_for_embedded_firmwares() code currently makes the
>>>> + * following 2 assumptions. This may needs to be revisited if embedded
>>>> firmware
>>>> + * is found where this is not true:
>>>> + * 1) The firmware is only found in EFI_BOOT_SERVICES_CODE memory
>>>> segments
>>>> + * 2) The firmware always starts at an offset which is a multiple of 8
>>>> bytes
>>>> + */
>>>> +static int __init efi_check_md_for_embedded_firmware(
>>>> +       efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
>>>> +{
>>>> +       struct embedded_fw *fw;
>>>> +       u64 i, size;
>>>> +       u32 crc;
>>>> +       u8 *mem;
>>>> +
>>>> +       size = md->num_pages << EFI_PAGE_SHIFT;
>>>> +       mem = memremap(md->phys_addr, size, MEMREMAP_WB);
>>>> +       if (!mem) {
>>>> +               pr_err("Error mapping EFI mem at %#llx\n",
>>>> md->phys_addr);
>>>> +               return -ENOMEM;
>>>> +       }
>>>> +
>>>> +       size -= desc->length;
>>>> +       for (i = 0; i < size; i += 8) {
>>>> +               if (*((u64 *)(mem + i)) != *((u64 *)desc->prefix))
>>>> +                       continue;
>>>> +
>>>
>>>
>>> Please use the proper APIs here to cast u8* to u64*, i.e., either use
>>> get_unaligned64() or use memcmp()
>>
>>
>> But we know the memory addresses are 64 bit aligned, so using
>> get_unaligned64 seems wrong, and I'm not sure if the compiler is
>> smart enough to optimize a memcmp to the single 64 bit integer comparison
>> we want done here.
>>
> 
> Fair enough. The memory regions are indeed guaranteed to be 4k aligned.
> 
> So in that case, please make mem a u64* and cast the other way where needed.

Ok, I've reworked the code to get rid of the compares in the if condition.

Regards,

Hans



> 
>>>
>>>> +               /* Seed with ~0, invert to match crc32 userspace utility
>>>> */
>>>> +               crc = ~crc32(~0, mem + i, desc->length);
>>>> +               if (crc == desc->crc)
>>>> +                       break;
>>>> +       }
>>>> +
>>>> +       memunmap(mem);
>>>> +
>>>> +       if (i >= size)
>>>> +               return -ENOENT;
>>>> +
>>>> +       pr_info("Found EFI embedded fw '%s' crc %08x\n", desc->name,
>>>> desc->crc);
>>>> +
>>>> +       fw = kmalloc(sizeof(*fw), GFP_KERNEL);
>>>> +       if (!fw)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       mem = memremap(md->phys_addr + i, desc->length, MEMREMAP_WB);
>>>> +       if (!mem) {
>>>> +               pr_err("Error mapping embedded firmware\n");
>>>> +               goto error_free_fw;
>>>> +       }
>>>> +       fw->data = kmemdup(mem, desc->length, GFP_KERNEL);
>>>> +       memunmap(mem);
>>>> +       if (!fw->data)
>>>> +               goto error_free_fw;
>>>> +
>>>> +       fw->name = desc->name;
>>>> +       fw->length = desc->length;
>>>> +       list_add(&fw->list, &found_fw_list);
>>>> +
>>>> +       return 0;
>>>> +
>>>> +error_free_fw:
>>>> +       kfree(fw);
>>>> +       return -ENOMEM;
>>>> +}
>>>> +
>>>> +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;
>>>> +               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;
>>>> +               }
>>>> +       }
>>>> +}
>>>> +
>>>> +int efi_get_embedded_fw(const char *name, void **data, size_t *size,
>>>> +                       size_t msize)
>>>> +{
>>>> +       struct embedded_fw *iter, *fw = NULL;
>>>> +       void *buf = *data;
>>>> +
>>>> +       list_for_each_entry(iter, &found_fw_list, list) {
>>>> +               if (strcmp(name, iter->name) == 0) {
>>>> +                       fw = iter;
>>>> +                       break;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       if (!fw)
>>>> +               return -ENOENT;
>>>> +
>>>> +       if (msize && msize < fw->length)
>>>> +               return -EFBIG;
>>>> +
>>>> +       if (!buf) {
>>>> +               buf = vmalloc(fw->length);
>>>> +               if (!buf)
>>>> +                       return -ENOMEM;
>>>> +       }
>>>> +
>>>> +       memcpy(buf, fw->data, fw->length);
>>>> +       *size = fw->length;
>>>> +       *data = buf;
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(efi_get_embedded_fw);
>>>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>>>> index 791088360c1e..23e8a9c26ce2 100644
>>>> --- a/include/linux/efi.h
>>>> +++ b/include/linux/efi.h
>>>> @@ -1575,6 +1575,12 @@ static inline void
>>>>    efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) {
>>>> }
>>>>    #endif
>>>>
>>>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
>>>> +void efi_check_for_embedded_firmwares(void);
>>>> +#else
>>>> +static inline void efi_check_for_embedded_firmwares(void) { }
>>>> +#endif
>>>> +
>>>>    void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table);
>>>>
>>>>    /*
>>>> diff --git a/include/linux/efi_embedded_fw.h
>>>> b/include/linux/efi_embedded_fw.h
>>>> new file mode 100644
>>>> index 000000000000..0f7d4df3f57a
>>>> --- /dev/null
>>>> +++ b/include/linux/efi_embedded_fw.h
>>>> @@ -0,0 +1,25 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +#ifndef _LINUX_EFI_EMBEDDED_FW_H
>>>> +#define _LINUX_EFI_EMBEDDED_FW_H
>>>> +
>>>> +#include <linux/mod_devicetable.h>
>>>> +
>>>> +/**
>>>> + * struct efi_embedded_fw_desc - This struct is used by the EFI
>>>> embedded-fw
>>>> + *                               code to search for embedded firmwares.
>>>> + *
>>>> + * @name:   Name to register the firmware with if found
>>>> + * @prefix: First 8 bytes of the firmware
>>>> + * @length: Length of the firmware in bytes including prefix
>>>> + * @crc:    Inverted little endian Ethernet style CRC32, with 0xffffffff
>>>> seed
>>>> + */
>>>> +struct efi_embedded_fw_desc {
>>>> +       const char *name;
>>>> +       u8 prefix[8];
>>>> +       u32 length;
>>>> +       u32 crc;
>>>> +};
>>>> +
>>>> +int efi_get_embedded_fw(const char *name, void **dat, size_t *sz, size_t
>>>> msize);
>>>> +
>>>> +#endif
>>>> diff --git a/init/main.c b/init/main.c
>>>> index b795aa341a3a..ab29775b35db 100644
>>>> --- a/init/main.c
>>>> +++ b/init/main.c
>>>> @@ -729,6 +729,9 @@ asmlinkage __visible void __init start_kernel(void)
>>>>           arch_post_acpi_subsys_init();
>>>>           sfi_init_late();
>>>>
>>>> +       if (efi_enabled(EFI_PRESERVE_BS_REGIONS))
>>>> +               efi_check_for_embedded_firmwares();
>>>> +
>>>>           if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>>>>                   efi_free_boot_services();
>>>>           }
>>>> --
>>>> 2.17.0
>>>>
>>

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

Hi,

On 05/08/2018 06:12 PM, Luis R. Rodriguez wrote:
> On Fri, May 04, 2018 at 07:54:28AM +0200, Ard Biesheuvel wrote:
>> On 4 May 2018 at 01:29, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>>> On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede wrote:
>> [...]
>>>> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
>>>> index c8bddbdcfd10..560dfed76e38 100644
>>>> --- a/Documentation/driver-api/firmware/request_firmware.rst
>>>> +++ b/Documentation/driver-api/firmware/request_firmware.rst
>>>> @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns non-zero and fw_entry
>>>>   is set to NULL. Once your driver is done with processing the firmware it
>>>>   can call call firmware_release(fw_entry) to release the firmware image
>>>>   and any related resource.
>>>> +
>>>> +EFI embedded firmware support
>>>> +=============================
>>>
>>> This is a new fallback mechanism, please see:
>>>
>>> Documentation/driver-api/firmware/fallback-mechanisms.rst
>>>
>>> Refer to the section "Types of fallback mechanisms", augument the list there
>>> and then move the section "Firmware sysfs loading facility" to a new file, and
>>> then add a new file for your own.
>>>
>>>> +
>>>> +On some devices the system's EFI code / ROM may contain an embedded copy
>>>> +of firmware for some of the system's integrated peripheral devices and
>>>> +the peripheral's Linux device-driver needs to access this firmware.
>>>
>>> You in no way indicate this is a just an invented scheme, a custom solution and
>>> nothing standard.  I realize Ard criticized that the EFI Firmware Volume Protocol
>>> is not part of the UEFI spec -- however it is a bit more widely used right?
>>> Why can't Linux support it instead?
>>>
>>
>> Most implementations of UEFI are based on PI,
> 
> That seems to be the UEFI Platform Initialization specification:
> 
> http://www.uefi.org/sites/default/files/resources/PI_Spec_1_6.pdf
> 
>> and so it is likely that
>> the protocols are available. However, the PI spec does not cover
>> firmware blobs,
> 
> Indeed, I cannot find anything about it on the PI Spec, but I *can* easily
> find a few documents referring to the Firmware Volume Protocol:
> 
> http://wiki.phoenix.com/wiki/index.php/EFI_FIRMWARE_VOLUME_PROTOCOL
> 
> But this has no references at all...
> 
> I see stupid patents over some of this and authentication mechanisms for it:
> 
> https://patents.google.com/patent/US20170098084
> 
>> and so it is undefined whether such blobs are self
>> contained (i.e., in separate files in the firmware volume), statically
>> linked into the driver or maybe even encrypted or otherwise
>> encapsulated, and the actual loadable image only lives in memory.
> 
> Got it, thanks this helps! There are two things then:
> 
>   1) The "EFI Firmware Volume Protocol" ("FV" for short in your descriptions
>      below), and whether to support it or not in the future and recommend it
>      for future use cases.
> 
>   b) Han's EFI scraper to help support 2 drivers, and whether or not to
>      recommend it for future use cases.
> 
>> Hans's case is the second one, i.e., the firmware is at an arbitrary
>> offset in the driver image. Using the FV protocol in this case would
>> result in a mix of both approaches: look up the driver file by GUID
>> [which could change btw between different versions of the system
>> firmware, although this is unlikely] and then still use the prefix/crc
>> based approach to sift through the image itself.
> 
> Got it. And to be clear its a reversed engineered solution to what
> two vendors decided to do.
> 
>> But my main objection is simply that from the UEFI forum point of
>> view, there is a clear distinction between the OS visible interfaces
>> in the UEFI spec and the internal interfaces in the PI spec (which for
>> instance are not subject to the same rules when it comes to backward
>> compatibility), and so I think we should not depend on PI at all.
> 
> Ah I see.
> 
>> This
>> is all the more important considering that we are trying to encourage
>> the creation of other implementations of UEFI that are not based on PI
>> (e.g., uboot for arm64 implements the required UEFI interfaces for
>> booting the kernel via GRUB), and adding dependencies on PI protocols
>> makes that a moving target.
> 
> Got it!
> 
>> So in my view, we either take a ad-hoc approach which works for the
>> few platforms we expect to support, in which case Hans's approach is
>> sufficient,
> 
> Modulo it needs some work for ARM as it only works for x86 right now ;)
> 
>> or we architect it properly, in which case we shouldn't
>> depend on PI because it does not belong in a properly architected
>> OS<->firmware exchange.
> 
> OK, it sounds to me like we have room to then implement our own de-facto
> standard for letting vendors stuff firmware into EFI as we in the Linux
> community see fit.
> 
> We can start out by supporting existing drivers, but also consider customizing
> this in the future for our own needs, so long as we document it and set
> expectations well.
> 
> So we need to support what Hans is implementing for two reasons then:
> 
> a) The FV Protocol cannot be used to support the two drivers he's
>     trying to provide support for -- I believe Hans tried and it didn't work,
>     Hans, correct me if I'm wrong?
> 
> b) The FV Protocol relies on *internal* interfaces of PI spec, and since:
>      1) The PI spec does not define firmware at all
>      2) The internal interfaces of PI Spec does not guarantee any backward
>         compatibility
>     Any implementation details in FV may be subject to change, and may vary
>     system to system. Supporting the FV Protocol would be difficult as it
>     purposely ambiguous.
> 
> If accurate, Hans, can you capture this in your documentation somehow?

Yes I've added some extra doc to this extend for the next version of the
patchset.

Regards,

Hans

Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support

[Hans de Goede]

Hi,

On 05/08/2018 06:12 PM, Luis R. Rodriguez wrote:
> On Fri, May 04, 2018 at 07:54:28AM +0200, Ard Biesheuvel wrote:
>> On 4 May 2018 at 01:29, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>>> On Sun, Apr 29, 2018 at 11:35:55AM +0200, Hans de Goede wrote:
>> [...]
>>>> diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst
>>>> index c8bddbdcfd10..560dfed76e38 100644
>>>> --- a/Documentation/driver-api/firmware/request_firmware.rst
>>>> +++ b/Documentation/driver-api/firmware/request_firmware.rst
>>>> @@ -73,3 +73,69 @@ If something went wrong firmware_request() returns non-zero and fw_entry
>>>>   is set to NULL. Once your driver is done with processing the firmware it
>>>>   can call call firmware_release(fw_entry) to release the firmware image
>>>>   and any related resource.
>>>> +
>>>> +EFI embedded firmware support
>>>> +=============================
>>>
>>> This is a new fallback mechanism, please see:
>>>
>>> Documentation/driver-api/firmware/fallback-mechanisms.rst
>>>
>>> Refer to the section "Types of fallback mechanisms", augument the list there
>>> and then move the section "Firmware sysfs loading facility" to a new file, and
>>> then add a new file for your own.
>>>
>>>> +
>>>> +On some devices the system's EFI code / ROM may contain an embedded copy
>>>> +of firmware for some of the system's integrated peripheral devices and
>>>> +the peripheral's Linux device-driver needs to access this firmware.
>>>
>>> You in no way indicate this is a just an invented scheme, a custom solution and
>>> nothing standard.  I realize Ard criticized that the EFI Firmware Volume Protocol
>>> is not part of the UEFI spec -- however it is a bit more widely used right?
>>> Why can't Linux support it instead?
>>>
>>
>> Most implementations of UEFI are based on PI,
> 
> That seems to be the UEFI Platform Initialization specification:
> 
> http://www.uefi.org/sites/default/files/resources/PI_Spec_1_6.pdf
> 
>> and so it is likely that
>> the protocols are available. However, the PI spec does not cover
>> firmware blobs,
> 
> Indeed, I cannot find anything about it on the PI Spec, but I *can* easily
> find a few documents referring to the Firmware Volume Protocol:
> 
> http://wiki.phoenix.com/wiki/index.php/EFI_FIRMWARE_VOLUME_PROTOCOL
> 
> But this has no references at all...
> 
> I see stupid patents over some of this and authentication mechanisms for it:
> 
> https://patents.google.com/patent/US20170098084
> 
>> and so it is undefined whether such blobs are self
>> contained (i.e., in separate files in the firmware volume), statically
>> linked into the driver or maybe even encrypted or otherwise
>> encapsulated, and the actual loadable image only lives in memory.
> 
> Got it, thanks this helps! There are two things then:
> 
>   1) The "EFI Firmware Volume Protocol" ("FV" for short in your descriptions
>      below), and whether to support it or not in the future and recommend it
>      for future use cases.
> 
>   b) Han's EFI scraper to help support 2 drivers, and whether or not to
>      recommend it for future use cases.
> 
>> Hans's case is the second one, i.e., the firmware is at an arbitrary
>> offset in the driver image. Using the FV protocol in this case would
>> result in a mix of both approaches: look up the driver file by GUID
>> [which could change btw between different versions of the system
>> firmware, although this is unlikely] and then still use the prefix/crc
>> based approach to sift through the image itself.
> 
> Got it. And to be clear its a reversed engineered solution to what
> two vendors decided to do.
> 
>> But my main objection is simply that from the UEFI forum point of
>> view, there is a clear distinction between the OS visible interfaces
>> in the UEFI spec and the internal interfaces in the PI spec (which for
>> instance are not subject to the same rules when it comes to backward
>> compatibility), and so I think we should not depend on PI at all.
> 
> Ah I see.
> 
>> This
>> is all the more important considering that we are trying to encourage
>> the creation of other implementations of UEFI that are not based on PI
>> (e.g., uboot for arm64 implements the required UEFI interfaces for
>> booting the kernel via GRUB), and adding dependencies on PI protocols
>> makes that a moving target.
> 
> Got it!
> 
>> So in my view, we either take a ad-hoc approach which works for the
>> few platforms we expect to support, in which case Hans's approach is
>> sufficient,
> 
> Modulo it needs some work for ARM as it only works for x86 right now ;)
> 
>> or we architect it properly, in which case we shouldn't
>> depend on PI because it does not belong in a properly architected
>> OS<->firmware exchange.
> 
> OK, it sounds to me like we have room to then implement our own de-facto
> standard for letting vendors stuff firmware into EFI as we in the Linux
> community see fit.
> 
> We can start out by supporting existing drivers, but also consider customizing
> this in the future for our own needs, so long as we document it and set
> expectations well.
> 
> So we need to support what Hans is implementing for two reasons then:
> 
> a) The FV Protocol cannot be used to support the two drivers he's
>     trying to provide support for -- I believe Hans tried and it didn't work,
>     Hans, correct me if I'm wrong?
> 
> b) The FV Protocol relies on *internal* interfaces of PI spec, and since:
>      1) The PI spec does not define firmware at all
>      2) The internal interfaces of PI Spec does not guarantee any backward
>         compatibility
>     Any implementation details in FV may be subject to change, and may vary
>     system to system. Supporting the FV Protocol would be difficult as it
>     purposely ambiguous.
> 
> If accurate, Hans, can you capture this in your documentation somehow?

Yes I've added some extra doc to this extend for the next version of the
patchset.

Regards,

Hans