Hi All,
Sorry for sending a v3 so soon after v2, I got the property name wrong in the
documentation added in v2 and of course noticed that minutes after sending v2.
This version fixes this.
Here is the v2 coverletter again:
Here is v2 of my patch-set to add support for EFI embedded fw to the kernel.
The 3 most prominent changes 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
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.
Signed-off-by: Hans de Goede <[email protected]>
---
Changes in v2:
-Do not call pr_err on debugfs call failures
---
arch/x86/platform/efi/quirks.c | 4 +++
drivers/firmware/efi/efi.c | 53 ++++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+)
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 5b513ccffde4..0f968c7bcfec 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -374,6 +374,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 cd42f66a7c85..10c896e8b82b 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>
@@ -316,6 +317,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
@@ -360,6 +410,9 @@ static int __init efisubsys_init(void)
goto err_remove_group;
}
+ if (efi_enabled(EFI_DBG))
+ efi_debugfs_init();
+
return 0;
err_remove_group:
--
2.17.0
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.
Signed-off-by: Hans de Goede <[email protected]>
---
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
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.
Signed-off-by: Hans de Goede <[email protected]>
---
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 0d5c55daeeba..99dd47e3b0dd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12618,7 +12618,7 @@ L: [email protected]
L: [email protected]
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 <[email protected]>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 1868aab0282a..b836576f0fe4 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1194,16 +1194,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
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 <[email protected]>
Suggested-by: Peter Jones <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
---
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
Changes in v3:
-Fix the docs using "efi-embedded-fw" as property name instead of
"efi-embedded-firmware"
---
.../driver-api/firmware/request_firmware.rst | 70 +++++++++
drivers/base/firmware_loader/main.c | 33 ++++
drivers/firmware/efi/Kconfig | 6 +
drivers/firmware/efi/Makefile | 1 +
drivers/firmware/efi/embedded-firmware.c | 148 ++++++++++++++++++
include/linux/efi.h | 6 +
include/linux/efi_embedded_fw.h | 25 +++
init/main.c | 1 +
8 files changed, 290 insertions(+)
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 20f21ed427a5..189b02f815c9 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -68,3 +68,73 @@ If something went wrong request_firmware() returns non-zero and fw_entry
is set to NULL. Once your driver is done with processing the firmware it
can call call release_firmware(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.
+
+4. On some systems the embedded-firmware may be accessible through the
+ EFI_FIRMWARE_VOLUME_PROTOCOL if this is the case this may be a better way
+ to access the firmware files.
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index eb34089e4299..1aa42f147415 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -33,6 +33,8 @@
#include <linux/syscore_ops.h>
#include <linux/reboot.h>
#include <linux/security.h>
+#include <linux/efi_embedded_fw.h>
+#include <linux/property.h>
#include <generated/utsrelease.h>
@@ -340,6 +342,28 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
return rc;
}
+#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+static int
+fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret)
+{
+ size_t size;
+ int rc;
+
+ rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size,
+ fw_priv->data ? fw_priv->allocated_size : 0);
+ if (rc == 0) {
+ dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
+ fw_priv->size = size;
+ fw_state_done(fw_priv);
+ ret = 0;
+ } else {
+ dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name);
+ }
+
+ return ret;
+}
+#endif
+
/* firmware holds the ownership of pages */
static void firmware_free_data(const struct firmware *fw)
{
@@ -576,6 +600,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
goto out;
ret = fw_get_filesystem_firmware(device, fw->priv);
+#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+ if (ret && device &&
+ device_property_read_bool(device, "efi-embedded-firmware")) {
+ ret = fw_get_efi_embedded_fw(device, fw->priv, ret);
+ if (ret == 0)
+ ret = assign_fw(fw, device, opt_flags | FW_OPT_NOCACHE);
+ goto out;
+ }
+#endif
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..efb190f5c157 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -164,6 +164,12 @@ config RESET_ATTACK_MITIGATION
have been evicted, since otherwise it will trigger even on clean
reboots.
+config EFI_EMBEDDED_FIRMWARE
+ # This needs boot-services-code to be kept around till after mm_init()
+ # so make this X86 only for now
+ depends on EFI_STUB && X86
+ 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..cb57225a340d
--- /dev/null
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Support for extracting embedded firmware for peripherals from EFI code,
+ *
+ * Copyright (c) 2018 Hans de Goede <[email protected]>
+ */
+
+#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;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f5083aa72eae..3847323ace2f 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1572,6 +1572,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 21efbf6ace93..51dc2981d229 100644
--- a/init/main.c
+++ b/init/main.c
@@ -710,6 +710,7 @@ asmlinkage __visible void __init start_kernel(void)
sfi_init_late();
if (efi_enabled(EFI_RUNTIME_SERVICES)) {
+ efi_check_for_embedded_firmwares();
efi_free_boot_services();
}
--
2.17.0
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.
Signed-off-by: Hans de Goede <[email protected]>
---
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 cb57225a340d..26101ac1a282 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 b836576f0fe4..5bb0f5edd7f2 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1197,6 +1197,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_STUB
---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
On Sun, Apr 8, 2018 at 8:40 PM, Hans de Goede <[email protected]> wrote:
> 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.
>
I have no objections from my side, though consider the following:
- I would like to be in sync with Darren on this
- make oldconfig will be broken after your change for existing users
- the usual pattern in kernel that we don't rename drivers; I guess
here we are on the safe side b/c this driver is used standalone
Taking above into attention, and assuming it will go via some other tree,
Acked-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> 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 0d5c55daeeba..99dd47e3b0dd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12618,7 +12618,7 @@ L: [email protected]
> L: [email protected]
> 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 <[email protected]>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 1868aab0282a..b836576f0fe4 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1194,16 +1194,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
>
--
With Best Regards,
Andy Shevchenko
On Sun, Apr 8, 2018 at 8:40 PM, Hans de Goede <[email protected]> wrote:
> 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 <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> 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
>
--
With Best Regards,
Andy Shevchenko
Hallo Hans,
On 8 April 2018 at 19:40, Hans de Goede <[email protected]> wrote:
> 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.
>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> Changes in v2:
> -Do not call pr_err on debugfs call failures
> ---
> arch/x86/platform/efi/quirks.c | 4 +++
> drivers/firmware/efi/efi.c | 53 ++++++++++++++++++++++++++++++++++
> 2 files changed, 57 insertions(+)
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 5b513ccffde4..0f968c7bcfec 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -374,6 +374,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 cd42f66a7c85..10c896e8b82b 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>
> @@ -316,6 +317,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
> @@ -360,6 +410,9 @@ static int __init efisubsys_init(void)
> goto err_remove_group;
> }
>
> + if (efi_enabled(EFI_DBG))
> + efi_debugfs_init();
> +
This doesn't really make any sense on non-x86. The boot services
regions are released to the kernel for general allocation, and so
exposing them this way only makes sense if you keep them as you do for
x86.
Could you please try to make this call specific to situations where it
makes sense? I don't mind allocating a new EFI_xxx flag for preserving
the boot services regions so we could decide to set it for ARM/arm64
as well in certain cases in the future.
> return 0;
>
> err_remove_group:
> --
> 2.17.0
>
On 8 April 2018 at 19:40, Hans de Goede <[email protected]> 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 <[email protected]>
> Suggested-by: Peter Jones <[email protected]>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> 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
>
> Changes in v3:
> -Fix the docs using "efi-embedded-fw" as property name instead of
> "efi-embedded-firmware"
> ---
> .../driver-api/firmware/request_firmware.rst | 70 +++++++++
> drivers/base/firmware_loader/main.c | 33 ++++
> drivers/firmware/efi/Kconfig | 6 +
> drivers/firmware/efi/Makefile | 1 +
> drivers/firmware/efi/embedded-firmware.c | 148 ++++++++++++++++++
> include/linux/efi.h | 6 +
> include/linux/efi_embedded_fw.h | 25 +++
> init/main.c | 1 +
> 8 files changed, 290 insertions(+)
> 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 20f21ed427a5..189b02f815c9 100644
> --- a/Documentation/driver-api/firmware/request_firmware.rst
> +++ b/Documentation/driver-api/firmware/request_firmware.rst
> @@ -68,3 +68,73 @@ If something went wrong request_firmware() returns non-zero and fw_entry
> is set to NULL. Once your driver is done with processing the firmware it
> can call call release_firmware(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.
> +
> +4. On some systems the embedded-firmware may be accessible through the
> + EFI_FIRMWARE_VOLUME_PROTOCOL if this is the case this may be a better way
> + to access the firmware files.
EFI_FIRMWARE_VOLUME_PROTOCOL is not a UEFI protocol, so I'd prefer it
if we just drop this point altogether.
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index eb34089e4299..1aa42f147415 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -33,6 +33,8 @@
> #include <linux/syscore_ops.h>
> #include <linux/reboot.h>
> #include <linux/security.h>
> +#include <linux/efi_embedded_fw.h>
> +#include <linux/property.h>
>
> #include <generated/utsrelease.h>
>
> @@ -340,6 +342,28 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
> return rc;
> }
>
> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
> +static int
> +fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret)
> +{
> + size_t size;
> + int rc;
> +
> + rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size,
> + fw_priv->data ? fw_priv->allocated_size : 0);
> + if (rc == 0) {
> + dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
> + fw_priv->size = size;
> + fw_state_done(fw_priv);
> + ret = 0;
> + } else {
> + dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name);
> + }
> +
> + return ret;
> +}
> +#endif
> +
> /* firmware holds the ownership of pages */
> static void firmware_free_data(const struct firmware *fw)
> {
> @@ -576,6 +600,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> goto out;
>
> ret = fw_get_filesystem_firmware(device, fw->priv);
> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
> + if (ret && device &&
> + device_property_read_bool(device, "efi-embedded-firmware")) {
> + ret = fw_get_efi_embedded_fw(device, fw->priv, ret);
> + if (ret == 0)
> + ret = assign_fw(fw, device, opt_flags | FW_OPT_NOCACHE);
> + goto out;
> + }
> +#endif
> 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..efb190f5c157 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -164,6 +164,12 @@ config RESET_ATTACK_MITIGATION
> have been evicted, since otherwise it will trigger even on clean
> reboots.
>
> +config EFI_EMBEDDED_FIRMWARE
> + # This needs boot-services-code to be kept around till after mm_init()
> + # so make this X86 only for now
> + depends on EFI_STUB && X86
> + 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..cb57225a340d
> --- /dev/null
> +++ b/drivers/firmware/efi/embedded-firmware.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Support for extracting embedded firmware for peripherals from EFI code,
> + *
> + * Copyright (c) 2018 Hans de Goede <[email protected]>
> + */
> +
> +#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;
> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index f5083aa72eae..3847323ace2f 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1572,6 +1572,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 21efbf6ace93..51dc2981d229 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -710,6 +710,7 @@ asmlinkage __visible void __init start_kernel(void)
> sfi_init_late();
>
> if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> + efi_check_for_embedded_firmwares();
Should this depend on EFI runtime services? I don't see why it should
matter whether you have access to variables or the EFI rtc services.
If the regions are not reserved in the first place in that case, I'd
rather fix that properly. (see comment re new EFI_xxx flag in previous
mail)
> efi_free_boot_services();
> }
>
> --
> 2.17.0
>
On Sun, Apr 08, 2018 at 07:40:11PM +0200, Hans de Goede wrote:
> static void firmware_free_data(const struct firmware *fw)
> {
> @@ -576,6 +600,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> goto out;
>
> ret = fw_get_filesystem_firmware(device, fw->priv);
> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
> + if (ret && device &&
> + device_property_read_bool(device, "efi-embedded-firmware")) {
> + ret = fw_get_efi_embedded_fw(device, fw->priv, ret);
> + if (ret == 0)
> + ret = assign_fw(fw, device, opt_flags | FW_OPT_NOCACHE);
> + goto out;
> + }
> +#endif
You mussed what I asked for in terms of adding a new flag, (please work on top
of Andre's patches as those likely will be merged first, and also have kdocs
for the flags) and then a new firmware API to wrap the above into a function
which would only do something if the driver *asked* for it on their firmware
API call. Ie, please add a new firmware_request_efi_fw(). Also if you see the
work I've done to remove the ifdefs over fallback mechanism you'll see it helps
split code and make it easier to read. We should strive to not add any more
ifdefery and instead make tehis code read easily.
Luis
Hi,
On 17-04-18 02:17, Luis R. Rodriguez wrote:
> On Sun, Apr 08, 2018 at 07:40:11PM +0200, Hans de Goede wrote:
>> static void firmware_free_data(const struct firmware *fw)
>> {
>> @@ -576,6 +600,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>> goto out;
>>
>> ret = fw_get_filesystem_firmware(device, fw->priv);
>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
>> + if (ret && device &&
>> + device_property_read_bool(device, "efi-embedded-firmware")) {
>> + ret = fw_get_efi_embedded_fw(device, fw->priv, ret);
>> + if (ret == 0)
>> + ret = assign_fw(fw, device, opt_flags | FW_OPT_NOCACHE);
>> + goto out;
>> + }
>> +#endif
>
> You mussed what I asked for in terms of adding a new flag, (please work on top
> of Andre's patches as those likely will be merged first, and also have kdocs
> for the flags)
Ok I will base my next version on top of Andres' series.
> and then a new firmware API to wrap the above into a function
> which would only do something if the driver *asked* for it on their firmware
> API call.
> Ie, please add a new firmware_request_efi_fw().
As I tried to explain in the changelog the problem with doing this, is that
this makes it a driver decision, where it really needs to be platform-code driven,
not driver driven.
Take for example the drivers/input/touchscreen/silead.c code that is used on
a lot of 32 bit ARM platforms too, which don't have EFI at all, so if that
needs to call request_firmware_efi() then should I add:
#ifdef CONFIG_X86
fw = request_firmware_efi(...);
#else
fw = request_firmware(...);
#endif
? But even on x86 only some devices with a silead touchscreen have EFI
embedded firmware, so then I would need something like:
#ifdef CONFIG_X86
if (device_property_get_bool(dev, "some-prop-name"))
fw = request_firmware_efi(...);
else
#else
fw = request_firmware(...);
#endif
That is assuming I still want the normal fallback path in the
case no EFI firmware is available, which I do because then
something like packagekit may see if the firmware is packaged
in one of the configured distro repositories.
We already have (x86) platform code in place to attach
properties (like a board specific firmware filename) to the
device using device-properties so that drivers like silead.c
don't get filled / polluted with board/platform specific knowledge,
which IMHO is the place where the knowledge fallback to
an EFI embedded firmware copy belongs.
As the further patches in v3 of this series shows, this actually
works quite nicely, because this also allows bundling the
EFI-embedded firmware info (prefix, length, crc, name) together
with the other board specific properties.
TL;DR: using request_firmware_efi() vs request_firmware() is
a driver decision, but whether EFI firmware fallback should be
is board/platform specific not driver specific, therefor I
believe that using a device-property to signal this is better.
If you insist on me adding a request_firmware_efi() I can give
this a shot, but I know that Dmitry (the input maintainer) will
very much dislike the silead.c changes that implies...
Still a question for lets sat we go that route, what do we
then do with request_firmware_efi() when CONFIG_EFI is not set ?
Should it be defined then or not, and if it should be defined
when CONFIG_EFI is not set what should it do then?
> Also if you see the
> work I've done to remove the ifdefs over fallback mechanism you'll see it helps
> split code and make it easier to read. We should strive to not add any more
> ifdefery and instead make tehis code read easily.
So looking at how the CONFIG_FW_LOADER_USER_HELPER stuff deals
with this, I should:
1) Move the definition of fw_get_efi_embedded_fw() to a new
drivers/base/firmware_loader/fallback_efi.c,
which only gets build if CONFIG_EFI_EMBEDDED_FIRMWARE is set
2) Put the following in fallback.h:
#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret);
#else
static inline int
fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret)
{
return ret;
}
#endif
have I got that right?
Regards,
Hans
Hi,
On 17-04-18 02:17, Luis R. Rodriguez wrote:
> On Sun, Apr 08, 2018 at 07:40:11PM +0200, Hans de Goede wrote:
>> static void firmware_free_data(const struct firmware *fw)
>> {
>> @@ -576,6 +600,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>> goto out;
>>
>> ret = fw_get_filesystem_firmware(device, fw->priv);
>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
>> + if (ret && device &&
>> + device_property_read_bool(device, "efi-embedded-firmware")) {
>> + ret = fw_get_efi_embedded_fw(device, fw->priv, ret);
>> + if (ret == 0)
>> + ret = assign_fw(fw, device, opt_flags | FW_OPT_NOCACHE);
>> + goto out;
>> + }
>> +#endif
>
So thinking some more about this, I can put the device_property check
inside the fw_get_efi_embedded_fw() call, as well as modify opt_flags
there to or in FW_OPT_NOCACHE on success, then together with the discussed
changed to drop the #ifdef, the code would look like this:
ret = fw_get_filesystem_firmware(device, fw->priv);
if (ret)
fw_get_efi_embedded_fw(device, fw->priv, &opt_flags, ret);
if (ret)
if (!(opt_flags & FW_OPT_NO_WARN))
dev_warn(device,
...
With just these 2 lines being new:
if (ret)
fw_get_efi_embedded_fw(device, fw->priv, &opt_flags, ret);
So the main.c changes will be nice and clean then.
Regards,
Hans
On Mon, Apr 09, 2018 at 11:07:03AM +0300, Andy Shevchenko wrote:
> On Sun, Apr 8, 2018 at 8:40 PM, Hans de Goede <[email protected]> wrote:
> > 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.
> >
>
> I have no objections from my side, though consider the following:
> - I would like to be in sync with Darren on this
> - make oldconfig will be broken after your change for existing users
> - the usual pattern in kernel that we don't rename drivers; I guess
> here we are on the safe side b/c this driver is used standalone
>
> Taking above into attention, and assuming it will go via some other tree,
> Acked-by: Andy Shevchenko <[email protected]>
This driver is all kinds of a special case, so no objection here either. :-)
--
Darren Hart
VMware Open Source Technology Center
On Sun, Apr 08, 2018 at 07:40:10PM +0200, Hans de Goede wrote:
> 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.
>
> Signed-off-by: Hans de Goede <[email protected]>
For the debugfs interaction:
Reviewed-by: Greg Kroah-Hartman <[email protected]>
Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new ID
and security for this type of request so IMA can reject it if the policy is
configured for it.
Please Cc Kees in future patches.
Luis
Hi,
On 16-04-18 10:23, Ard Biesheuvel wrote:
> Hallo Hans,
>
> On 8 April 2018 at 19:40, Hans de Goede <[email protected]> wrote:
>> 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.
>>
>> Signed-off-by: Hans de Goede <[email protected]>
>> ---
>> Changes in v2:
>> -Do not call pr_err on debugfs call failures
>> ---
>> arch/x86/platform/efi/quirks.c | 4 +++
>> drivers/firmware/efi/efi.c | 53 ++++++++++++++++++++++++++++++++++
>> 2 files changed, 57 insertions(+)
>>
>> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
>> index 5b513ccffde4..0f968c7bcfec 100644
>> --- a/arch/x86/platform/efi/quirks.c
>> +++ b/arch/x86/platform/efi/quirks.c
>> @@ -374,6 +374,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 cd42f66a7c85..10c896e8b82b 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>
>> @@ -316,6 +317,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
>> @@ -360,6 +410,9 @@ static int __init efisubsys_init(void)
>> goto err_remove_group;
>> }
>>
>> + if (efi_enabled(EFI_DBG))
>> + efi_debugfs_init();
>> +
>
> This doesn't really make any sense on non-x86. The boot services
> regions are released to the kernel for general allocation, and so
> exposing them this way only makes sense if you keep them as you do for
> x86.
>
> Could you please try to make this call specific to situations where it
> makes sense? I don't mind allocating a new EFI_xxx flag for preserving
> the boot services regions so we could decide to set it for ARM/arm64
> as well in certain cases in the future.
Ok, I've added a new EFI_BOOT_SERVICES flag for this for v4 of this
patchset, mirroring the existing EFI_RUNTIME_SERVICES flag in naming.
Regards,
Hans
Hi,
On 16-04-18 10:28, Ard Biesheuvel wrote:
> On 8 April 2018 at 19:40, Hans de Goede <[email protected]> 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 <[email protected]>
>> Suggested-by: Peter Jones <[email protected]>
>> Signed-off-by: Hans de Goede <[email protected]>
>> ---
>> 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
>>
>> Changes in v3:
>> -Fix the docs using "efi-embedded-fw" as property name instead of
>> "efi-embedded-firmware"
>> ---
>> .../driver-api/firmware/request_firmware.rst | 70 +++++++++
>> drivers/base/firmware_loader/main.c | 33 ++++
>> drivers/firmware/efi/Kconfig | 6 +
>> drivers/firmware/efi/Makefile | 1 +
>> drivers/firmware/efi/embedded-firmware.c | 148 ++++++++++++++++++
>> include/linux/efi.h | 6 +
>> include/linux/efi_embedded_fw.h | 25 +++
>> init/main.c | 1 +
>> 8 files changed, 290 insertions(+)
>> 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 20f21ed427a5..189b02f815c9 100644
>> --- a/Documentation/driver-api/firmware/request_firmware.rst
>> +++ b/Documentation/driver-api/firmware/request_firmware.rst
>> @@ -68,3 +68,73 @@ If something went wrong request_firmware() returns non-zero and fw_entry
>> is set to NULL. Once your driver is done with processing the firmware it
>> can call call release_firmware(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.
>> +
>> +4. On some systems the embedded-firmware may be accessible through the
>> + EFI_FIRMWARE_VOLUME_PROTOCOL if this is the case this may be a better way
>> + to access the firmware files.
>
> EFI_FIRMWARE_VOLUME_PROTOCOL is not a UEFI protocol, so I'd prefer it
> if we just drop this point altogether.
Hmm, there have been requests for me to document this because it might be
an interesting approach in some cases, anyways dropped for v4.
>> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
>> index eb34089e4299..1aa42f147415 100644
>> --- a/drivers/base/firmware_loader/main.c
>> +++ b/drivers/base/firmware_loader/main.c
>> @@ -33,6 +33,8 @@
>> #include <linux/syscore_ops.h>
>> #include <linux/reboot.h>
>> #include <linux/security.h>
>> +#include <linux/efi_embedded_fw.h>
>> +#include <linux/property.h>
>>
>> #include <generated/utsrelease.h>
>>
>> @@ -340,6 +342,28 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
>> return rc;
>> }
>>
>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
>> +static int
>> +fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv, int ret)
>> +{
>> + size_t size;
>> + int rc;
>> +
>> + rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size,
>> + fw_priv->data ? fw_priv->allocated_size : 0);
>> + if (rc == 0) {
>> + dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
>> + fw_priv->size = size;
>> + fw_state_done(fw_priv);
>> + ret = 0;
>> + } else {
>> + dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name);
>> + }
>> +
>> + return ret;
>> +}
>> +#endif
>> +
>> /* firmware holds the ownership of pages */
>> static void firmware_free_data(const struct firmware *fw)
>> {
>> @@ -576,6 +600,15 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>> goto out;
>>
>> ret = fw_get_filesystem_firmware(device, fw->priv);
>> +#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
>> + if (ret && device &&
>> + device_property_read_bool(device, "efi-embedded-firmware")) {
>> + ret = fw_get_efi_embedded_fw(device, fw->priv, ret);
>> + if (ret == 0)
>> + ret = assign_fw(fw, device, opt_flags | FW_OPT_NOCACHE);
>> + goto out;
>> + }
>> +#endif
>> 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..efb190f5c157 100644
>> --- a/drivers/firmware/efi/Kconfig
>> +++ b/drivers/firmware/efi/Kconfig
>> @@ -164,6 +164,12 @@ config RESET_ATTACK_MITIGATION
>> have been evicted, since otherwise it will trigger even on clean
>> reboots.
>>
>> +config EFI_EMBEDDED_FIRMWARE
>> + # This needs boot-services-code to be kept around till after mm_init()
>> + # so make this X86 only for now
>> + depends on EFI_STUB && X86
>> + 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..cb57225a340d
>> --- /dev/null
>> +++ b/drivers/firmware/efi/embedded-firmware.c
>> @@ -0,0 +1,148 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Support for extracting embedded firmware for peripherals from EFI code,
>> + *
>> + * Copyright (c) 2018 Hans de Goede <[email protected]>
>> + */
>> +
>> +#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;
>> +}
>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>> index f5083aa72eae..3847323ace2f 100644
>> --- a/include/linux/efi.h
>> +++ b/include/linux/efi.h
>> @@ -1572,6 +1572,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 21efbf6ace93..51dc2981d229 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -710,6 +710,7 @@ asmlinkage __visible void __init start_kernel(void)
>> sfi_init_late();
>>
>> if (efi_enabled(EFI_RUNTIME_SERVICES)) {
>> + efi_check_for_embedded_firmwares();
>
> Should this depend on EFI runtime services? I don't see why it should
> matter whether you have access to variables or the EFI rtc services.
>
> If the regions are not reserved in the first place in that case, I'd
> rather fix that properly. (see comment re new EFI_xxx flag in previous
> mail)
Ok, I've put this under an "if (efi_enabled(EFI_BOOT_SERVICES))"
check for v4.
Regards,
Hans
Hi,
On 23-04-18 23:11, Luis R. Rodriguez wrote:
> Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new ID
> and security for this type of request so IMA can reject it if the policy is
> configured for it.
Hmm, interesting, actually it seems like the whole existence
of READING_FIRMWARE_PREALLOC_BUFFER is a mistake, the IMA
framework really does not care if we are loading the firmware
into memory allocated by the firmware-loader code, or into
memory allocated by the device-driver requesting the firmware.
As such the current IMA code (from v4.17-rc2) actually does
not handle READING_FIRMWARE_PREALLOC_BUFFER at all, here
are bits of code from: security/integrity/ima/ima_main.c:
static int read_idmap[READING_MAX_ID] = {
[READING_FIRMWARE] = FIRMWARE_CHECK,
[READING_MODULE] = MODULE_CHECK,
[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
[READING_POLICY] = POLICY_CHECK
};
int ima_post_read_file(struct file *file, void *buf, loff_t size,
...
if (!file && read_id == READING_FIRMWARE) {
if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
(ima_appraise & IMA_APPRAISE_ENFORCE))
return -EACCES; /* INTEGRITY_UNKNOWN */
return 0;
}
Which show that the IMA code is not handling
READING_FIRMWARE_PREALLOC_BUFFER as it should (I believe it
should handle it the same as READING_FIRMWARE).
Now we could fix that, but the only user of
READING_FIRMWARE_PREALLOC_BUFFER is the code which originally
introduced it:
https://patchwork.kernel.org/patch/9162011/
So I believe it might be better to instead replace it
with just READING_FIRMWARE and find another way to tell
kernel_read_file() that there is a pre-allocated buffer,
perhaps the easiest way there is that *buf must be
NULL when the caller wants kernel_read_file() to
vmalloc the mem. This would of course require auditing
all callers that the buf which the pass in is initialized
to NULL.
Either way adding a third READING_FIRMWARE_FOO to the
kernel_read_file_id enum seems like a bad idea, from
the IMA pov firmware is firmware.
What this whole exercise has shown me though is that
I need to call security_kernel_post_read_file() when
loading EFI embedded firmware. I will add a call to
security_kernel_post_read_file() for v4 of the patch-set.
> Please Cc Kees in future patches.
Will do.
Regards,
Hans
On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
> Hi,
>
> On 23-04-18 23:11, Luis R. Rodriguez wrote:
> > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new ID
> > and security for this type of request so IMA can reject it if the policy is
> > configured for it.
>
> Hmm, interesting, actually it seems like the whole existence
> of READING_FIRMWARE_PREALLOC_BUFFER is a mistake, the IMA
> framework really does not care if we are loading the firmware
> into memory allocated by the firmware-loader code, or into
> memory allocated by the device-driver requesting the firmware.
>
> As such the current IMA code (from v4.17-rc2) actually does
> not handle READING_FIRMWARE_PREALLOC_BUFFER at all,
Right, it doesn't yet address READING_FIRMWARE_PREALLOC_BUFFER, but
should.
Depending on whether the device requesting the firmware has access to
the DMA memory, before the signature verification, will determine how
IMA-appraisal addresses READING_FIRMWARE_PREALLOC_BUFFER.
Mimi
> here
> are bits of code from: security/integrity/ima/ima_main.c:
>
> static int read_idmap[READING_MAX_ID] = {
> [READING_FIRMWARE] = FIRMWARE_CHECK,
> [READING_MODULE] = MODULE_CHECK,
> [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
> [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
> [READING_POLICY] = POLICY_CHECK
> };
>
> int ima_post_read_file(struct file *file, void *buf, loff_t size,
> ...
> if (!file && read_id == READING_FIRMWARE) {
> if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> (ima_appraise & IMA_APPRAISE_ENFORCE))
> return -EACCES; /* INTEGRITY_UNKNOWN */
> return 0;
> }
>
> Which show that the IMA code is not handling
> READING_FIRMWARE_PREALLOC_BUFFER as it should (I believe it
> should handle it the same as READING_FIRMWARE).
>
> Now we could fix that, but the only user of
> READING_FIRMWARE_PREALLOC_BUFFER is the code which originally
> introduced it:
>
> https://patchwork.kernel.org/patch/9162011/
>
> So I believe it might be better to instead replace it
> with just READING_FIRMWARE and find another way to tell
> kernel_read_file() that there is a pre-allocated buffer,
> perhaps the easiest way there is that *buf must be
> NULL when the caller wants kernel_read_file() to
> vmalloc the mem. This would of course require auditing
> all callers that the buf which the pass in is initialized
> to NULL.
>
> Either way adding a third READING_FIRMWARE_FOO to the
> kernel_read_file_id enum seems like a bad idea, from
> the IMA pov firmware is firmware.
>
> What this whole exercise has shown me though is that
> I need to call security_kernel_post_read_file() when
> loading EFI embedded firmware. I will add a call to
> security_kernel_post_read_file() for v4 of the patch-set.
>
> > Please Cc Kees in future patches.
>
> Will do.
>
> Regards,
>
> Hans
>
Hi,
On 24-04-18 18:07, Mimi Zohar wrote:
> On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 23-04-18 23:11, Luis R. Rodriguez wrote:
>>> Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new ID
>>> and security for this type of request so IMA can reject it if the policy is
>>> configured for it.
>>
>> Hmm, interesting, actually it seems like the whole existence
>> of READING_FIRMWARE_PREALLOC_BUFFER is a mistake, the IMA
>> framework really does not care if we are loading the firmware
>> into memory allocated by the firmware-loader code, or into
>> memory allocated by the device-driver requesting the firmware.
>>
>> As such the current IMA code (from v4.17-rc2) actually does
>> not handle READING_FIRMWARE_PREALLOC_BUFFER at all,
>
> Right, it doesn't yet address READING_FIRMWARE_PREALLOC_BUFFER, but
> should.
>
> Depending on whether the device requesting the firmware has access to
> the DMA memory, before the signature verification, will determine how
> IMA-appraisal addresses READING_FIRMWARE_PREALLOC_BUFFER.
Ah I see. So this probably means that the IMA integration for
my EFI embedded firmware code should also pass READING_FIRMWARE or
READING_FIRMWARE_PREALLOC_BUFFER depending on if a pre-allocated
buffer is used.
Hmm, the security_kernel_post_read_file() call in
drivers/base/firmware_loader/fallback.c
Unconditionally passes READING_FIRMWARE, it should probably check
fw_priv->is_paged_buf and base the id to pass on that.
And yes it is possible AFAICT for the firmware_request_into_buf()
method to fallback to the userspace helper, this can happen if the
fw_fallback_config.force_sysfs_fallback flag is set.
Regards,
Hans
On Tue, Apr 24, 2018 at 12:07:01PM -0400, Mimi Zohar wrote:
> On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
> > Hi,
> >
> > On 23-04-18 23:11, Luis R. Rodriguez wrote:
> > > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new ID
> > > and security for this type of request so IMA can reject it if the policy is
> > > configured for it.
> >
> > Hmm, interesting, actually it seems like the whole existence
> > of READING_FIRMWARE_PREALLOC_BUFFER is a mistake,
request_firmware_into_buf() was merged without my own review, however,
the ID thing did get review from Mimi:
https://patchwork.kernel.org/patch/9074611/
The ID is not for IMA alone, its for any LSM to decide what to do.
Note Mimi asked for READING_FIRMWARE_DMA if such buffer was in DMA,
otherise READING_FIRMWARE_PREALLOC_BUFFER was suggested.
> > the IMA
> > framework really does not care if we are loading the firmware
> > into memory allocated by the firmware-loader code, or into
> > memory allocated by the device-driver requesting the firmware.
That's up to LSM folks to decide. We have these so far:
#define __kernel_read_file_id(id) \
id(UNKNOWN, unknown) \
id(FIRMWARE, firmware) \
id(FIRMWARE_PREALLOC_BUFFER, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image) \
id(KEXEC_INITRAMFS, kexec-initramfs) \
id(POLICY, security-policy) \
id(X509_CERTIFICATE, x509-certificate) \
id(MAX_ID, )
The first type of IDs added was about type of files the kernel
LSMs may want to do different things for.
Mimi why did you want a separate ID for it back before?
I should note now that request_firmware_into_buf() and its
READING_FIRMWARE_PREALLOC_BUFFER was to enable a driver on memory constrained
devices. The files are large (commit says 16 MiB).
I've heard of larger possible files with remoteproc and with Android using
the custom fallback mechanism -- which could mean a proprietary tool
fetching firmware from a random special place on a device.
I could perhaps imagine an LSM which may be aware of such type of
arrangement may want to do its own vetting of some sort, but this
would not be specific to READING_FIRMWARE_PREALLOC_BUFFER, but rather
the custom fallback mechaism.
Whether or not the buffer was preallocated by the driver seems a little
odd for security folks to do something different with it. Security LSM
folks please chime in.
I could see a bit more of a use case for an ID for firmware scraped
from EFI, which Hans' patch will provide. But that *also* should get
good review from other LSM folks.
One of the issues with accepting more IDs loosely is where do we
stop though? If no one really is using READING_FIRMWARE_PREALLOC_BUFFER
I'd say lets remove it. Likewise, for this EFI thing I'd like an idea
if we really are going to have users for it.
If its of any help --
drivers/soc/qcom/mdt_loader.c is the only driver currently using
request_firmware_into_buf() however I'll note qcom_mdt_load() is used in many
other drivers so they are wrappers around request_firmware_into_buf():
drivers/gpu/drm/msm/adreno/a5xx_gpu.c: * adreno_request_fw() handles this, but qcom_mdt_load() does
drivers/gpu/drm/msm/adreno/a5xx_gpu.c: ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID,
drivers/gpu/drm/msm/adreno/a5xx_gpu.c: ret = qcom_mdt_load(dev, fw, newname, GPU_PAS_ID,
drivers/media/platform/qcom/venus/firmware.c: ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
drivers/remoteproc/qcom_adsp_pil.c: return qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id,
drivers/remoteproc/qcom_wcnss.c: return qcom_mdt_load(wcnss->dev, fw, rproc->firmware, WCNSS_PAS_ID,
Are we going to add more IDs for more types of firmware?
What type of *different* decisions could LSMs take if the firmware
was being written to a buffer? Or in this new case that is coming
up, if the file came scraping EFI, would having that information
be useful?
> > As such the current IMA code (from v4.17-rc2) actually does
> > not handle READING_FIRMWARE_PREALLOC_BUFFER at all,
>
> Right, it doesn't yet address READING_FIRMWARE_PREALLOC_BUFFER, but
> should.
>
> Depending on whether the device requesting the firmware has access to
> the DMA memory, before the signature verification,
It would seem from the original patch review about READING_FIRMWARE_PREALLOC_BUFFER
that this is not a DMA buffer.
The device driver should have access to the buffer pointer with write given
that with request_firmware_into_buf() the driver is giving full write access to
the memory pointer so that the firmware API can stuff the firmware it finds
there.
Firmware signature verification would be up to the device hardware to do upon
load *after* request_firmware_into_buf().
Luis
> will determine how
> IMA-appraisal addresses READING_FIRMWARE_PREALLOC_BUFFER.
>
> Mimi
>
> > here
> > are bits of code from: security/integrity/ima/ima_main.c:
> >
> > static int read_idmap[READING_MAX_ID] = {
> > [READING_FIRMWARE] = FIRMWARE_CHECK,
> > [READING_MODULE] = MODULE_CHECK,
> > [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
> > [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
> > [READING_POLICY] = POLICY_CHECK
> > };
> >
> > int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > ...
> > if (!file && read_id == READING_FIRMWARE) {
> > if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> > (ima_appraise & IMA_APPRAISE_ENFORCE))
> > return -EACCES; /* INTEGRITY_UNKNOWN */
> > return 0;
> > }
> >
> > Which show that the IMA code is not handling
> > READING_FIRMWARE_PREALLOC_BUFFER as it should (I believe it
> > should handle it the same as READING_FIRMWARE).
> >
> > Now we could fix that, but the only user of
> > READING_FIRMWARE_PREALLOC_BUFFER is the code which originally
> > introduced it:
> >
> > https://patchwork.kernel.org/patch/9162011/
> >
> > So I believe it might be better to instead replace it
> > with just READING_FIRMWARE and find another way to tell
> > kernel_read_file() that there is a pre-allocated buffer,
> > perhaps the easiest way there is that *buf must be
> > NULL when the caller wants kernel_read_file() to
> > vmalloc the mem. This would of course require auditing
> > all callers that the buf which the pass in is initialized
> > to NULL.
> >
> > Either way adding a third READING_FIRMWARE_FOO to the
> > kernel_read_file_id enum seems like a bad idea, from
> > the IMA pov firmware is firmware.
> >
> > What this whole exercise has shown me though is that
> > I need to call security_kernel_post_read_file() when
> > loading EFI embedded firmware. I will add a call to
> > security_kernel_post_read_file() for v4 of the patch-set.
> >
> > > Please Cc Kees in future patches.
> >
> > Will do.
> >
> > Regards,
> >
> > Hans
> >
>
>
--
Do not panic
On Tue, 2018-04-24 at 23:42 +0000, Luis R. Rodriguez wrote:
> On Tue, Apr 24, 2018 at 12:07:01PM -0400, Mimi Zohar wrote:
> > On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
> > > Hi,
> > >
> > > On 23-04-18 23:11, Luis R. Rodriguez wrote:
> > > > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new ID
> > > > and security for this type of request so IMA can reject it if the policy is
> > > > configured for it.
> > >
> > > Hmm, interesting, actually it seems like the whole existence
> > > of READING_FIRMWARE_PREALLOC_BUFFER is a mistake,
>
> request_firmware_into_buf() was merged without my own review, however,
> the ID thing did get review from Mimi:
>
> https://patchwork.kernel.org/patch/9074611/
>
> The ID is not for IMA alone, its for any LSM to decide what to do.
> Note Mimi asked for READING_FIRMWARE_DMA if such buffer was in DMA,
> otherise READING_FIRMWARE_PREALLOC_BUFFER was suggested.
>
> > > the IMA
> > > framework really does not care if we are loading the firmware
> > > into memory allocated by the firmware-loader code, or into
> > > memory allocated by the device-driver requesting the firmware.
>
> That's up to LSM folks to decide. We have these so far:
>
> #define __kernel_read_file_id(id) \
> id(UNKNOWN, unknown) \
> id(FIRMWARE, firmware) \
> id(FIRMWARE_PREALLOC_BUFFER, firmware) \
> id(MODULE, kernel-module) \
> id(KEXEC_IMAGE, kexec-image) \
> id(KEXEC_INITRAMFS, kexec-initramfs) \
> id(POLICY, security-policy) \
> id(X509_CERTIFICATE, x509-certificate) \
> id(MAX_ID, )
>
> The first type of IDs added was about type of files the kernel
> LSMs may want to do different things for.
>
> Mimi why did you want a separate ID for it back before?
The point of commit a098ecd2fa7d ("firmware: support loading into a
pre-allocated buffer") is to avoid reading the firmware into kernel
memory and then copying it "to it's final resting place". Â My concern
is that if the device driver has access to the buffer, it could access
the buffer prior to the firmware's signature having been verified by
the kernel.
In tightly controlled environments interested in limiting which signed
firmware version is loaded, require's the device driver not having
access to the buffer until after the signature has been verified by
the kernel (eg. IMA-appraisal).
>
> I should note now that request_firmware_into_buf() and its
> READING_FIRMWARE_PREALLOC_BUFFER was to enable a driver on memory constrained
> devices. The files are large (commit says 16 MiB).
>
> I've heard of larger possible files with remoteproc and with Android using
> the custom fallback mechanism -- which could mean a proprietary tool
> fetching firmware from a random special place on a device.
>
> I could perhaps imagine an LSM which may be aware of such type of
> arrangement may want to do its own vetting of some sort, but this
> would not be specific to READING_FIRMWARE_PREALLOC_BUFFER, but rather
> the custom fallback mechaism.
>
> Whether or not the buffer was preallocated by the driver seems a little
> odd for security folks to do something different with it. Security LSM
> folks please chime in.
>
> I could see a bit more of a use case for an ID for firmware scraped
> from EFI, which Hans' patch will provide. But that *also* should get
> good review from other LSM folks.
>
> One of the issues with accepting more IDs loosely is where do we
> stop though? If no one really is using READING_FIRMWARE_PREALLOC_BUFFER
> I'd say lets remove it. Likewise, for this EFI thing I'd like an idea
> if we really are going to have users for it.
>
> If its of any help --
>
> drivers/soc/qcom/mdt_loader.c is the only driver currently using
> request_firmware_into_buf() however I'll note qcom_mdt_load() is used in many
> other drivers so they are wrappers around request_firmware_into_buf():
>
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c: * adreno_request_fw() handles this, but qcom_mdt_load() does
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c: ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID,
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c: ret = qcom_mdt_load(dev, fw, newname, GPU_PAS_ID,
> drivers/media/platform/qcom/venus/firmware.c: ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
> drivers/remoteproc/qcom_adsp_pil.c: return qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id,
> drivers/remoteproc/qcom_wcnss.c: return qcom_mdt_load(wcnss->dev, fw, rproc->firmware, WCNSS_PAS_ID,
>
> Are we going to add more IDs for more types of firmware?
> What type of *different* decisions could LSMs take if the firmware
> was being written to a buffer? Or in this new case that is coming
> up, if the file came scraping EFI, would having that information
> be useful?
>
> > > As such the current IMA code (from v4.17-rc2) actually does
> > > not handle READING_FIRMWARE_PREALLOC_BUFFER at all,
> >
> > Right, it doesn't yet address READING_FIRMWARE_PREALLOC_BUFFER, but
> > should.
> >
> > Depending on whether the device requesting the firmware has access to
> > the DMA memory, before the signature verification,
>
> It would seem from the original patch review about READING_FIRMWARE_PREALLOC_BUFFER
> that this is not a DMA buffer.
The call sequence:
qcom_mdt_load() ->Â qcom_scm_pas_init_image() ->Â dma_alloc_coherent()
If dma_alloc_coherent() isn't allocating a DMA buffer, then the
function name is misleading/confusing.
>
> The device driver should have access to the buffer pointer with write given
> that with request_firmware_into_buf() the driver is giving full write access to
> the memory pointer so that the firmware API can stuff the firmware it finds
> there.
>
> Firmware signature verification would be up to the device hardware to do upon
> load *after* request_firmware_into_buf().
We're discussing the kernel's signature verification, not the device
hardware's signature verification. Â Can the device driver access the
buffer, before IMA-appraisal has verified the firmware's signature?
Mimi
>
> Luis
>
> > will determine how
> > IMA-appraisal addresses READING_FIRMWARE_PREALLOC_BUFFER.
> >
> > Mimi
> >
> > > here
> > > are bits of code from: security/integrity/ima/ima_main.c:
> > >
> > > static int read_idmap[READING_MAX_ID] = {
> > > [READING_FIRMWARE] = FIRMWARE_CHECK,
> > > [READING_MODULE] = MODULE_CHECK,
> > > [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
> > > [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
> > > [READING_POLICY] = POLICY_CHECK
> > > };
> > >
> > > int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > ...
> > > if (!file && read_id == READING_FIRMWARE) {
> > > if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> > > (ima_appraise & IMA_APPRAISE_ENFORCE))
> > > return -EACCES; /* INTEGRITY_UNKNOWN */
> > > return 0;
> > > }
> > >
> > > Which show that the IMA code is not handling
> > > READING_FIRMWARE_PREALLOC_BUFFER as it should (I believe it
> > > should handle it the same as READING_FIRMWARE).
> > >
> > > Now we could fix that, but the only user of
> > > READING_FIRMWARE_PREALLOC_BUFFER is the code which originally
> > > introduced it:
> > >
> > > https://patchwork.kernel.org/patch/9162011/
> > >
> > > So I believe it might be better to instead replace it
> > > with just READING_FIRMWARE and find another way to tell
> > > kernel_read_file() that there is a pre-allocated buffer,
> > > perhaps the easiest way there is that *buf must be
> > > NULL when the caller wants kernel_read_file() to
> > > vmalloc the mem. This would of course require auditing
> > > all callers that the buf which the pass in is initialized
> > > to NULL.
> > >
> > > Either way adding a third READING_FIRMWARE_FOO to the
> > > kernel_read_file_id enum seems like a bad idea, from
> > > the IMA pov firmware is firmware.
> > >
> > > What this whole exercise has shown me though is that
> > > I need to call security_kernel_post_read_file() when
> > > loading EFI embedded firmware. I will add a call to
> > > security_kernel_post_read_file() for v4 of the patch-set.
> > >
> > > > Please Cc Kees in future patches.
> > >
> > > Will do.
> > >
> > > Regards,
> > >
> > > Hans
> > >
> >
> >
>
On Wed, Apr 25, 2018 at 01:00:09AM -0400, Mimi Zohar wrote:
> On Tue, 2018-04-24 at 23:42 +0000, Luis R. Rodriguez wrote:
> > On Tue, Apr 24, 2018 at 12:07:01PM -0400, Mimi Zohar wrote:
> > > On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
> > > > On 23-04-18 23:11, Luis R. Rodriguez wrote:
> > > > > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new ID
> > > > > and security for this type of request so IMA can reject it if the policy is
> > > > > configured for it.
> > > >
> > > > Hmm, interesting, actually it seems like the whole existence
> > > > of READING_FIRMWARE_PREALLOC_BUFFER is a mistake,
> >
> > request_firmware_into_buf() was merged without my own review, however,
> > the ID thing did get review from Mimi:
> >
> > https://patchwork.kernel.org/patch/9074611/
> >
> > The ID is not for IMA alone, its for any LSM to decide what to do.
> > Note Mimi asked for READING_FIRMWARE_DMA if such buffer was in DMA,
> > otherise READING_FIRMWARE_PREALLOC_BUFFER was suggested.
> >
> > Mimi why did you want a separate ID for it back before?
>
> The point of commit a098ecd2fa7d ("firmware: support loading into a
> pre-allocated buffer") is to avoid reading the firmware into kernel
> memory and then copying it "to it's final resting place". ?My concern
> is that if the device driver has access to the buffer, it could access
> the buffer prior to the firmware's signature having been verified by
> the kernel.
If request_firmware_into_buf() is used and the firmware was found in
/lib/firmware/ paths then the driver will *not* use the firmware prior
to any LSM doing any firmware signature verification because
kernel_read_file_from_path() and in turn security_kernel_read_file().
The firmware API has a fallback mechanism [0] though, and if that is used then
security_kernel_post_read_file() is used once the firmware is loaded through
the sysfs interface *prior* to handing the firmware data to the driver. As
Hans noted though security_kernel_post_read_file() currently *only* uses
READING_FIRMWARE, so this needs to be fixed. Also note though that LSMs
get a hint of what is going to happen *soon* prior to the fallback
mechanism kicking on as we travere the /lib/firmware/ paths for direct
filesystem loading.
If this is not sufficient to cover LSM appraisals *one* option could be to
have security_kernel_read_file() return a special error of some sort
for READING_FIRMWARE_PREALLOC_BUFFER so that kernel_read_file_from_path()
users could *know* to fatally give up.
Currently the device drivers using request_firmware_into_buf() can end up
getting the buffer with firmware stashed in it without having the kernel do any
firmware signature verification at all through its LSMs. The LSM hooks added to
the firmware loader long ago by Kees via commit 6593d9245bc66 ("firmware_class:
perform new LSM checks") on v3.17 added an LSM for direct filesystem lookups,
but on the fallback mechanism seems to have only added a post LSM hook
security_kernel_fw_from_file().
There is also a custom fallback mechanism [1] which can be used if the path to
the firmware may be out of the /lib/firmware/ paths or perhaps the firmware
requires some very custom fetching of some sort. The only thing this does
though is just *not* issue a uevent when we don't find the firmware and also
sets the timeout to a practically never-ending value. The custom fallback
mechanism is only usable for request_firmware_nowait() though. In retrospect
the custom fallback mechanism is pure crap and these days we've acknowledged
that even in crazy custom firmware fetching cases folks should be able to
accomplish this by relying on uevents and using the firmwared [2] or forking
it, or a different similar proprietary similar solution, which would just
monitor for uevents for firmware and just Do The Right Thing (TM).
Consider some mobile devices which may want to fetch it from some custom
partition which only it can know how to get.
There is a kernel config option which enables the fallback mechanism always,
This is now easily readable as follows:
drivers/base/firmware_loader/fallback_table.c
struct firmware_fallback_config fw_fallback_config = {
.force_sysfs_fallback = IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
.loading_timeout = 60,
.old_timeout = 60,
};
Even if this is used we always do direct fs lookups first.
Android became the primary user of CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
It would be good for us to hear from Android folks if their current use of
request_firmware_into_buf() is designed in practice to *never* use the direct
filesystem firmware loading interface, and always rely instead on the
fallback mechanism.
That would answer help your appraisal question in practice today.
[0] https://www.kernel.org/doc/html/latest/driver-api/firmware/fallback-mechanisms.html
[1] https://www.kernel.org/doc/html/latest/driver-api/firmware/fallback-mechanisms.html#firmware-custom-fallback-mechanism
[2] https://github.com/teg/firmwared
> In tightly controlled environments interested in limiting which signed
> firmware version is loaded, require's the device driver not having
> access to the buffer until after the signature has been verified by
> the kernel (eg. IMA-appraisal).
We may need more work for this for request_firmware_into_buf().
> > I should note now that request_firmware_into_buf() and its
> > READING_FIRMWARE_PREALLOC_BUFFER was to enable a driver on memory constrained
> > devices. The files are large (commit says 16 MiB).
> >
> > I've heard of larger possible files with remoteproc and with Android using
> > the custom fallback mechanism -- which could mean a proprietary tool
> > fetching firmware from a random special place on a device.
> >
> > I could perhaps imagine an LSM which may be aware of such type of
> > arrangement may want to do its own vetting of some sort, but this
> > would not be specific to READING_FIRMWARE_PREALLOC_BUFFER, but rather
> > the custom fallback mechaism.
> >
> > Whether or not the buffer was preallocated by the driver seems a little
> > odd for security folks to do something different with it. Security LSM
> > folks please chime in.
> >
> > I could see a bit more of a use case for an ID for firmware scraped
> > from EFI, which Hans' patch will provide. But that *also* should get
> > good review from other LSM folks.
> >
> > One of the issues with accepting more IDs loosely is where do we
> > stop though? If no one really is using READING_FIRMWARE_PREALLOC_BUFFER
> > I'd say lets remove it. Likewise, for this EFI thing I'd like an idea
> > if we really are going to have users for it.
> >
> > If its of any help --
> >
> > drivers/soc/qcom/mdt_loader.c is the only driver currently using
> > request_firmware_into_buf() however I'll note qcom_mdt_load() is used in many
> > other drivers so they are wrappers around request_firmware_into_buf():
> >
> > drivers/gpu/drm/msm/adreno/a5xx_gpu.c: * adreno_request_fw() handles this, but qcom_mdt_load() does
> > drivers/gpu/drm/msm/adreno/a5xx_gpu.c: ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID,
> > drivers/gpu/drm/msm/adreno/a5xx_gpu.c: ret = qcom_mdt_load(dev, fw, newname, GPU_PAS_ID,
> > drivers/media/platform/qcom/venus/firmware.c: ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
> > drivers/remoteproc/qcom_adsp_pil.c: return qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id,
> > drivers/remoteproc/qcom_wcnss.c: return qcom_mdt_load(wcnss->dev, fw, rproc->firmware, WCNSS_PAS_ID,
> >
> > Are we going to add more IDs for more types of firmware?
> > What type of *different* decisions could LSMs take if the firmware
> > was being written to a buffer? Or in this new case that is coming
> > up, if the file came scraping EFI, would having that information
> > be useful?
> >
> > > > As such the current IMA code (from v4.17-rc2) actually does
> > > > not handle READING_FIRMWARE_PREALLOC_BUFFER at all,
> > >
> > > Right, it doesn't yet address READING_FIRMWARE_PREALLOC_BUFFER, but
> > > should.
> > >
> > > Depending on whether the device requesting the firmware has access to
> > > the DMA memory, before the signature verification,
> >
> > It would seem from the original patch review about READING_FIRMWARE_PREALLOC_BUFFER
> > that this is not a DMA buffer.
>
> The call sequence:
> qcom_mdt_load() ->?qcom_scm_pas_init_image() ->?dma_alloc_coherent()
>
> If dma_alloc_coherent() isn't allocating a DMA buffer, then the
> function name is misleading/confusing.
Hah, by *definition* the device *and* processor has immediate access
to data written *immediately* when dma_alloc_coherent() is used. From
Documentation/DMA-API.txt:
-----------------------------------------------------------------------
Part Ia - Using large DMA-coherent buffers
------------------------------------------
::
void *
dma_alloc_coherent(struct device *dev, size_t size,
dma_addr_t *dma_handle, gfp_t flag)
Consistent memory is memory for which a write by either the device or
the processor can immediately be read by the processor or device
without having to worry about caching effects. (You may however need
to make sure to flush the processor's write buffers before telling
devices to read that memory.)
------------------------------------------------------------------------
Is ptr below
ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
ptr, phdr->p_filesz);
Also part of the DMA buffer allocated earlier via:
ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size);
Android folks?
> > The device driver should have access to the buffer pointer with write given
> > that with request_firmware_into_buf() the driver is giving full write access to
> > the memory pointer so that the firmware API can stuff the firmware it finds
> > there.
> >
> > Firmware signature verification would be up to the device hardware to do upon
> > load *after* request_firmware_into_buf().
>
> We're discussing the kernel's signature verification, not the device
> hardware's signature verification. ?Can the device driver access the
> buffer, before IMA-appraisal has verified the firmware's signature?
It will depend on the above question.
Luis
>
> Mimi
>
> >
> > Luis
> >
> > > will determine how
> > > IMA-appraisal addresses READING_FIRMWARE_PREALLOC_BUFFER.
> > >
> > > Mimi
> > >
> > > > here
> > > > are bits of code from: security/integrity/ima/ima_main.c:
> > > >
> > > > static int read_idmap[READING_MAX_ID] = {
> > > > [READING_FIRMWARE] = FIRMWARE_CHECK,
> > > > [READING_MODULE] = MODULE_CHECK,
> > > > [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
> > > > [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
> > > > [READING_POLICY] = POLICY_CHECK
> > > > };
> > > >
> > > > int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > > > ...
> > > > if (!file && read_id == READING_FIRMWARE) {
> > > > if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> > > > (ima_appraise & IMA_APPRAISE_ENFORCE))
> > > > return -EACCES; /* INTEGRITY_UNKNOWN */
> > > > return 0;
> > > > }
> > > >
> > > > Which show that the IMA code is not handling
> > > > READING_FIRMWARE_PREALLOC_BUFFER as it should (I believe it
> > > > should handle it the same as READING_FIRMWARE).
> > > >
> > > > Now we could fix that, but the only user of
> > > > READING_FIRMWARE_PREALLOC_BUFFER is the code which originally
> > > > introduced it:
> > > >
> > > > https://patchwork.kernel.org/patch/9162011/
> > > >
> > > > So I believe it might be better to instead replace it
> > > > with just READING_FIRMWARE and find another way to tell
> > > > kernel_read_file() that there is a pre-allocated buffer,
> > > > perhaps the easiest way there is that *buf must be
> > > > NULL when the caller wants kernel_read_file() to
> > > > vmalloc the mem. This would of course require auditing
> > > > all callers that the buf which the pass in is initialized
> > > > to NULL.
> > > >
> > > > Either way adding a third READING_FIRMWARE_FOO to the
> > > > kernel_read_file_id enum seems like a bad idea, from
> > > > the IMA pov firmware is firmware.
> > > >
> > > > What this whole exercise has shown me though is that
> > > > I need to call security_kernel_post_read_file() when
> > > > loading EFI embedded firmware. I will add a call to
> > > > security_kernel_post_read_file() for v4 of the patch-set.
> > > >
> > > > > Please Cc Kees in future patches.
> > > >
> > > > Will do.
> > > >
> > > > Regards,
> > > >
> > > > Hans
> > > >
> > >
> > >
> >
>
>
--
Do not panic
Android folks, poke below. otherwise we'll have no option but to seriously
consider Mimi's patch to prevent these calls when IMA appraisal is enforced:
http://lkml.kernel.org/r/[email protected]
Please read below....
On Wed, Apr 25, 2018 at 05:55:57PM +0000, Luis R. Rodriguez wrote:
> On Wed, Apr 25, 2018 at 01:00:09AM -0400, Mimi Zohar wrote:
> > On Tue, 2018-04-24 at 23:42 +0000, Luis R. Rodriguez wrote:
> > > On Tue, Apr 24, 2018 at 12:07:01PM -0400, Mimi Zohar wrote:
> > > > On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
> > > If its of any help --
> > >
> > > drivers/soc/qcom/mdt_loader.c is the only driver currently using
> > > request_firmware_into_buf() however I'll note qcom_mdt_load() is used in many
> > > other drivers so they are wrappers around request_firmware_into_buf():
> > >
> > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c: * adreno_request_fw() handles this, but qcom_mdt_load() does
> > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c: ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID,
> > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c: ret = qcom_mdt_load(dev, fw, newname, GPU_PAS_ID,
> > > drivers/media/platform/qcom/venus/firmware.c: ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
> > > drivers/remoteproc/qcom_adsp_pil.c: return qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id,
> > > drivers/remoteproc/qcom_wcnss.c: return qcom_mdt_load(wcnss->dev, fw, rproc->firmware, WCNSS_PAS_ID,
> > >
> > > > > As such the current IMA code (from v4.17-rc2) actually does
> > > > > not handle READING_FIRMWARE_PREALLOC_BUFFER at all,
> > > >
> > > > Right, it doesn't yet address READING_FIRMWARE_PREALLOC_BUFFER, but
> > > > should.
> > > >
> > > > Depending on whether the device requesting the firmware has access to
> > > > the DMA memory, before the signature verification,
> > >
> > > It would seem from the original patch review about READING_FIRMWARE_PREALLOC_BUFFER
> > > that this is not a DMA buffer.
To be very clear I believe Stephen implied this was not DMA buffer. Mimi
asked for READING_FIRMWARE_DMA if it was:
https://patchwork.kernel.org/patch/9074611/
> > The call sequence:
> > qcom_mdt_load() ->?qcom_scm_pas_init_image() ->?dma_alloc_coherent()
> >
> > If dma_alloc_coherent() isn't allocating a DMA buffer, then the
> > function name is misleading/confusing.
>
> Hah, by *definition* the device *and* processor has immediate access
> to data written *immediately* when dma_alloc_coherent() is used. From
> Documentation/DMA-API.txt:
>
> -----------------------------------------------------------------------
> Part Ia - Using large DMA-coherent buffers
> ------------------------------------------
>
> ::
>
> void *
> dma_alloc_coherent(struct device *dev, size_t size,
> dma_addr_t *dma_handle, gfp_t flag)
>
> Consistent memory is memory for which a write by either the device or
> the processor can immediately be read by the processor or device
> without having to worry about caching effects. (You may however need
> to make sure to flush the processor's write buffers before telling
> devices to read that memory.)
> ------------------------------------------------------------------------
>
> Is ptr below
>
> ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
> ptr, phdr->p_filesz);
>
> Also part of the DMA buffer allocated earlier via:
>
> ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size);
>
> Android folks?
Android folks?
> > > The device driver should have access to the buffer pointer with write given
> > > that with request_firmware_into_buf() the driver is giving full write access to
> > > the memory pointer so that the firmware API can stuff the firmware it finds
> > > there.
> > >
> > > Firmware signature verification would be up to the device hardware to do upon
> > > load *after* request_firmware_into_buf().
> >
> > We're discussing the kernel's signature verification, not the device
> > hardware's signature verification. ?Can the device driver access the
> > buffer, before IMA-appraisal has verified the firmware's signature?
>
> It will depend on the above question.
Luis
On Thu, May 3, 2018 at 5:21 PM, Luis R. Rodriguez <[email protected]> wrote:
> Android folks, poke below. otherwise we'll have no option but to seriously
> consider Mimi's patch to prevent these calls when IMA appraisal is enforced:
Sorry, figuring out who's the right person to answer this, will get
back to you ASAP.
Martijn
>
> http://lkml.kernel.org/r/[email protected]
>
> Please read below....
>
> On Wed, Apr 25, 2018 at 05:55:57PM +0000, Luis R. Rodriguez wrote:
>> On Wed, Apr 25, 2018 at 01:00:09AM -0400, Mimi Zohar wrote:
>> > On Tue, 2018-04-24 at 23:42 +0000, Luis R. Rodriguez wrote:
>> > > On Tue, Apr 24, 2018 at 12:07:01PM -0400, Mimi Zohar wrote:
>> > > > On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
>> > > If its of any help --
>> > >
>> > > drivers/soc/qcom/mdt_loader.c is the only driver currently using
>> > > request_firmware_into_buf() however I'll note qcom_mdt_load() is used in many
>> > > other drivers so they are wrappers around request_firmware_into_buf():
>> > >
>> > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c: * adreno_request_fw() handles this, but qcom_mdt_load() does
>> > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c: ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID,
>> > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c: ret = qcom_mdt_load(dev, fw, newname, GPU_PAS_ID,
>> > > drivers/media/platform/qcom/venus/firmware.c: ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID, mem_va, mem_phys,
>> > > drivers/remoteproc/qcom_adsp_pil.c: return qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id,
>> > > drivers/remoteproc/qcom_wcnss.c: return qcom_mdt_load(wcnss->dev, fw, rproc->firmware, WCNSS_PAS_ID,
>> > >
>> > > > > As such the current IMA code (from v4.17-rc2) actually does
>> > > > > not handle READING_FIRMWARE_PREALLOC_BUFFER at all,
>> > > >
>> > > > Right, it doesn't yet address READING_FIRMWARE_PREALLOC_BUFFER, but
>> > > > should.
>> > > >
>> > > > Depending on whether the device requesting the firmware has access to
>> > > > the DMA memory, before the signature verification,
>> > >
>> > > It would seem from the original patch review about READING_FIRMWARE_PREALLOC_BUFFER
>> > > that this is not a DMA buffer.
>
> To be very clear I believe Stephen implied this was not DMA buffer. Mimi
> asked for READING_FIRMWARE_DMA if it was:
>
> https://patchwork.kernel.org/patch/9074611/
>
>> > The call sequence:
>> > qcom_mdt_load() -> qcom_scm_pas_init_image() -> dma_alloc_coherent()
>> >
>> > If dma_alloc_coherent() isn't allocating a DMA buffer, then the
>> > function name is misleading/confusing.
>>
>> Hah, by *definition* the device *and* processor has immediate access
>> to data written *immediately* when dma_alloc_coherent() is used. From
>> Documentation/DMA-API.txt:
>>
>> -----------------------------------------------------------------------
>> Part Ia - Using large DMA-coherent buffers
>> ------------------------------------------
>>
>> ::
>>
>> void *
>> dma_alloc_coherent(struct device *dev, size_t size,
>> dma_addr_t *dma_handle, gfp_t flag)
>>
>> Consistent memory is memory for which a write by either the device or
>> the processor can immediately be read by the processor or device
>> without having to worry about caching effects. (You may however need
>> to make sure to flush the processor's write buffers before telling
>> devices to read that memory.)
>> ------------------------------------------------------------------------
>>
>> Is ptr below
>>
>> ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
>> ptr, phdr->p_filesz);
>>
>> Also part of the DMA buffer allocated earlier via:
>>
>> ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size);
>>
>> Android folks?
>
> Android folks?
>
>> > > The device driver should have access to the buffer pointer with write given
>> > > that with request_firmware_into_buf() the driver is giving full write access to
>> > > the memory pointer so that the firmware API can stuff the firmware it finds
>> > > there.
>> > >
>> > > Firmware signature verification would be up to the device hardware to do upon
>> > > load *after* request_firmware_into_buf().
>> >
>> > We're discussing the kernel's signature verification, not the device
>> > hardware's signature verification. Can the device driver access the
>> > buffer, before IMA-appraisal has verified the firmware's signature?
>>
>> It will depend on the above question.
>
> Luis
On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez <[email protected]> wrote:
> Android became the primary user of CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
>
> It would be good for us to hear from Android folks if their current use of
> request_firmware_into_buf() is designed in practice to *never* use the direct
> filesystem firmware loading interface, and always rely instead on the
> fallback mechanism.
It's hard to answer this question for Android in general. As far as I
can tell the reasons we use CONFIG_FW_LOADER_USER_HELPER(_FALLBACK)
are:
1) We have multiple different paths on our devices where firmware can
be located, and the direct loader only supports one custom path
2) Most of those paths are not mounted by the time the corresponding
drivers are loaded, because pretty much all Android kernels today are
built without module support, and therefore drivers are loaded well
before the firmware partition is mounted
3) I think we use _FALLBACK because doing this with uevents is just
the easiest thing to do; our init code has a firmware helper that
deals with this and searches the paths that we care about
2) will change at some point, because Android is moving towards a
model where device-specific peripheral drivers will be loaded as
modules, and since those modules would likely come from the same
partition as the firmware, it's possible that the direct load would
succeed (depending on whether the custom path is configured there or
not). But I don't think we can rely on the direct loader even in those
cases, unless we could configure it with multiple custom paths.
I have no reason to believe request_firmware_into_buf() is special in
this regard; drivers that depend on it may have their corresponding
firmware in different locations, so just depending on the direct
loader would not be good enough.
>
> Is ptr below
>
> ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
> ptr, phdr->p_filesz);
>
> Also part of the DMA buffer allocated earlier via:
>
> ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size);
>
> Android folks?
I think the Qualcomm folks owning this (Andy, David, Bjorn, already
cc'd here) are better suited to answer that question.
Thanks,
Martijn
On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote:
> On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez <[email protected]> wrote:
> > Android became the primary user of CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
> >
> > It would be good for us to hear from Android folks if their current use of
> > request_firmware_into_buf() is designed in practice to *never* use the direct
> > filesystem firmware loading interface, and always rely instead on the
> > fallback mechanism.
>
> It's hard to answer this question for Android in general. As far as I
> can tell the reasons we use CONFIG_FW_LOADER_USER_HELPER(_FALLBACK)
> are:
> 1) We have multiple different paths on our devices where firmware can
> be located, and the direct loader only supports one custom path
> 2) Most of those paths are not mounted by the time the corresponding
> drivers are loaded, because pretty much all Android kernels today are
> built without module support, and therefore drivers are loaded well
> before the firmware partition is mounted
> 3) I think we use _FALLBACK because doing this with uevents is just
> the easiest thing to do; our init code has a firmware helper that
> deals with this and searches the paths that we care about
>
> 2) will change at some point, because Android is moving towards a
> model where device-specific peripheral drivers will be loaded as
> modules, and since those modules would likely come from the same
> partition as the firmware, it's possible that the direct load would
> succeed (depending on whether the custom path is configured there or
> not). But I don't think we can rely on the direct loader even in those
> cases, unless we could configure it with multiple custom paths.
>
> I have no reason to believe request_firmware_into_buf() is special in
> this regard; drivers that depend on it may have their corresponding
> firmware in different locations, so just depending on the direct
> loader would not be good enough.
Thanks! This is very useful! This provides yet-another justification and use
case to document for the fallback mechanism. I'll go and extend it.
> >
> > Is ptr below
> >
> > ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
> > ptr, phdr->p_filesz);
> >
> > Also part of the DMA buffer allocated earlier via:
> >
> > ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size);
> >
> > Android folks?
>
> I think the Qualcomm folks owning this (Andy, David, Bjorn, already
> cc'd here) are better suited to answer that question.
Andy, David, Bjorn?
Luis
On Tue, May 08, 2018 at 03:38:05PM +0000, Luis R. Rodriguez wrote:
> On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote:
> > On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez <[email protected]> wrote:
> > > Android became the primary user of CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
> > >
> > > It would be good for us to hear from Android folks if their current use of
> > > request_firmware_into_buf() is designed in practice to *never* use the direct
> > > filesystem firmware loading interface, and always rely instead on the
> > > fallback mechanism.
> >
> > It's hard to answer this question for Android in general. As far as I
> > can tell the reasons we use CONFIG_FW_LOADER_USER_HELPER(_FALLBACK)
> > are:
> > 1) We have multiple different paths on our devices where firmware can
> > be located, and the direct loader only supports one custom path
FWIW I'd love to consider patches to address this, if this is something
you may find a need for in the future to *avoid* the fallback, however
would like a clean solution.
> > 2) Most of those paths are not mounted by the time the corresponding
> > drivers are loaded, because pretty much all Android kernels today are
> > built without module support, and therefore drivers are loaded well
> > before the firmware partition is mounted
I've given this some more thought and you can address this with initramfs,
this is how other Linux distributions are addressing this. One way to
address this automatically is to scrape the drivers built-in or needed early on
boot in initamfs and if the driver has a MODULE_FIRMWARE() its respective
firmware is added to initramfs as well.
If you *don't* use initramfs, then yes you can obviously run into issues
where your firmware may not be accessible if the driver is somehow loaded
early.
> > 3) I think we use _FALLBACK because doing this with uevents is just
> > the easiest thing to do; our init code has a firmware helper that
> > deals with this and searches the paths that we care about
> >
> > 2) will change at some point, because Android is moving towards a
> > model where device-specific peripheral drivers will be loaded as
> > modules, and since those modules would likely come from the same
> > partition as the firmware, it's possible that the direct load would
> > succeed (depending on whether the custom path is configured there or
> > not). But I don't think we can rely on the direct loader even in those
> > cases, unless we could configure it with multiple custom paths.
Using initramfs will help, but because of the custom path needs -- you're
right, we don't have anything for that yet, its also a bit unclear if
something nice and clean can be drawn up for it. So perhaps dealing with
the fallback mechanism is the way to go for this for sure, since we already
have support for it.
Just keep in mind that the fallback mechanism costs you about ~13436 bytes.
So, if someone comes up with a clean interface for custom paths I'd love
to consider it to avoid those 13436 bytes.
Luis
On Tue, May 08, 2018 at 03:38:05PM +0000, Luis R. Rodriguez wrote:
> On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote:
> > On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez <[email protected]> wrote:
> > > Is ptr below
> > >
> > > ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
> > > ptr, phdr->p_filesz);
> > >
> > > Also part of the DMA buffer allocated earlier via:
> > >
> > > ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size);
> > >
> > > Android folks?
> >
> > I think the Qualcomm folks owning this (Andy, David, Bjorn, already
> > cc'd here) are better suited to answer that question.
>
> Andy, David, Bjorn?
Andy, David, Bjorn?
Note: as-is we have no option but to assume this is DMA memory for now.
We cannot keep IMA's guarantees with the current prealloc firmware API
buffer, so I've suggested:
a) The prealloc buffer API be expanded to enable the caller to descrbe it
b) Have the qcom driver say this is DMA
c) IMA would reject it to ensure it stays true to what it needs to gaurantee
d) Future platforms which want to use IMA but want to trust DMA buffers
would need to devise a way to describe IMA can trust some of these
calls.
I'll leave it up to you guys (Andy, David, Bjorn) to come up with the code for
d) once and if you guys want to use IMA later. But since what is pressing here
is to stay to true to IMA, with a-c IMA would reject such calls for now.
Luis
On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote:
> On Tue, May 08, 2018 at 03:38:05PM +0000, Luis R. Rodriguez wrote:
> > On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote:
> > >
> > > I think the Qualcomm folks owning this (Andy, David, Bjorn, already
> > > cc'd here) are better suited to answer that question.
> >
> > Andy, David, Bjorn?
>
> Andy, David, Bjorn?
A month now with no answer...
Perhaps someone who has this hardware can find out empirically for us, as
follows (mm folks is this right?):
page = virt_to_page(address);
if (!page)
fail closed...
if (page_zone(page) == ZONE_DMA || page_zone(page) == ZONE_DMA32)
this is a DMA buffer
else
not DMA!
Note that when request_firmware_into_buf() was being reviewed Mimi had asked back
in 2016 [0] that if a DMA buffer was going to be used READING_FIRMWARE_DMA should be
used otherwise READING_FIRMWARE_PREALLOC_BUFFER was fine.
If it is a DMA buffer *now*, why / how did this change?
[0] https://patchwork.kernel.org/patch/9074611/
Luis
On Wed 06 Jun 13:32 PDT 2018, Luis R. Rodriguez wrote:
> On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote:
> > On Tue, May 08, 2018 at 03:38:05PM +0000, Luis R. Rodriguez wrote:
> > > On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote:
> > > >
> > > > I think the Qualcomm folks owning this (Andy, David, Bjorn, already
> > > > cc'd here) are better suited to answer that question.
> > >
> > > Andy, David, Bjorn?
> >
> > Andy, David, Bjorn?
>
> A month now with no answer...
>
The patch at the top of this thread doesn't interest me and you didn't
bother sending your question To me.
As a matter of fact I'm confused to what the actual question is.
> Perhaps someone who has this hardware can find out empirically for us, as
> follows (mm folks is this right?):
>
> page = virt_to_page(address);
> if (!page)
> fail closed...
> if (page_zone(page) == ZONE_DMA || page_zone(page) == ZONE_DMA32)
> this is a DMA buffer
> else
> not DMA!
>
Where do you want to put this?
> Note that when request_firmware_into_buf() was being reviewed Mimi had asked back
> in 2016 [0] that if a DMA buffer was going to be used READING_FIRMWARE_DMA should be
> used otherwise READING_FIRMWARE_PREALLOC_BUFFER was fine.
>
> If it is a DMA buffer *now*, why / how did this change?
>
From what I can see [0] says is to use READING_FIRMWARE_PREALLOC_BUFFER
regardless of where the memory comes from.
Regards,
Bjorn
> [0] https://patchwork.kernel.org/patch/9074611/
>
> Luis
On Tue 24 Apr 22:00 PDT 2018, Mimi Zohar wrote:
> On Tue, 2018-04-24 at 23:42 +0000, Luis R. Rodriguez wrote:
> > On Tue, Apr 24, 2018 at 12:07:01PM -0400, Mimi Zohar wrote:
> > > On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
[..]
> > > > As such the current IMA code (from v4.17-rc2) actually does
> > > > not handle READING_FIRMWARE_PREALLOC_BUFFER at all,
> > >
> > > Right, it doesn't yet address READING_FIRMWARE_PREALLOC_BUFFER, but
> > > should.
> > >
> > > Depending on whether the device requesting the firmware has access to
> > > the DMA memory, before the signature verification,
> >
> > It would seem from the original patch review about READING_FIRMWARE_PREALLOC_BUFFER
> > that this is not a DMA buffer.
>
> The call sequence:
> qcom_mdt_load() ->?qcom_scm_pas_init_image() ->?dma_alloc_coherent()
>
qcom_mdt_load() is passed a struct firmware object, which "data" is
passed into qcom_scm_pas_init_image(), which uses dma_alloc_coherent()
to allocate scratch memory to perform a call into secure world. So the
dma_alloc_coherent() here has nothing to do with firmware loading.
qcom_mdt_load() will then use request_firmware_into_buf() to load other
files into the passed void *mem_region, which either comes from a
memremap() or dma_alloc_coherent() call.
> If dma_alloc_coherent() isn't allocating a DMA buffer, then the
> function name is misleading/confusing.
>
It does allocate memory.
> >
> > The device driver should have access to the buffer pointer with write given
> > that with request_firmware_into_buf() the driver is giving full write access to
> > the memory pointer so that the firmware API can stuff the firmware it finds
> > there.
> >
> > Firmware signature verification would be up to the device hardware to do upon
> > load *after* request_firmware_into_buf().
>
> We're discussing the kernel's signature verification, not the device
> hardware's signature verification. ?Can the device driver access the
> buffer, before IMA-appraisal has verified the firmware's signature?
>
I would expect that it's possible to read the content of the buffer from
a secondary execution context before the request_firmware_into_buf() has
verified the content of the buffer, but I would be considered a
seriously broken driver.
Regards,
Bjorn
On Tue 08 May 09:10 PDT 2018, Luis R. Rodriguez wrote:
> On Tue, May 08, 2018 at 03:38:05PM +0000, Luis R. Rodriguez wrote:
> > On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote:
> > > On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez <[email protected]> wrote:
[..]
> > > 2) Most of those paths are not mounted by the time the corresponding
> > > drivers are loaded, because pretty much all Android kernels today are
> > > built without module support, and therefore drivers are loaded well
> > > before the firmware partition is mounted
>
> I've given this some more thought and you can address this with initramfs,
> this is how other Linux distributions are addressing this. One way to
> address this automatically is to scrape the drivers built-in or needed early on
> boot in initamfs and if the driver has a MODULE_FIRMWARE() its respective
> firmware is added to initramfs as well.
>
That could be done, but it would not change the fact that the
/sys/class/firmware is ABI and you may not break it.
And it doesn't change the fact that the ramdisk would have to be over
100mb to facilitate this.
> If you *don't* use initramfs, then yes you can obviously run into issues
> where your firmware may not be accessible if the driver is somehow loaded
> early.
>
This is still a problem that lacks a solution.
> > > 3) I think we use _FALLBACK because doing this with uevents is just
> > > the easiest thing to do; our init code has a firmware helper that
> > > deals with this and searches the paths that we care about
> > >
> > > 2) will change at some point, because Android is moving towards a
> > > model where device-specific peripheral drivers will be loaded as
> > > modules, and since those modules would likely come from the same
> > > partition as the firmware, it's possible that the direct load would
> > > succeed (depending on whether the custom path is configured there or
> > > not). But I don't think we can rely on the direct loader even in those
> > > cases, unless we could configure it with multiple custom paths.
>
> Using initramfs will help, but because of the custom path needs -- you're
> right, we don't have anything for that yet, its also a bit unclear if
> something nice and clean can be drawn up for it. So perhaps dealing with
> the fallback mechanism is the way to go for this for sure, since we already
> have support for it.
>
> Just keep in mind that the fallback mechanism costs you about ~13436 bytes.
>
Remember that putting the firmware in the ramdisk would cost about
10000x (yes, ten thousand times) more ram.
> So, if someone comes up with a clean interface for custom paths I'd love
> to consider it to avoid those 13436 bytes.
>
Combined with a way of synchronizing this with the availability of the
firmware, this would be a nice thing!
Regards,
Bjorn
On Thu, Jun 07, 2018 at 09:49:50AM -0700, Bjorn Andersson wrote:
> On Tue 08 May 09:10 PDT 2018, Luis R. Rodriguez wrote:
>
> > On Tue, May 08, 2018 at 03:38:05PM +0000, Luis R. Rodriguez wrote:
> > > On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote:
> > > > On Wed, Apr 25, 2018 at 10:55 AM, Luis R. Rodriguez <[email protected]> wrote:
> [..]
> > > > 2) Most of those paths are not mounted by the time the corresponding
> > > > drivers are loaded, because pretty much all Android kernels today are
> > > > built without module support, and therefore drivers are loaded well
> > > > before the firmware partition is mounted
> >
> > I've given this some more thought and you can address this with initramfs,
> > this is how other Linux distributions are addressing this. One way to
> > address this automatically is to scrape the drivers built-in or needed early on
> > boot in initamfs and if the driver has a MODULE_FIRMWARE() its respective
> > firmware is added to initramfs as well.
> >
>
> That could be done, but it would not change the fact that the
> /sys/class/firmware is ABI and you may not break it.
Right, this is now well documented and also the latest changes to the firmware
API have made the sysfs fallback loader an option through a sysctl knob. The
code should be much easier to follow and test now.
> And it doesn't change the fact that the ramdisk would have to be over
> 100mb to facilitate this.
Indeed, this is now acknowledged in the latest Kconfig for the firmware loader.
> > If you *don't* use initramfs, then yes you can obviously run into issues
> > where your firmware may not be accessible if the driver is somehow loaded
> > early.
> >
>
> This is still a problem that lacks a solution.
The firmwared solution capturing uevents and using the sysfs fallback
mechanism should resolve this. Its also now properly documented on the
firmware loader Kconfig.
> > > > 3) I think we use _FALLBACK because doing this with uevents is just
> > > > the easiest thing to do; our init code has a firmware helper that
> > > > deals with this and searches the paths that we care about
> > > >
> > > > 2) will change at some point, because Android is moving towards a
> > > > model where device-specific peripheral drivers will be loaded as
> > > > modules, and since those modules would likely come from the same
> > > > partition as the firmware, it's possible that the direct load would
> > > > succeed (depending on whether the custom path is configured there or
> > > > not). But I don't think we can rely on the direct loader even in those
> > > > cases, unless we could configure it with multiple custom paths.
> >
> > Using initramfs will help, but because of the custom path needs -- you're
> > right, we don't have anything for that yet, its also a bit unclear if
> > something nice and clean can be drawn up for it. So perhaps dealing with
> > the fallback mechanism is the way to go for this for sure, since we already
> > have support for it.
> >
> > Just keep in mind that the fallback mechanism costs you about ~13436 bytes.
> >
>
> Remember that putting the firmware in the ramdisk would cost about
> 10000x (yes, ten thousand times) more ram.
Indeed, this is now documented on the Kconfig too.
> > So, if someone comes up with a clean interface for custom paths I'd love
> > to consider it to avoid those 13436 bytes.
> >
>
> Combined with a way of synchronizing this with the availability of the
> firmware, this would be a nice thing!
The firmwared solution seems to be the way to go for now.
Luis
On 06/07/2018 06:33 PM, Greg Kroah-Hartman wrote:
> On Thu, Jun 07, 2018 at 06:23:01PM +0200, Ard Biesheuvel wrote:
>> On 7 June 2018 at 18:18, Bjorn Andersson <[email protected]> wrote:
>>> On Wed 06 Jun 13:32 PDT 2018, Luis R. Rodriguez wrote:
>>>
>>>> On Fri, Jun 01, 2018 at 09:23:46PM +0200, Luis R. Rodriguez wrote:
>>>>> On Tue, May 08, 2018 at 03:38:05PM +0000, Luis R. Rodriguez wrote:
>>>>>> On Fri, May 04, 2018 at 12:44:37PM -0700, Martijn Coenen wrote:
>>>>>>>
>>>>>>> I think the Qualcomm folks owning this (Andy, David, Bjorn, already
>>>>>>> cc'd here) are better suited to answer that question.
>>>>>>
>>>>>> Andy, David, Bjorn?
>>>>>
>>>>> Andy, David, Bjorn?
>>>>
>>>> A month now with no answer...
>>>>
>>>
>>> The patch at the top of this thread doesn't interest me and you didn't
>>> bother sending your question To me.
>>>
>>> As a matter of fact I'm confused to what the actual question is.
>>>
>>
>> The actual question is whether it is really required that the firmware
>> is loaded by the kernel into a buffer that is already mapped for DMA
>> at that point, and thus accessible by the device.
>>
>> To me, it is not entirely clear what the nature is of the firmware
>> that we are talking about, since it seems to be getting passed to the
>> secure world as well?
>>
>> In any case, the preferred model in terms of validation/sig checking is
>>
>> 1) allocate a CPU accessible buffer
>>
>> 2) request the firmware into it (which may include a sig check under the hood)
>>
>> 3) map the buffer for DMA to the device so it can load the firmware.
>>
>> 4) kick off the DMA transfer.
>>
>> The use of dma_alloc_coherent() for this purpose seems unnecessary,
>> given that the DMA transfer is not bidirectional. Would it be possible
>> to replace it with something like the above sequence?
>
> Why not just use kmalloc, it will always return a DMAable buffer.
DMAble in what sense? For devices that can't handle physical addresses
above 16M you need to pass __GFP_DMA to get those, from ZONE_DMA.
Otherwise it can return anything from lowmem. That's for x86_64, some
other arches have different DMA zone.
> Is the problem that vmalloc() might not?
vmalloc() could only be used as an alternative if you used kvmalloc(),
otherwise kmalloc() won't give you anything from vmalloc
> We need to drop the whole DMA zone crud, it confuses everyone who sees
> it and was primarily for really really old systems.
Yeah that would be nice.
> greg k-h
>