* [PATCH v2 0/3] arch-agnostic initrd loading method for EFI systems
@ 2020-02-16 14:11 Ard Biesheuvel
2020-02-16 14:11 ` [PATCH v2 1/3] efi/dev-path-parser: Add struct definition for vendor type device path nodes Ard Biesheuvel
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2020-02-16 14:11 UTC (permalink / raw)
To: linux-efi
Cc: linux-kernel, Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf,
ilias.apalodimas, xypron.glpk, daniel.kiper, nivedita,
James.Bottomley, lukas
This series introduces an arch agnostic way of loading the initrd into
memory from the EFI stub. This addresses a number of shortcomings that
affect the current implementations that exist across architectures:
- The initrd=<file> command line option can only load files that reside
on the same file system that the kernel itself was loaded from, which
requires the bootloader or firmware to expose that file system via the
appropriate EFI protocol, which is not always feasible. From the kernel
side, this protocol is problematic since it is incompatible with mixed
mode on x86 (this is due to the fact that some of its methods have
prototypes that are difficult to marshall)
- The approach that is ordinarily taken by GRUB is to load the initrd into
memory, and pass it to the kernel proper via the bootparams structure or
via the device tree. This requires the boot loader to have an understanding
of those structures, which are not always set in stone, and of the policies
around where the initrd may be loaded into memory. In the ARM case, it
requires GRUB to modify the hardware description provided by the firmware,
given that the initrd base and offset in memory are passed via the same
data structure. It also creates a time window where the initrd data sits
in memory, and can potentially be corrupted before the kernel is booted.
Considering that we will soon have new users of these interfaces (EFI for
kvmtool on ARM, RISC-V in u-boot, etc), it makes sense to add a generic
interface now, before having another wave of bespoke arch specific code
coming in.
Another aspect to take into account is that support for UEFI secure boot
and measured boot is being taken into the upstream, and being able to
rely on the PE entry point for booting any architecture makes the GRUB
vs shim story much cleaner, as we should be able to rely on LoadImage
and StartImage on all architectures, while retaining the ability to
load initrds from anywhere.
Note that these patches depend on a fair amount of cleanup work that I
am targetting for v5.7. Branch can be found at:
https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/log/?h=next
A PoC implementation for OVMF and ArmVirtQemu (OVMF for ARM aka AAVMF) can
be found at https://github.com/ardbiesheuvel/edk2/commits/linux-efi-generic.
A U-Boot implementation is under development as well, and can be found at
https://github.com/apalos/u-boot/commits/efi_load_file_8
Changes since v1:
- merge vendor media device path type definition with the existing device path
definitions we already have, and rework the latter slightly to be more easily
reusable
- use 'dev_path' not 'devpath' consistently
- pass correct FilePath value to LoadFile2 (i.e., the device path pointer that
was advanced to point to the 'end' node by locate_device_path())
- add kerneldoc comment to efi_load_initrd_dev_path()
- take care to only return EFI_NOT_FOUND from efi_load_initrd_dev_path() if the
LoadFile2 protocol does not exist on the LINUX_EFI_INITRD_MEDIA_GUID device
path - this makes the logic whether to fallback to the command line method
more robust
Cc: lersek@redhat.com
Cc: leif@nuviainc.com
Cc: pjones@redhat.com
Cc: mjg59@google.com
Cc: agraf@csgraf.de
Cc: ilias.apalodimas@linaro.org
Cc: xypron.glpk@gmx.de
Cc: daniel.kiper@oracle.com
Cc: nivedita@alum.mit.edu
Cc: James.Bottomley@hansenpartnership.com
Cc: lukas@wunner.de
Ard Biesheuvel (3):
efi/dev-path-parser: Add struct definition for vendor type device path
nodes
efi/libstub: Add support for loading the initrd from a device path
efi/libstub: Take noinitrd cmdline argument into account for devpath
initrd
drivers/firmware/efi/apple-properties.c | 8 +-
drivers/firmware/efi/dev-path-parser.c | 38 ++++----
drivers/firmware/efi/libstub/arm-stub.c | 20 ++++-
.../firmware/efi/libstub/efi-stub-helper.c | 89 +++++++++++++++++++
drivers/firmware/efi/libstub/efistub.h | 5 ++
drivers/firmware/efi/libstub/x86-stub.c | 47 ++++++++--
include/linux/efi.h | 49 ++++++----
7 files changed, 201 insertions(+), 55 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/3] efi/dev-path-parser: Add struct definition for vendor type device path nodes
2020-02-16 14:11 [PATCH v2 0/3] arch-agnostic initrd loading method for EFI systems Ard Biesheuvel
@ 2020-02-16 14:11 ` Ard Biesheuvel
2020-02-16 14:11 ` [PATCH v2 2/3] efi/libstub: Add support for loading the initrd from a device path Ard Biesheuvel
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2020-02-16 14:11 UTC (permalink / raw)
To: linux-efi
Cc: linux-kernel, Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf,
ilias.apalodimas, xypron.glpk, daniel.kiper, nivedita,
James.Bottomley, lukas
In preparation of adding support for loading the initrd via a special
device path, add the struct definition of a vendor GUIDed device path
node to efi.h.
Since we will be producing these data structures rather than just
consumsing the ones instantiated by the firmware, refactor the various
device path node definitions so we can take the size of each node using
sizeof() rather than having to resort to opaque arithmetic in the static
initializers.
While at it, drop the #if IS_ENABLED() check for the declaration of
efi_get_device_by_path(), which is unnecessary, and constify its first
argument as well.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
drivers/firmware/efi/apple-properties.c | 8 ++--
drivers/firmware/efi/dev-path-parser.c | 38 ++++++++--------
include/linux/efi.h | 48 ++++++++++++--------
3 files changed, 52 insertions(+), 42 deletions(-)
diff --git a/drivers/firmware/efi/apple-properties.c b/drivers/firmware/efi/apple-properties.c
index 084942846f4d..34f53d898acb 100644
--- a/drivers/firmware/efi/apple-properties.c
+++ b/drivers/firmware/efi/apple-properties.c
@@ -46,7 +46,7 @@ struct properties_header {
};
static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
- struct device *dev, void *ptr,
+ struct device *dev, const void *ptr,
struct property_entry entry[])
{
int i;
@@ -117,10 +117,10 @@ static int __init unmarshal_devices(struct properties_header *properties)
while (offset + sizeof(struct dev_header) < properties->len) {
struct dev_header *dev_header = (void *)properties + offset;
struct property_entry *entry = NULL;
+ const struct efi_dev_path *ptr;
struct device *dev;
size_t len;
int ret, i;
- void *ptr;
if (offset + dev_header->len > properties->len ||
dev_header->len <= sizeof(*dev_header)) {
@@ -131,10 +131,10 @@ static int __init unmarshal_devices(struct properties_header *properties)
ptr = dev_header->path;
len = dev_header->len - sizeof(*dev_header);
- dev = efi_get_device_by_path((struct efi_dev_path **)&ptr, &len);
+ dev = efi_get_device_by_path(&ptr, &len);
if (IS_ERR(dev)) {
pr_err("device path parse error %ld at %#zx:\n",
- PTR_ERR(dev), ptr - (void *)dev_header);
+ PTR_ERR(dev), (void *)ptr - (void *)dev_header);
print_hex_dump(KERN_ERR, pr_fmt(), DUMP_PREFIX_OFFSET,
16, 1, dev_header, dev_header->len, true);
dev = NULL;
diff --git a/drivers/firmware/efi/dev-path-parser.c b/drivers/firmware/efi/dev-path-parser.c
index 20123384271c..5c9625e552f4 100644
--- a/drivers/firmware/efi/dev-path-parser.c
+++ b/drivers/firmware/efi/dev-path-parser.c
@@ -31,13 +31,13 @@ static int __init match_acpi_dev(struct device *dev, const void *data)
return !strcmp("0", hid_uid.uid);
}
-static long __init parse_acpi_path(struct efi_dev_path *node,
+static long __init parse_acpi_path(const struct efi_dev_path *node,
struct device *parent, struct device **child)
{
struct acpi_hid_uid hid_uid = {};
struct device *phys_dev;
- if (node->length != 12)
+ if (node->header.length != 12)
return -EINVAL;
sprintf(hid_uid.hid[0].id, "%c%c%c%04X",
@@ -69,12 +69,12 @@ static int __init match_pci_dev(struct device *dev, void *data)
return dev_is_pci(dev) && to_pci_dev(dev)->devfn == devfn;
}
-static long __init parse_pci_path(struct efi_dev_path *node,
+static long __init parse_pci_path(const struct efi_dev_path *node,
struct device *parent, struct device **child)
{
unsigned int devfn;
- if (node->length != 6)
+ if (node->header.length != 6)
return -EINVAL;
if (!parent)
return -EINVAL;
@@ -105,19 +105,19 @@ static long __init parse_pci_path(struct efi_dev_path *node,
* search for a device.
*/
-static long __init parse_end_path(struct efi_dev_path *node,
+static long __init parse_end_path(const struct efi_dev_path *node,
struct device *parent, struct device **child)
{
- if (node->length != 4)
+ if (node->header.length != 4)
return -EINVAL;
- if (node->sub_type != EFI_DEV_END_INSTANCE &&
- node->sub_type != EFI_DEV_END_ENTIRE)
+ if (node->header.sub_type != EFI_DEV_END_INSTANCE &&
+ node->header.sub_type != EFI_DEV_END_ENTIRE)
return -EINVAL;
if (!parent)
return -ENODEV;
*child = get_device(parent);
- return node->sub_type;
+ return node->header.sub_type;
}
/**
@@ -156,7 +156,7 @@ static long __init parse_end_path(struct efi_dev_path *node,
* %ERR_PTR(-EINVAL) if a node is malformed or exceeds @len,
* %ERR_PTR(-ENOTSUPP) if support for a node type is not yet implemented.
*/
-struct device * __init efi_get_device_by_path(struct efi_dev_path **node,
+struct device * __init efi_get_device_by_path(const struct efi_dev_path **node,
size_t *len)
{
struct device *parent = NULL, *child;
@@ -166,16 +166,16 @@ struct device * __init efi_get_device_by_path(struct efi_dev_path **node,
return NULL;
while (!ret) {
- if (*len < 4 || *len < (*node)->length)
+ if (*len < 4 || *len < (*node)->header.length)
ret = -EINVAL;
- else if ((*node)->type == EFI_DEV_ACPI &&
- (*node)->sub_type == EFI_DEV_BASIC_ACPI)
+ else if ((*node)->header.type == EFI_DEV_ACPI &&
+ (*node)->header.sub_type == EFI_DEV_BASIC_ACPI)
ret = parse_acpi_path(*node, parent, &child);
- else if ((*node)->type == EFI_DEV_HW &&
- (*node)->sub_type == EFI_DEV_PCI)
+ else if ((*node)->header.type == EFI_DEV_HW &&
+ (*node)->header.sub_type == EFI_DEV_PCI)
ret = parse_pci_path(*node, parent, &child);
- else if (((*node)->type == EFI_DEV_END_PATH ||
- (*node)->type == EFI_DEV_END_PATH2))
+ else if (((*node)->header.type == EFI_DEV_END_PATH ||
+ (*node)->header.type == EFI_DEV_END_PATH2))
ret = parse_end_path(*node, parent, &child);
else
ret = -ENOTSUPP;
@@ -185,8 +185,8 @@ struct device * __init efi_get_device_by_path(struct efi_dev_path **node,
return ERR_PTR(ret);
parent = child;
- *node = (void *)*node + (*node)->length;
- *len -= (*node)->length;
+ *node = (void *)*node + (*node)->header.length;
+ *len -= (*node)->header.length;
}
if (ret == EFI_DEV_END_ENTIRE)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 9ccf313fe9de..0976e57b4caa 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -855,30 +855,40 @@ extern int efi_status_to_err(efi_status_t status);
#define EFI_DEV_END_ENTIRE 0xFF
struct efi_generic_dev_path {
- u8 type;
- u8 sub_type;
- u16 length;
-} __attribute ((packed));
+ u8 type;
+ u8 sub_type;
+ u16 length;
+} __packed;
+
+struct efi_acpi_dev_path {
+ struct efi_generic_dev_path header;
+ u32 hid;
+ u32 uid;
+} __packed;
+
+struct efi_pci_dev_path {
+ struct efi_generic_dev_path header;
+ u8 fn;
+ u8 dev;
+} __packed;
+
+struct efi_vendor_dev_path {
+ struct efi_generic_dev_path header;
+ efi_guid_t vendorguid;
+ u8 vendordata[];
+} __packed;
struct efi_dev_path {
- u8 type; /* can be replaced with unnamed */
- u8 sub_type; /* struct efi_generic_dev_path; */
- u16 length; /* once we've moved to -std=c11 */
union {
- struct {
- u32 hid;
- u32 uid;
- } acpi;
- struct {
- u8 fn;
- u8 dev;
- } pci;
+ struct efi_generic_dev_path header;
+ struct efi_acpi_dev_path acpi;
+ struct efi_pci_dev_path pci;
+ struct efi_vendor_dev_path vendor;
};
-} __attribute ((packed));
+} __packed;
-#if IS_ENABLED(CONFIG_EFI_DEV_PATH_PARSER)
-struct device *efi_get_device_by_path(struct efi_dev_path **node, size_t *len);
-#endif
+struct device *efi_get_device_by_path(const struct efi_dev_path **node,
+ size_t *len);
static inline void memrange_efi_to_native(u64 *addr, u64 *npages)
{
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/3] efi/libstub: Add support for loading the initrd from a device path
2020-02-16 14:11 [PATCH v2 0/3] arch-agnostic initrd loading method for EFI systems Ard Biesheuvel
2020-02-16 14:11 ` [PATCH v2 1/3] efi/dev-path-parser: Add struct definition for vendor type device path nodes Ard Biesheuvel
@ 2020-02-16 14:11 ` Ard Biesheuvel
2020-02-17 9:15 ` Laszlo Ersek
2020-02-16 14:11 ` [PATCH v2 3/3] efi/libstub: Take noinitrd cmdline argument into account for devpath initrd Ard Biesheuvel
2020-02-17 19:10 ` [PATCH v2 0/3] arch-agnostic initrd loading method for EFI systems Bhupesh Sharma
3 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2020-02-16 14:11 UTC (permalink / raw)
To: linux-efi
Cc: linux-kernel, Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf,
ilias.apalodimas, xypron.glpk, daniel.kiper, nivedita,
James.Bottomley, lukas
There are currently two ways to specify the initrd to be passed to the
Linux kernel when booting via the EFI stub:
- it can be passed as a initrd= command line option when doing a pure PE
boot (as opposed to the EFI handover protocol that exists for x86)
- otherwise, the bootloader or firmware can load the initrd into memory,
and pass the address and size via the bootparams struct (x86) or
device tree (ARM)
In the first case, we are limited to loading from the same file system
that the kernel was loaded from, and it is also problematic in a trusted
boot context, given that we cannot easily protect the command line from
tampering without either adding complicated white/blacklisting of boot
arguments or locking down the command line altogether.
In the second case, we force the bootloader to duplicate knowledge about
the boot protocol which is already encoded in the stub, and which may be
subject to change over time, e.g., bootparams struct definitions, memory
allocation/alignment requirements for the placement of the initrd etc etc.
In the ARM case, it also requires the bootloader to modify the hardware
description provided by the firmware, as it is passed in the same file.
On systems where the initrd is measured after loading, it creates a time
window where the initrd contents might be manipulated in memory before
handing over to the kernel.
Address these concerns by adding support for loading the initrd into
memory by invoking the EFI LoadFile2 protocol installed on a vendor
GUIDed device path that specifically designates a Linux initrd.
This addresses the above concerns, by putting the EFI stub in charge of
placement in memory and of passing the base and size to the kernel proper
(via whatever means it desires) while still leaving it up to the firmware
or bootloader to obtain the file contents, potentially from other file
systems than the one the kernel itself was loaded from. On platforms that
implement measured boot, it permits the firmware to take the measurement
right before the kernel actually consumes the contents.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
drivers/firmware/efi/libstub/arm-stub.c | 15 +++-
drivers/firmware/efi/libstub/efi-stub-helper.c | 82 ++++++++++++++++++++
drivers/firmware/efi/libstub/efistub.h | 4 +
drivers/firmware/efi/libstub/x86-stub.c | 23 ++++++
include/linux/efi.h | 1 +
5 files changed, 122 insertions(+), 3 deletions(-)
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 2edc673ea06c..4bae620b95b9 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -160,6 +160,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
enum efi_secureboot_mode secure_boot;
struct screen_info *si;
efi_properties_table_t *prop_tbl;
+ unsigned long max_addr;
sys_table = sys_table_arg;
@@ -258,10 +259,18 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
if (!fdt_addr)
pr_efi("Generating empty DTB\n");
- status = efi_load_initrd(image, &initrd_addr, &initrd_size, ULONG_MAX,
- efi_get_max_initrd_addr(dram_base, *image_addr));
+ max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
+ status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size, max_addr);
+ if (status == EFI_SUCCESS) {
+ pr_efi("Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path\n");
+ } else if (status == EFI_NOT_FOUND) {
+ status = efi_load_initrd(image, &initrd_addr, &initrd_size,
+ ULONG_MAX, max_addr);
+ if (status == EFI_SUCCESS)
+ pr_efi("Loaded initrd from command line option\n");
+ }
if (status != EFI_SUCCESS)
- pr_efi_err("Failed initrd from command line!\n");
+ pr_efi_err("Failed to load initrd!\n");
efi_random_get_seed();
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 49008ac88b63..e37afe2c752e 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -299,3 +299,85 @@ void efi_char16_printk(efi_char16_t *str)
efi_call_proto(efi_table_attr(efi_system_table(), con_out),
output_string, str);
}
+
+/*
+ * The LINUX_EFI_INITRD_MEDIA_GUID vendor media device path below provides a way
+ * for the firmware or bootloader to expose the initrd data directly to the stub
+ * via the trivial LoadFile2 protocol, which is defined in the UEFI spec, and is
+ * very easy to implement. It is a simple Linux initrd specific conduit between
+ * kernel and firmware, allowing us to put the EFI stub (being part of the
+ * kernel) in charge of where and when to load the initrd, while leaving it up
+ * to the firmware to decide whether it needs to expose its filesystem hierarchy
+ * via EFI protocols.
+ */
+static const struct {
+ struct efi_vendor_dev_path vendor;
+ struct efi_generic_dev_path end;
+} __packed initrd_dev_path = {
+ {
+ EFI_DEV_MEDIA,
+ EFI_DEV_MEDIA_VENDOR,
+ sizeof(struct efi_vendor_dev_path),
+ LINUX_EFI_INITRD_MEDIA_GUID
+ }, {
+ EFI_DEV_END_PATH,
+ EFI_DEV_END_ENTIRE,
+ sizeof(struct efi_generic_dev_path)
+ }
+};
+
+/**
+ * efi_load_initrd_dev_path - load the initrd from the Linux initrd device path
+ * @load_addr: pointer to store the address where the initrd was loaded
+ * @load_size: pointer to store the size of the loaded initrd
+ * @max: upper limit for the initrd memory allocation
+ * @return: %EFI_SUCCESS if the initrd was loaded successfully, in which case
+ * @load_addr and @load_size are assigned accordingly
+ * %EFI_NOT_FOUND if no LoadFile2 protocol exists on the initrd
+ * device path
+ * %EFI_LOAD_ERROR in all other cases
+ */
+efi_status_t efi_load_initrd_dev_path(unsigned long *load_addr,
+ unsigned long *load_size,
+ unsigned long max)
+{
+ efi_guid_t lf2_proto_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
+ efi_device_path_protocol_t *dp;
+ efi_load_file2_protocol_t *lf2;
+ unsigned long initrd_addr;
+ unsigned long initrd_size;
+ efi_handle_t handle;
+ efi_status_t status;
+
+ if (!load_addr || !load_size)
+ return EFI_INVALID_PARAMETER;
+
+ dp = (efi_device_path_protocol_t *)&initrd_dev_path;
+ status = efi_bs_call(locate_device_path, &lf2_proto_guid, &dp, &handle);
+ if (status != EFI_SUCCESS)
+ return status;
+
+ status = efi_bs_call(handle_protocol, handle, &lf2_proto_guid,
+ (void **)&lf2);
+ if (status != EFI_SUCCESS)
+ return status;
+
+ status = efi_call_proto(lf2, load_file, dp, false, &initrd_size, NULL);
+ if (status != EFI_BUFFER_TOO_SMALL)
+ return EFI_LOAD_ERROR;
+
+ status = efi_allocate_pages(initrd_size, &initrd_addr, max);
+ if (status != EFI_SUCCESS)
+ return status;
+
+ status = efi_call_proto(lf2, load_file, dp, false, &initrd_size,
+ (void *)initrd_addr);
+ if (status != EFI_SUCCESS) {
+ efi_free(initrd_size, initrd_addr);
+ return EFI_LOAD_ERROR;
+ }
+
+ *load_addr = initrd_addr;
+ *load_size = initrd_size;
+ return EFI_SUCCESS;
+}
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 34fe3fad042f..b58cb2c4474e 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -640,4 +640,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
unsigned long soft_limit,
unsigned long hard_limit);
+efi_status_t efi_load_initrd_dev_path(unsigned long *load_addr,
+ unsigned long *load_size,
+ unsigned long max);
+
#endif
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 681b620d8d40..16bf4ed21f1f 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -699,9 +699,14 @@ struct boot_params *efi_main(efi_handle_t handle,
{
unsigned long bzimage_addr = (unsigned long)startup_32;
struct setup_header *hdr = &boot_params->hdr;
+ unsigned long max_addr = hdr->initrd_addr_max;
+ unsigned long initrd_addr, initrd_size;
efi_status_t status;
unsigned long cmdline_paddr;
+ if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
+ max_addr = ULONG_MAX;
+
sys_table = sys_table_arg;
/* Check if we were booted by the EFI firmware */
@@ -734,6 +739,24 @@ struct boot_params *efi_main(efi_handle_t handle,
((u64)boot_params->ext_cmd_line_ptr << 32));
efi_parse_options((char *)cmdline_paddr);
+ /*
+ * At this point, an initrd may already have been loaded, either by
+ * the bootloader and passed via bootparams, or loaded from a initrd=
+ * command line option by efi_pe_entry() above. In either case, we
+ * permit an initrd loaded from the LINUX_EFI_INITRD_MEDIA_GUID device
+ * path to supersede it.
+ */
+ status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size, max_addr);
+ if (status == EFI_SUCCESS) {
+ hdr->ramdisk_image = (u32)initrd_addr;
+ hdr->ramdisk_size = (u32)initrd_size;
+ boot_params->ext_ramdisk_image = (u64)initrd_addr >> 32;
+ boot_params->ext_ramdisk_size = (u64)initrd_size >> 32;
+ } else if (status != EFI_NOT_FOUND) {
+ efi_printk("efi_load_initrd_dev_path() failed!\n");
+ goto fail;
+ }
+
/*
* If the boot loader gave us a value for secure_boot then we use that,
* otherwise we ask the BIOS.
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 0976e57b4caa..1bf482daa22d 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -353,6 +353,7 @@ void efi_native_runtime_setup(void);
#define LINUX_EFI_TPM_EVENT_LOG_GUID EFI_GUID(0xb7799cb0, 0xeca2, 0x4943, 0x96, 0x67, 0x1f, 0xae, 0x07, 0xb7, 0x47, 0xfa)
#define LINUX_EFI_TPM_FINAL_LOG_GUID EFI_GUID(0x1e2ed096, 0x30e2, 0x4254, 0xbd, 0x89, 0x86, 0x3b, 0xbe, 0xf8, 0x23, 0x25)
#define LINUX_EFI_MEMRESERVE_TABLE_GUID EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5, 0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
+#define LINUX_EFI_INITRD_MEDIA_GUID EFI_GUID(0x5568e427, 0x68fc, 0x4f3d, 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68)
/* OEM GUIDs */
#define DELLEMC_EFI_RCI2_TABLE_GUID EFI_GUID(0x2d9f28a2, 0xa886, 0x456a, 0x97, 0xa8, 0xf1, 0x1e, 0xf2, 0x4f, 0xf4, 0x55)
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] efi/libstub: Take noinitrd cmdline argument into account for devpath initrd
2020-02-16 14:11 [PATCH v2 0/3] arch-agnostic initrd loading method for EFI systems Ard Biesheuvel
2020-02-16 14:11 ` [PATCH v2 1/3] efi/dev-path-parser: Add struct definition for vendor type device path nodes Ard Biesheuvel
2020-02-16 14:11 ` [PATCH v2 2/3] efi/libstub: Add support for loading the initrd from a device path Ard Biesheuvel
@ 2020-02-16 14:11 ` Ard Biesheuvel
2020-02-17 19:10 ` [PATCH v2 0/3] arch-agnostic initrd loading method for EFI systems Bhupesh Sharma
3 siblings, 0 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2020-02-16 14:11 UTC (permalink / raw)
To: linux-efi
Cc: linux-kernel, Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf,
ilias.apalodimas, xypron.glpk, daniel.kiper, nivedita,
James.Bottomley, lukas
One of the advantages of using what basically amounts to a callback
interface into the bootloader for loading the initrd is that it provides
a natural place for the bootloader or firmware to measure the initrd
contents while they are being passed to the kernel.
Unfortunately, this is not a guarantee that the initrd will in fact be
loaded and its /init invoked by the kernel, since the command line may
contain the 'noinitrd' option, in which case the initrd is ignored, but
this will not be reflected in the PCR that covers the initrd measurement.
This could be addressed by measuring the command line as well, and
including that PCR in the attestation policy, but this locks down the
command line completely, which may be too restrictive.
So let's take the noinitrd argument into account in the stub, too. This
forces any PCR that covers the initrd to assume a different value when
noinitrd is passed, allowing an attestation policy to disregard the
command line if there is no need to take its measurement into account
for other reasons.
As Peter points out, this would still require the agent that takes the
measurements to measure a separator event into the PCR in question at
ExitBootServices() time, to prevent replay attacks using the known
measurement from the TPM log.
Cc: Peter Jones <pjones@redhat.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
drivers/firmware/efi/libstub/arm-stub.c | 25 +++++-----
drivers/firmware/efi/libstub/efi-stub-helper.c | 7 +++
drivers/firmware/efi/libstub/efistub.h | 1 +
drivers/firmware/efi/libstub/x86-stub.c | 52 +++++++++++---------
4 files changed, 51 insertions(+), 34 deletions(-)
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 4bae620b95b9..56e475c1aa55 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -259,18 +259,21 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
if (!fdt_addr)
pr_efi("Generating empty DTB\n");
- max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
- status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size, max_addr);
- if (status == EFI_SUCCESS) {
- pr_efi("Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path\n");
- } else if (status == EFI_NOT_FOUND) {
- status = efi_load_initrd(image, &initrd_addr, &initrd_size,
- ULONG_MAX, max_addr);
- if (status == EFI_SUCCESS)
- pr_efi("Loaded initrd from command line option\n");
+ if (!noinitrd()) {
+ max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
+ status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size,
+ max_addr);
+ if (status == EFI_SUCCESS) {
+ pr_efi("Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path\n");
+ } else if (status == EFI_NOT_FOUND) {
+ status = efi_load_initrd(image, &initrd_addr, &initrd_size,
+ ULONG_MAX, max_addr);
+ if (status == EFI_SUCCESS)
+ pr_efi("Loaded initrd from command line option\n");
+ }
+ if (status != EFI_SUCCESS)
+ pr_efi_err("Failed to load initrd!\n");
}
- if (status != EFI_SUCCESS)
- pr_efi_err("Failed to load initrd!\n");
efi_random_get_seed();
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index e37afe2c752e..c5d04147ed17 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -14,6 +14,7 @@
static bool __efistub_global efi_nochunk;
static bool __efistub_global efi_nokaslr;
+static bool __efistub_global efi_noinitrd;
static bool __efistub_global efi_quiet;
static bool __efistub_global efi_novamap;
static bool __efistub_global efi_nosoftreserve;
@@ -28,6 +29,10 @@ bool __pure nokaslr(void)
{
return efi_nokaslr;
}
+bool __pure noinitrd(void)
+{
+ return efi_noinitrd;
+}
bool __pure is_quiet(void)
{
return efi_quiet;
@@ -87,6 +92,8 @@ efi_status_t efi_parse_options(char const *cmdline)
efi_nokaslr = true;
} else if (!strcmp(param, "quiet")) {
efi_quiet = true;
+ } else if (!strcmp(param, "noinitrd")) {
+ efi_noinitrd = true;
} else if (!strcmp(param, "efi") && val) {
efi_nochunk = parse_option_str(val, "nochunk");
efi_novamap = parse_option_str(val, "novamap");
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index b58cb2c4474e..2e5e79edb4d7 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -33,6 +33,7 @@
extern bool __pure nochunk(void);
extern bool __pure nokaslr(void);
+extern bool __pure noinitrd(void);
extern bool __pure is_quiet(void);
extern bool __pure novamap(void);
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 16bf4ed21f1f..7d4866471f86 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -421,15 +421,18 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
if (status != EFI_SUCCESS)
goto fail2;
- status = efi_load_initrd(image, &ramdisk_addr, &ramdisk_size,
- hdr->initrd_addr_max,
- above4g ? ULONG_MAX : hdr->initrd_addr_max);
- if (status != EFI_SUCCESS)
- goto fail2;
- hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
- hdr->ramdisk_size = ramdisk_size & 0xffffffff;
- boot_params->ext_ramdisk_image = (u64)ramdisk_addr >> 32;
- boot_params->ext_ramdisk_size = (u64)ramdisk_size >> 32;
+ if (!noinitrd()) {
+ status = efi_load_initrd(image, &ramdisk_addr, &ramdisk_size,
+ hdr->initrd_addr_max,
+ above4g ? ULONG_MAX
+ : hdr->initrd_addr_max);
+ if (status != EFI_SUCCESS)
+ goto fail2;
+ hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
+ hdr->ramdisk_size = ramdisk_size & 0xffffffff;
+ boot_params->ext_ramdisk_image = (u64)ramdisk_addr >> 32;
+ boot_params->ext_ramdisk_size = (u64)ramdisk_size >> 32;
+ }
efi_stub_entry(handle, sys_table, boot_params);
/* not reached */
@@ -699,14 +702,9 @@ struct boot_params *efi_main(efi_handle_t handle,
{
unsigned long bzimage_addr = (unsigned long)startup_32;
struct setup_header *hdr = &boot_params->hdr;
- unsigned long max_addr = hdr->initrd_addr_max;
- unsigned long initrd_addr, initrd_size;
efi_status_t status;
unsigned long cmdline_paddr;
- if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
- max_addr = ULONG_MAX;
-
sys_table = sys_table_arg;
/* Check if we were booted by the EFI firmware */
@@ -746,15 +744,23 @@ struct boot_params *efi_main(efi_handle_t handle,
* permit an initrd loaded from the LINUX_EFI_INITRD_MEDIA_GUID device
* path to supersede it.
*/
- status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size, max_addr);
- if (status == EFI_SUCCESS) {
- hdr->ramdisk_image = (u32)initrd_addr;
- hdr->ramdisk_size = (u32)initrd_size;
- boot_params->ext_ramdisk_image = (u64)initrd_addr >> 32;
- boot_params->ext_ramdisk_size = (u64)initrd_size >> 32;
- } else if (status != EFI_NOT_FOUND) {
- efi_printk("efi_load_initrd_dev_path() failed!\n");
- goto fail;
+ if (!noinitrd()) {
+ unsigned long addr, size;
+ unsigned long max_addr = hdr->initrd_addr_max;
+
+ if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
+ max_addr = ULONG_MAX;
+
+ status = efi_load_initrd_dev_path(&addr, &size, max_addr);
+ if (status == EFI_SUCCESS) {
+ hdr->ramdisk_image = (u32)addr;
+ hdr->ramdisk_size = (u32)size;
+ boot_params->ext_ramdisk_image = (u64)addr >> 32;
+ boot_params->ext_ramdisk_size = (u64)size >> 32;
+ } else if (status != EFI_NOT_FOUND) {
+ efi_printk("efi_load_initrd_dev_path() failed!\n");
+ goto fail;
+ }
}
/*
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] efi/libstub: Add support for loading the initrd from a device path
2020-02-16 14:11 ` [PATCH v2 2/3] efi/libstub: Add support for loading the initrd from a device path Ard Biesheuvel
@ 2020-02-17 9:15 ` Laszlo Ersek
2020-02-17 9:23 ` Ard Biesheuvel
0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2020-02-17 9:15 UTC (permalink / raw)
To: Ard Biesheuvel, linux-efi
Cc: linux-kernel, leif, pjones, mjg59, agraf, ilias.apalodimas,
xypron.glpk, daniel.kiper, nivedita, James.Bottomley, lukas
On 02/16/20 15:11, Ard Biesheuvel wrote:
> There are currently two ways to specify the initrd to be passed to the
> Linux kernel when booting via the EFI stub:
> - it can be passed as a initrd= command line option when doing a pure PE
> boot (as opposed to the EFI handover protocol that exists for x86)
> - otherwise, the bootloader or firmware can load the initrd into memory,
> and pass the address and size via the bootparams struct (x86) or
> device tree (ARM)
>
> In the first case, we are limited to loading from the same file system
> that the kernel was loaded from, and it is also problematic in a trusted
> boot context, given that we cannot easily protect the command line from
> tampering without either adding complicated white/blacklisting of boot
> arguments or locking down the command line altogether.
>
> In the second case, we force the bootloader to duplicate knowledge about
> the boot protocol which is already encoded in the stub, and which may be
> subject to change over time, e.g., bootparams struct definitions, memory
> allocation/alignment requirements for the placement of the initrd etc etc.
> In the ARM case, it also requires the bootloader to modify the hardware
> description provided by the firmware, as it is passed in the same file.
> On systems where the initrd is measured after loading, it creates a time
> window where the initrd contents might be manipulated in memory before
> handing over to the kernel.
>
> Address these concerns by adding support for loading the initrd into
> memory by invoking the EFI LoadFile2 protocol installed on a vendor
> GUIDed device path that specifically designates a Linux initrd.
> This addresses the above concerns, by putting the EFI stub in charge of
> placement in memory and of passing the base and size to the kernel proper
> (via whatever means it desires) while still leaving it up to the firmware
> or bootloader to obtain the file contents, potentially from other file
> systems than the one the kernel itself was loaded from. On platforms that
> implement measured boot, it permits the firmware to take the measurement
> right before the kernel actually consumes the contents.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> drivers/firmware/efi/libstub/arm-stub.c | 15 +++-
> drivers/firmware/efi/libstub/efi-stub-helper.c | 82 ++++++++++++++++++++
> drivers/firmware/efi/libstub/efistub.h | 4 +
> drivers/firmware/efi/libstub/x86-stub.c | 23 ++++++
> include/linux/efi.h | 1 +
> 5 files changed, 122 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index 2edc673ea06c..4bae620b95b9 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -160,6 +160,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
> enum efi_secureboot_mode secure_boot;
> struct screen_info *si;
> efi_properties_table_t *prop_tbl;
> + unsigned long max_addr;
>
> sys_table = sys_table_arg;
>
> @@ -258,10 +259,18 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
> if (!fdt_addr)
> pr_efi("Generating empty DTB\n");
>
> - status = efi_load_initrd(image, &initrd_addr, &initrd_size, ULONG_MAX,
> - efi_get_max_initrd_addr(dram_base, *image_addr));
> + max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
> + status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size, max_addr);
> + if (status == EFI_SUCCESS) {
> + pr_efi("Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path\n");
> + } else if (status == EFI_NOT_FOUND) {
> + status = efi_load_initrd(image, &initrd_addr, &initrd_size,
> + ULONG_MAX, max_addr);
> + if (status == EFI_SUCCESS)
> + pr_efi("Loaded initrd from command line option\n");
> + }
> if (status != EFI_SUCCESS)
> - pr_efi_err("Failed initrd from command line!\n");
> + pr_efi_err("Failed to load initrd!\n");
>
> efi_random_get_seed();
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 49008ac88b63..e37afe2c752e 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -299,3 +299,85 @@ void efi_char16_printk(efi_char16_t *str)
> efi_call_proto(efi_table_attr(efi_system_table(), con_out),
> output_string, str);
> }
> +
> +/*
> + * The LINUX_EFI_INITRD_MEDIA_GUID vendor media device path below provides a way
> + * for the firmware or bootloader to expose the initrd data directly to the stub
> + * via the trivial LoadFile2 protocol, which is defined in the UEFI spec, and is
> + * very easy to implement. It is a simple Linux initrd specific conduit between
> + * kernel and firmware, allowing us to put the EFI stub (being part of the
> + * kernel) in charge of where and when to load the initrd, while leaving it up
> + * to the firmware to decide whether it needs to expose its filesystem hierarchy
> + * via EFI protocols.
> + */
> +static const struct {
> + struct efi_vendor_dev_path vendor;
> + struct efi_generic_dev_path end;
> +} __packed initrd_dev_path = {
> + {
> + EFI_DEV_MEDIA,
> + EFI_DEV_MEDIA_VENDOR,
> + sizeof(struct efi_vendor_dev_path),
> + LINUX_EFI_INITRD_MEDIA_GUID
> + }, {
> + EFI_DEV_END_PATH,
> + EFI_DEV_END_ENTIRE,
> + sizeof(struct efi_generic_dev_path)
> + }
> +};
> +
> +/**
> + * efi_load_initrd_dev_path - load the initrd from the Linux initrd device path
> + * @load_addr: pointer to store the address where the initrd was loaded
> + * @load_size: pointer to store the size of the loaded initrd
> + * @max: upper limit for the initrd memory allocation
> + * @return: %EFI_SUCCESS if the initrd was loaded successfully, in which case
> + * @load_addr and @load_size are assigned accordingly
> + * %EFI_NOT_FOUND if no LoadFile2 protocol exists on the initrd
> + * device path
> + * %EFI_LOAD_ERROR in all other cases
[*]
> + */
> +efi_status_t efi_load_initrd_dev_path(unsigned long *load_addr,
> + unsigned long *load_size,
> + unsigned long max)
> +{
> + efi_guid_t lf2_proto_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
> + efi_device_path_protocol_t *dp;
> + efi_load_file2_protocol_t *lf2;
> + unsigned long initrd_addr;
> + unsigned long initrd_size;
> + efi_handle_t handle;
> + efi_status_t status;
> +
> + if (!load_addr || !load_size)
> + return EFI_INVALID_PARAMETER;
Doesn't return EFI_LOAD_ERROR.
> +
> + dp = (efi_device_path_protocol_t *)&initrd_dev_path;
> + status = efi_bs_call(locate_device_path, &lf2_proto_guid, &dp, &handle);
> + if (status != EFI_SUCCESS)
> + return status;
Seems safe (the only plausible error could be EFI_NOT_FOUND).
> +
> + status = efi_bs_call(handle_protocol, handle, &lf2_proto_guid,
> + (void **)&lf2);
> + if (status != EFI_SUCCESS)
> + return status;
Interesting case; this should never fail... but note, if it does, it
returns EFI_UNSUPPORTED, not EFI_NOT_FOUND (if the protocol is missing
from the handle).
> +
> + status = efi_call_proto(lf2, load_file, dp, false, &initrd_size, NULL);
> + if (status != EFI_BUFFER_TOO_SMALL)
> + return EFI_LOAD_ERROR;
> +
> + status = efi_allocate_pages(initrd_size, &initrd_addr, max);
> + if (status != EFI_SUCCESS)
> + return status;
Not sure about the efi_allocate_pages() wrapper (?); the UEFI service
could return EFI_OUT_OF_RESOURCES.
Looks OK to me otherwise.
(... I'm a bit doubtful of passing and End node to LF2 rather than a
filepath node with "" for pathname, but it's an LF2 on our own vendor
path, so I guess we dictate what we accept.)
Thanks
Laszlo
> +
> + status = efi_call_proto(lf2, load_file, dp, false, &initrd_size,
> + (void *)initrd_addr);
> + if (status != EFI_SUCCESS) {
> + efi_free(initrd_size, initrd_addr);
> + return EFI_LOAD_ERROR;
> + }
> +
> + *load_addr = initrd_addr;
> + *load_size = initrd_size;
> + return EFI_SUCCESS;
> +}
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 34fe3fad042f..b58cb2c4474e 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -640,4 +640,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
> unsigned long soft_limit,
> unsigned long hard_limit);
>
> +efi_status_t efi_load_initrd_dev_path(unsigned long *load_addr,
> + unsigned long *load_size,
> + unsigned long max);
> +
> #endif
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 681b620d8d40..16bf4ed21f1f 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -699,9 +699,14 @@ struct boot_params *efi_main(efi_handle_t handle,
> {
> unsigned long bzimage_addr = (unsigned long)startup_32;
> struct setup_header *hdr = &boot_params->hdr;
> + unsigned long max_addr = hdr->initrd_addr_max;
> + unsigned long initrd_addr, initrd_size;
> efi_status_t status;
> unsigned long cmdline_paddr;
>
> + if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
> + max_addr = ULONG_MAX;
> +
> sys_table = sys_table_arg;
>
> /* Check if we were booted by the EFI firmware */
> @@ -734,6 +739,24 @@ struct boot_params *efi_main(efi_handle_t handle,
> ((u64)boot_params->ext_cmd_line_ptr << 32));
> efi_parse_options((char *)cmdline_paddr);
>
> + /*
> + * At this point, an initrd may already have been loaded, either by
> + * the bootloader and passed via bootparams, or loaded from a initrd=
> + * command line option by efi_pe_entry() above. In either case, we
> + * permit an initrd loaded from the LINUX_EFI_INITRD_MEDIA_GUID device
> + * path to supersede it.
> + */
> + status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size, max_addr);
> + if (status == EFI_SUCCESS) {
> + hdr->ramdisk_image = (u32)initrd_addr;
> + hdr->ramdisk_size = (u32)initrd_size;
> + boot_params->ext_ramdisk_image = (u64)initrd_addr >> 32;
> + boot_params->ext_ramdisk_size = (u64)initrd_size >> 32;
> + } else if (status != EFI_NOT_FOUND) {
> + efi_printk("efi_load_initrd_dev_path() failed!\n");
> + goto fail;
> + }
> +
> /*
> * If the boot loader gave us a value for secure_boot then we use that,
> * otherwise we ask the BIOS.
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 0976e57b4caa..1bf482daa22d 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -353,6 +353,7 @@ void efi_native_runtime_setup(void);
> #define LINUX_EFI_TPM_EVENT_LOG_GUID EFI_GUID(0xb7799cb0, 0xeca2, 0x4943, 0x96, 0x67, 0x1f, 0xae, 0x07, 0xb7, 0x47, 0xfa)
> #define LINUX_EFI_TPM_FINAL_LOG_GUID EFI_GUID(0x1e2ed096, 0x30e2, 0x4254, 0xbd, 0x89, 0x86, 0x3b, 0xbe, 0xf8, 0x23, 0x25)
> #define LINUX_EFI_MEMRESERVE_TABLE_GUID EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5, 0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
> +#define LINUX_EFI_INITRD_MEDIA_GUID EFI_GUID(0x5568e427, 0x68fc, 0x4f3d, 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68)
>
> /* OEM GUIDs */
> #define DELLEMC_EFI_RCI2_TABLE_GUID EFI_GUID(0x2d9f28a2, 0xa886, 0x456a, 0x97, 0xa8, 0xf1, 0x1e, 0xf2, 0x4f, 0xf4, 0x55)
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] efi/libstub: Add support for loading the initrd from a device path
2020-02-17 9:15 ` Laszlo Ersek
@ 2020-02-17 9:23 ` Ard Biesheuvel
2020-02-17 10:01 ` Laszlo Ersek
2020-02-17 10:22 ` Ard Biesheuvel
0 siblings, 2 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2020-02-17 9:23 UTC (permalink / raw)
To: Laszlo Ersek
Cc: linux-efi, Linux Kernel Mailing List, Leif Lindholm, Peter Jones,
Matthew Garrett, Alexander Graf, Ilias Apalodimas,
Heinrich Schuchardt, Daniel Kiper, Arvind Sankar,
James Bottomley, Lukas Wunner
On Mon, 17 Feb 2020 at 10:15, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 02/16/20 15:11, Ard Biesheuvel wrote:
> > There are currently two ways to specify the initrd to be passed to the
> > Linux kernel when booting via the EFI stub:
> > - it can be passed as a initrd= command line option when doing a pure PE
> > boot (as opposed to the EFI handover protocol that exists for x86)
> > - otherwise, the bootloader or firmware can load the initrd into memory,
> > and pass the address and size via the bootparams struct (x86) or
> > device tree (ARM)
> >
> > In the first case, we are limited to loading from the same file system
> > that the kernel was loaded from, and it is also problematic in a trusted
> > boot context, given that we cannot easily protect the command line from
> > tampering without either adding complicated white/blacklisting of boot
> > arguments or locking down the command line altogether.
> >
> > In the second case, we force the bootloader to duplicate knowledge about
> > the boot protocol which is already encoded in the stub, and which may be
> > subject to change over time, e.g., bootparams struct definitions, memory
> > allocation/alignment requirements for the placement of the initrd etc etc.
> > In the ARM case, it also requires the bootloader to modify the hardware
> > description provided by the firmware, as it is passed in the same file.
> > On systems where the initrd is measured after loading, it creates a time
> > window where the initrd contents might be manipulated in memory before
> > handing over to the kernel.
> >
> > Address these concerns by adding support for loading the initrd into
> > memory by invoking the EFI LoadFile2 protocol installed on a vendor
> > GUIDed device path that specifically designates a Linux initrd.
> > This addresses the above concerns, by putting the EFI stub in charge of
> > placement in memory and of passing the base and size to the kernel proper
> > (via whatever means it desires) while still leaving it up to the firmware
> > or bootloader to obtain the file contents, potentially from other file
> > systems than the one the kernel itself was loaded from. On platforms that
> > implement measured boot, it permits the firmware to take the measurement
> > right before the kernel actually consumes the contents.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > drivers/firmware/efi/libstub/arm-stub.c | 15 +++-
> > drivers/firmware/efi/libstub/efi-stub-helper.c | 82 ++++++++++++++++++++
> > drivers/firmware/efi/libstub/efistub.h | 4 +
> > drivers/firmware/efi/libstub/x86-stub.c | 23 ++++++
> > include/linux/efi.h | 1 +
> > 5 files changed, 122 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> > index 2edc673ea06c..4bae620b95b9 100644
> > --- a/drivers/firmware/efi/libstub/arm-stub.c
> > +++ b/drivers/firmware/efi/libstub/arm-stub.c
> > @@ -160,6 +160,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
> > enum efi_secureboot_mode secure_boot;
> > struct screen_info *si;
> > efi_properties_table_t *prop_tbl;
> > + unsigned long max_addr;
> >
> > sys_table = sys_table_arg;
> >
> > @@ -258,10 +259,18 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
> > if (!fdt_addr)
> > pr_efi("Generating empty DTB\n");
> >
> > - status = efi_load_initrd(image, &initrd_addr, &initrd_size, ULONG_MAX,
> > - efi_get_max_initrd_addr(dram_base, *image_addr));
> > + max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
> > + status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size, max_addr);
> > + if (status == EFI_SUCCESS) {
> > + pr_efi("Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path\n");
> > + } else if (status == EFI_NOT_FOUND) {
> > + status = efi_load_initrd(image, &initrd_addr, &initrd_size,
> > + ULONG_MAX, max_addr);
> > + if (status == EFI_SUCCESS)
> > + pr_efi("Loaded initrd from command line option\n");
> > + }
> > if (status != EFI_SUCCESS)
> > - pr_efi_err("Failed initrd from command line!\n");
> > + pr_efi_err("Failed to load initrd!\n");
> >
> > efi_random_get_seed();
> >
> > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > index 49008ac88b63..e37afe2c752e 100644
> > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > @@ -299,3 +299,85 @@ void efi_char16_printk(efi_char16_t *str)
> > efi_call_proto(efi_table_attr(efi_system_table(), con_out),
> > output_string, str);
> > }
> > +
> > +/*
> > + * The LINUX_EFI_INITRD_MEDIA_GUID vendor media device path below provides a way
> > + * for the firmware or bootloader to expose the initrd data directly to the stub
> > + * via the trivial LoadFile2 protocol, which is defined in the UEFI spec, and is
> > + * very easy to implement. It is a simple Linux initrd specific conduit between
> > + * kernel and firmware, allowing us to put the EFI stub (being part of the
> > + * kernel) in charge of where and when to load the initrd, while leaving it up
> > + * to the firmware to decide whether it needs to expose its filesystem hierarchy
> > + * via EFI protocols.
> > + */
> > +static const struct {
> > + struct efi_vendor_dev_path vendor;
> > + struct efi_generic_dev_path end;
> > +} __packed initrd_dev_path = {
> > + {
> > + EFI_DEV_MEDIA,
> > + EFI_DEV_MEDIA_VENDOR,
> > + sizeof(struct efi_vendor_dev_path),
> > + LINUX_EFI_INITRD_MEDIA_GUID
> > + }, {
> > + EFI_DEV_END_PATH,
> > + EFI_DEV_END_ENTIRE,
> > + sizeof(struct efi_generic_dev_path)
> > + }
> > +};
> > +
> > +/**
> > + * efi_load_initrd_dev_path - load the initrd from the Linux initrd device path
> > + * @load_addr: pointer to store the address where the initrd was loaded
> > + * @load_size: pointer to store the size of the loaded initrd
> > + * @max: upper limit for the initrd memory allocation
> > + * @return: %EFI_SUCCESS if the initrd was loaded successfully, in which case
> > + * @load_addr and @load_size are assigned accordingly
> > + * %EFI_NOT_FOUND if no LoadFile2 protocol exists on the initrd
> > + * device path
> > + * %EFI_LOAD_ERROR in all other cases
>
> [*]
>
> > + */
> > +efi_status_t efi_load_initrd_dev_path(unsigned long *load_addr,
> > + unsigned long *load_size,
> > + unsigned long max)
> > +{
> > + efi_guid_t lf2_proto_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
> > + efi_device_path_protocol_t *dp;
> > + efi_load_file2_protocol_t *lf2;
> > + unsigned long initrd_addr;
> > + unsigned long initrd_size;
> > + efi_handle_t handle;
> > + efi_status_t status;
> > +
> > + if (!load_addr || !load_size)
> > + return EFI_INVALID_PARAMETER;
>
> Doesn't return EFI_LOAD_ERROR.
>
> > +
> > + dp = (efi_device_path_protocol_t *)&initrd_dev_path;
> > + status = efi_bs_call(locate_device_path, &lf2_proto_guid, &dp, &handle);
> > + if (status != EFI_SUCCESS)
> > + return status;
>
> Seems safe (the only plausible error could be EFI_NOT_FOUND).
>
> > +
> > + status = efi_bs_call(handle_protocol, handle, &lf2_proto_guid,
> > + (void **)&lf2);
> > + if (status != EFI_SUCCESS)
> > + return status;
>
> Interesting case; this should never fail... but note, if it does, it
> returns EFI_UNSUPPORTED, not EFI_NOT_FOUND (if the protocol is missing
> from the handle).
>
> > +
> > + status = efi_call_proto(lf2, load_file, dp, false, &initrd_size, NULL);
> > + if (status != EFI_BUFFER_TOO_SMALL)
> > + return EFI_LOAD_ERROR;
> > +
> > + status = efi_allocate_pages(initrd_size, &initrd_addr, max);
> > + if (status != EFI_SUCCESS)
> > + return status;
>
> Not sure about the efi_allocate_pages() wrapper (?); the UEFI service
> could return EFI_OUT_OF_RESOURCES.
>
Hmm, guess I was a bit sloppy with the return codes. The important
thing is that EFI_NOT_FOUND is only returned in the one specifically
defined case.
> Looks OK to me otherwise.
>
Thanks.
> (... I'm a bit doubtful of passing and End node to LF2 rather than a
> filepath node with "" for pathname, but it's an LF2 on our own vendor
> path, so I guess we dictate what we accept.)
>
It seems to me that the whole point of advancing the
pointer-to-pointer-to-device path protocol past the matched part of
the device path is to make it straight-forward for the caller to pass
the remainder into the protocol implementation. So if the matched part
fully defines the target, pointing to an end node is the only correct
thing to do imo, as the empty file did not appear in the device path
that was used to locate the protocol.
> > +
> > + status = efi_call_proto(lf2, load_file, dp, false, &initrd_size,
> > + (void *)initrd_addr);
> > + if (status != EFI_SUCCESS) {
> > + efi_free(initrd_size, initrd_addr);
> > + return EFI_LOAD_ERROR;
> > + }
> > +
> > + *load_addr = initrd_addr;
> > + *load_size = initrd_size;
> > + return EFI_SUCCESS;
> > +}
> > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> > index 34fe3fad042f..b58cb2c4474e 100644
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -640,4 +640,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
> > unsigned long soft_limit,
> > unsigned long hard_limit);
> >
> > +efi_status_t efi_load_initrd_dev_path(unsigned long *load_addr,
> > + unsigned long *load_size,
> > + unsigned long max);
> > +
> > #endif
> > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > index 681b620d8d40..16bf4ed21f1f 100644
> > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > @@ -699,9 +699,14 @@ struct boot_params *efi_main(efi_handle_t handle,
> > {
> > unsigned long bzimage_addr = (unsigned long)startup_32;
> > struct setup_header *hdr = &boot_params->hdr;
> > + unsigned long max_addr = hdr->initrd_addr_max;
> > + unsigned long initrd_addr, initrd_size;
> > efi_status_t status;
> > unsigned long cmdline_paddr;
> >
> > + if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
> > + max_addr = ULONG_MAX;
> > +
> > sys_table = sys_table_arg;
> >
> > /* Check if we were booted by the EFI firmware */
> > @@ -734,6 +739,24 @@ struct boot_params *efi_main(efi_handle_t handle,
> > ((u64)boot_params->ext_cmd_line_ptr << 32));
> > efi_parse_options((char *)cmdline_paddr);
> >
> > + /*
> > + * At this point, an initrd may already have been loaded, either by
> > + * the bootloader and passed via bootparams, or loaded from a initrd=
> > + * command line option by efi_pe_entry() above. In either case, we
> > + * permit an initrd loaded from the LINUX_EFI_INITRD_MEDIA_GUID device
> > + * path to supersede it.
> > + */
> > + status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size, max_addr);
> > + if (status == EFI_SUCCESS) {
> > + hdr->ramdisk_image = (u32)initrd_addr;
> > + hdr->ramdisk_size = (u32)initrd_size;
> > + boot_params->ext_ramdisk_image = (u64)initrd_addr >> 32;
> > + boot_params->ext_ramdisk_size = (u64)initrd_size >> 32;
> > + } else if (status != EFI_NOT_FOUND) {
> > + efi_printk("efi_load_initrd_dev_path() failed!\n");
> > + goto fail;
> > + }
> > +
> > /*
> > * If the boot loader gave us a value for secure_boot then we use that,
> > * otherwise we ask the BIOS.
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 0976e57b4caa..1bf482daa22d 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -353,6 +353,7 @@ void efi_native_runtime_setup(void);
> > #define LINUX_EFI_TPM_EVENT_LOG_GUID EFI_GUID(0xb7799cb0, 0xeca2, 0x4943, 0x96, 0x67, 0x1f, 0xae, 0x07, 0xb7, 0x47, 0xfa)
> > #define LINUX_EFI_TPM_FINAL_LOG_GUID EFI_GUID(0x1e2ed096, 0x30e2, 0x4254, 0xbd, 0x89, 0x86, 0x3b, 0xbe, 0xf8, 0x23, 0x25)
> > #define LINUX_EFI_MEMRESERVE_TABLE_GUID EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5, 0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
> > +#define LINUX_EFI_INITRD_MEDIA_GUID EFI_GUID(0x5568e427, 0x68fc, 0x4f3d, 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68)
> >
> > /* OEM GUIDs */
> > #define DELLEMC_EFI_RCI2_TABLE_GUID EFI_GUID(0x2d9f28a2, 0xa886, 0x456a, 0x97, 0xa8, 0xf1, 0x1e, 0xf2, 0x4f, 0xf4, 0x55)
> >
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] efi/libstub: Add support for loading the initrd from a device path
2020-02-17 9:23 ` Ard Biesheuvel
@ 2020-02-17 10:01 ` Laszlo Ersek
2020-02-17 10:22 ` Ard Biesheuvel
1 sibling, 0 replies; 13+ messages in thread
From: Laszlo Ersek @ 2020-02-17 10:01 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-efi, Linux Kernel Mailing List, Leif Lindholm, Peter Jones,
Matthew Garrett, Alexander Graf, Ilias Apalodimas,
Heinrich Schuchardt, Daniel Kiper, Arvind Sankar,
James Bottomley, Lukas Wunner
On 02/17/20 10:23, Ard Biesheuvel wrote:
> On Mon, 17 Feb 2020 at 10:15, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 02/16/20 15:11, Ard Biesheuvel wrote:
>>> There are currently two ways to specify the initrd to be passed to the
>>> Linux kernel when booting via the EFI stub:
>>> - it can be passed as a initrd= command line option when doing a pure PE
>>> boot (as opposed to the EFI handover protocol that exists for x86)
>>> - otherwise, the bootloader or firmware can load the initrd into memory,
>>> and pass the address and size via the bootparams struct (x86) or
>>> device tree (ARM)
>>>
>>> In the first case, we are limited to loading from the same file system
>>> that the kernel was loaded from, and it is also problematic in a trusted
>>> boot context, given that we cannot easily protect the command line from
>>> tampering without either adding complicated white/blacklisting of boot
>>> arguments or locking down the command line altogether.
>>>
>>> In the second case, we force the bootloader to duplicate knowledge about
>>> the boot protocol which is already encoded in the stub, and which may be
>>> subject to change over time, e.g., bootparams struct definitions, memory
>>> allocation/alignment requirements for the placement of the initrd etc etc.
>>> In the ARM case, it also requires the bootloader to modify the hardware
>>> description provided by the firmware, as it is passed in the same file.
>>> On systems where the initrd is measured after loading, it creates a time
>>> window where the initrd contents might be manipulated in memory before
>>> handing over to the kernel.
>>>
>>> Address these concerns by adding support for loading the initrd into
>>> memory by invoking the EFI LoadFile2 protocol installed on a vendor
>>> GUIDed device path that specifically designates a Linux initrd.
>>> This addresses the above concerns, by putting the EFI stub in charge of
>>> placement in memory and of passing the base and size to the kernel proper
>>> (via whatever means it desires) while still leaving it up to the firmware
>>> or bootloader to obtain the file contents, potentially from other file
>>> systems than the one the kernel itself was loaded from. On platforms that
>>> implement measured boot, it permits the firmware to take the measurement
>>> right before the kernel actually consumes the contents.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>> drivers/firmware/efi/libstub/arm-stub.c | 15 +++-
>>> drivers/firmware/efi/libstub/efi-stub-helper.c | 82 ++++++++++++++++++++
>>> drivers/firmware/efi/libstub/efistub.h | 4 +
>>> drivers/firmware/efi/libstub/x86-stub.c | 23 ++++++
>>> include/linux/efi.h | 1 +
>>> 5 files changed, 122 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>>> index 2edc673ea06c..4bae620b95b9 100644
>>> --- a/drivers/firmware/efi/libstub/arm-stub.c
>>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>>> @@ -160,6 +160,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
>>> enum efi_secureboot_mode secure_boot;
>>> struct screen_info *si;
>>> efi_properties_table_t *prop_tbl;
>>> + unsigned long max_addr;
>>>
>>> sys_table = sys_table_arg;
>>>
>>> @@ -258,10 +259,18 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
>>> if (!fdt_addr)
>>> pr_efi("Generating empty DTB\n");
>>>
>>> - status = efi_load_initrd(image, &initrd_addr, &initrd_size, ULONG_MAX,
>>> - efi_get_max_initrd_addr(dram_base, *image_addr));
>>> + max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
>>> + status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size, max_addr);
>>> + if (status == EFI_SUCCESS) {
>>> + pr_efi("Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path\n");
>>> + } else if (status == EFI_NOT_FOUND) {
>>> + status = efi_load_initrd(image, &initrd_addr, &initrd_size,
>>> + ULONG_MAX, max_addr);
>>> + if (status == EFI_SUCCESS)
>>> + pr_efi("Loaded initrd from command line option\n");
>>> + }
>>> if (status != EFI_SUCCESS)
>>> - pr_efi_err("Failed initrd from command line!\n");
>>> + pr_efi_err("Failed to load initrd!\n");
>>>
>>> efi_random_get_seed();
>>>
>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>> index 49008ac88b63..e37afe2c752e 100644
>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>> @@ -299,3 +299,85 @@ void efi_char16_printk(efi_char16_t *str)
>>> efi_call_proto(efi_table_attr(efi_system_table(), con_out),
>>> output_string, str);
>>> }
>>> +
>>> +/*
>>> + * The LINUX_EFI_INITRD_MEDIA_GUID vendor media device path below provides a way
>>> + * for the firmware or bootloader to expose the initrd data directly to the stub
>>> + * via the trivial LoadFile2 protocol, which is defined in the UEFI spec, and is
>>> + * very easy to implement. It is a simple Linux initrd specific conduit between
>>> + * kernel and firmware, allowing us to put the EFI stub (being part of the
>>> + * kernel) in charge of where and when to load the initrd, while leaving it up
>>> + * to the firmware to decide whether it needs to expose its filesystem hierarchy
>>> + * via EFI protocols.
>>> + */
>>> +static const struct {
>>> + struct efi_vendor_dev_path vendor;
>>> + struct efi_generic_dev_path end;
>>> +} __packed initrd_dev_path = {
>>> + {
>>> + EFI_DEV_MEDIA,
>>> + EFI_DEV_MEDIA_VENDOR,
>>> + sizeof(struct efi_vendor_dev_path),
>>> + LINUX_EFI_INITRD_MEDIA_GUID
>>> + }, {
>>> + EFI_DEV_END_PATH,
>>> + EFI_DEV_END_ENTIRE,
>>> + sizeof(struct efi_generic_dev_path)
>>> + }
>>> +};
>>> +
>>> +/**
>>> + * efi_load_initrd_dev_path - load the initrd from the Linux initrd device path
>>> + * @load_addr: pointer to store the address where the initrd was loaded
>>> + * @load_size: pointer to store the size of the loaded initrd
>>> + * @max: upper limit for the initrd memory allocation
>>> + * @return: %EFI_SUCCESS if the initrd was loaded successfully, in which case
>>> + * @load_addr and @load_size are assigned accordingly
>>> + * %EFI_NOT_FOUND if no LoadFile2 protocol exists on the initrd
>>> + * device path
>>> + * %EFI_LOAD_ERROR in all other cases
>>
>> [*]
>>
>>> + */
>>> +efi_status_t efi_load_initrd_dev_path(unsigned long *load_addr,
>>> + unsigned long *load_size,
>>> + unsigned long max)
>>> +{
>>> + efi_guid_t lf2_proto_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
>>> + efi_device_path_protocol_t *dp;
>>> + efi_load_file2_protocol_t *lf2;
>>> + unsigned long initrd_addr;
>>> + unsigned long initrd_size;
>>> + efi_handle_t handle;
>>> + efi_status_t status;
>>> +
>>> + if (!load_addr || !load_size)
>>> + return EFI_INVALID_PARAMETER;
>>
>> Doesn't return EFI_LOAD_ERROR.
>>
>>> +
>>> + dp = (efi_device_path_protocol_t *)&initrd_dev_path;
>>> + status = efi_bs_call(locate_device_path, &lf2_proto_guid, &dp, &handle);
>>> + if (status != EFI_SUCCESS)
>>> + return status;
>>
>> Seems safe (the only plausible error could be EFI_NOT_FOUND).
>>
>>> +
>>> + status = efi_bs_call(handle_protocol, handle, &lf2_proto_guid,
>>> + (void **)&lf2);
>>> + if (status != EFI_SUCCESS)
>>> + return status;
>>
>> Interesting case; this should never fail... but note, if it does, it
>> returns EFI_UNSUPPORTED, not EFI_NOT_FOUND (if the protocol is missing
>> from the handle).
>>
>>> +
>>> + status = efi_call_proto(lf2, load_file, dp, false, &initrd_size, NULL);
>>> + if (status != EFI_BUFFER_TOO_SMALL)
>>> + return EFI_LOAD_ERROR;
>>> +
>>> + status = efi_allocate_pages(initrd_size, &initrd_addr, max);
>>> + if (status != EFI_SUCCESS)
>>> + return status;
>>
>> Not sure about the efi_allocate_pages() wrapper (?); the UEFI service
>> could return EFI_OUT_OF_RESOURCES.
>>
>
> Hmm, guess I was a bit sloppy with the return codes. The important
> thing is that EFI_NOT_FOUND is only returned in the one specifically
> defined case.
>
>> Looks OK to me otherwise.
>>
>
> Thanks.
>
>> (... I'm a bit doubtful of passing and End node to LF2 rather than a
>> filepath node with "" for pathname, but it's an LF2 on our own vendor
>> path, so I guess we dictate what we accept.)
>>
>
> It seems to me that the whole point of advancing the
> pointer-to-pointer-to-device path protocol past the matched part of
> the device path is to make it straight-forward for the caller to pass
> the remainder into the protocol implementation. So if the matched part
> fully defines the target, pointing to an end node is the only correct
> thing to do imo, as the empty file did not appear in the device path
> that was used to locate the protocol.
Fair enough.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] efi/libstub: Add support for loading the initrd from a device path
2020-02-17 9:23 ` Ard Biesheuvel
2020-02-17 10:01 ` Laszlo Ersek
@ 2020-02-17 10:22 ` Ard Biesheuvel
2020-02-17 10:26 ` Laszlo Ersek
2020-02-17 11:09 ` Ilias Apalodimas
1 sibling, 2 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2020-02-17 10:22 UTC (permalink / raw)
To: Laszlo Ersek
Cc: linux-efi, Linux Kernel Mailing List, Leif Lindholm, Peter Jones,
Matthew Garrett, Alexander Graf, Ilias Apalodimas,
Heinrich Schuchardt, Daniel Kiper, Arvind Sankar,
James Bottomley, Lukas Wunner
On Mon, 17 Feb 2020 at 10:23, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 17 Feb 2020 at 10:15, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > On 02/16/20 15:11, Ard Biesheuvel wrote:
> > > There are currently two ways to specify the initrd to be passed to the
> > > Linux kernel when booting via the EFI stub:
> > > - it can be passed as a initrd= command line option when doing a pure PE
> > > boot (as opposed to the EFI handover protocol that exists for x86)
> > > - otherwise, the bootloader or firmware can load the initrd into memory,
> > > and pass the address and size via the bootparams struct (x86) or
> > > device tree (ARM)
> > >
> > > In the first case, we are limited to loading from the same file system
> > > that the kernel was loaded from, and it is also problematic in a trusted
> > > boot context, given that we cannot easily protect the command line from
> > > tampering without either adding complicated white/blacklisting of boot
> > > arguments or locking down the command line altogether.
> > >
> > > In the second case, we force the bootloader to duplicate knowledge about
> > > the boot protocol which is already encoded in the stub, and which may be
> > > subject to change over time, e.g., bootparams struct definitions, memory
> > > allocation/alignment requirements for the placement of the initrd etc etc.
> > > In the ARM case, it also requires the bootloader to modify the hardware
> > > description provided by the firmware, as it is passed in the same file.
> > > On systems where the initrd is measured after loading, it creates a time
> > > window where the initrd contents might be manipulated in memory before
> > > handing over to the kernel.
> > >
> > > Address these concerns by adding support for loading the initrd into
> > > memory by invoking the EFI LoadFile2 protocol installed on a vendor
> > > GUIDed device path that specifically designates a Linux initrd.
> > > This addresses the above concerns, by putting the EFI stub in charge of
> > > placement in memory and of passing the base and size to the kernel proper
> > > (via whatever means it desires) while still leaving it up to the firmware
> > > or bootloader to obtain the file contents, potentially from other file
> > > systems than the one the kernel itself was loaded from. On platforms that
> > > implement measured boot, it permits the firmware to take the measurement
> > > right before the kernel actually consumes the contents.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > > drivers/firmware/efi/libstub/arm-stub.c | 15 +++-
> > > drivers/firmware/efi/libstub/efi-stub-helper.c | 82 ++++++++++++++++++++
> > > drivers/firmware/efi/libstub/efistub.h | 4 +
> > > drivers/firmware/efi/libstub/x86-stub.c | 23 ++++++
> > > include/linux/efi.h | 1 +
> > > 5 files changed, 122 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> > > index 2edc673ea06c..4bae620b95b9 100644
> > > --- a/drivers/firmware/efi/libstub/arm-stub.c
> > > +++ b/drivers/firmware/efi/libstub/arm-stub.c
> > > @@ -160,6 +160,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
> > > enum efi_secureboot_mode secure_boot;
> > > struct screen_info *si;
> > > efi_properties_table_t *prop_tbl;
> > > + unsigned long max_addr;
> > >
> > > sys_table = sys_table_arg;
> > >
> > > @@ -258,10 +259,18 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
> > > if (!fdt_addr)
> > > pr_efi("Generating empty DTB\n");
> > >
> > > - status = efi_load_initrd(image, &initrd_addr, &initrd_size, ULONG_MAX,
> > > - efi_get_max_initrd_addr(dram_base, *image_addr));
> > > + max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
> > > + status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size, max_addr);
> > > + if (status == EFI_SUCCESS) {
> > > + pr_efi("Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path\n");
> > > + } else if (status == EFI_NOT_FOUND) {
> > > + status = efi_load_initrd(image, &initrd_addr, &initrd_size,
> > > + ULONG_MAX, max_addr);
> > > + if (status == EFI_SUCCESS)
> > > + pr_efi("Loaded initrd from command line option\n");
> > > + }
> > > if (status != EFI_SUCCESS)
> > > - pr_efi_err("Failed initrd from command line!\n");
> > > + pr_efi_err("Failed to load initrd!\n");
> > >
> > > efi_random_get_seed();
> > >
> > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > index 49008ac88b63..e37afe2c752e 100644
> > > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > @@ -299,3 +299,85 @@ void efi_char16_printk(efi_char16_t *str)
> > > efi_call_proto(efi_table_attr(efi_system_table(), con_out),
> > > output_string, str);
> > > }
> > > +
> > > +/*
> > > + * The LINUX_EFI_INITRD_MEDIA_GUID vendor media device path below provides a way
> > > + * for the firmware or bootloader to expose the initrd data directly to the stub
> > > + * via the trivial LoadFile2 protocol, which is defined in the UEFI spec, and is
> > > + * very easy to implement. It is a simple Linux initrd specific conduit between
> > > + * kernel and firmware, allowing us to put the EFI stub (being part of the
> > > + * kernel) in charge of where and when to load the initrd, while leaving it up
> > > + * to the firmware to decide whether it needs to expose its filesystem hierarchy
> > > + * via EFI protocols.
> > > + */
> > > +static const struct {
> > > + struct efi_vendor_dev_path vendor;
> > > + struct efi_generic_dev_path end;
> > > +} __packed initrd_dev_path = {
> > > + {
> > > + EFI_DEV_MEDIA,
> > > + EFI_DEV_MEDIA_VENDOR,
> > > + sizeof(struct efi_vendor_dev_path),
> > > + LINUX_EFI_INITRD_MEDIA_GUID
> > > + }, {
> > > + EFI_DEV_END_PATH,
> > > + EFI_DEV_END_ENTIRE,
> > > + sizeof(struct efi_generic_dev_path)
> > > + }
> > > +};
> > > +
> > > +/**
> > > + * efi_load_initrd_dev_path - load the initrd from the Linux initrd device path
> > > + * @load_addr: pointer to store the address where the initrd was loaded
> > > + * @load_size: pointer to store the size of the loaded initrd
> > > + * @max: upper limit for the initrd memory allocation
> > > + * @return: %EFI_SUCCESS if the initrd was loaded successfully, in which case
> > > + * @load_addr and @load_size are assigned accordingly
> > > + * %EFI_NOT_FOUND if no LoadFile2 protocol exists on the initrd
> > > + * device path
> > > + * %EFI_LOAD_ERROR in all other cases
> >
> > [*]
> >
> > > + */
> > > +efi_status_t efi_load_initrd_dev_path(unsigned long *load_addr,
> > > + unsigned long *load_size,
> > > + unsigned long max)
> > > +{
> > > + efi_guid_t lf2_proto_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
> > > + efi_device_path_protocol_t *dp;
> > > + efi_load_file2_protocol_t *lf2;
> > > + unsigned long initrd_addr;
> > > + unsigned long initrd_size;
> > > + efi_handle_t handle;
> > > + efi_status_t status;
> > > +
> > > + if (!load_addr || !load_size)
> > > + return EFI_INVALID_PARAMETER;
> >
> > Doesn't return EFI_LOAD_ERROR.
> >
> > > +
> > > + dp = (efi_device_path_protocol_t *)&initrd_dev_path;
> > > + status = efi_bs_call(locate_device_path, &lf2_proto_guid, &dp, &handle);
> > > + if (status != EFI_SUCCESS)
> > > + return status;
> >
> > Seems safe (the only plausible error could be EFI_NOT_FOUND).
> >
> > > +
> > > + status = efi_bs_call(handle_protocol, handle, &lf2_proto_guid,
> > > + (void **)&lf2);
> > > + if (status != EFI_SUCCESS)
> > > + return status;
> >
> > Interesting case; this should never fail... but note, if it does, it
> > returns EFI_UNSUPPORTED, not EFI_NOT_FOUND (if the protocol is missing
> > from the handle).
> >
> > > +
> > > + status = efi_call_proto(lf2, load_file, dp, false, &initrd_size, NULL);
> > > + if (status != EFI_BUFFER_TOO_SMALL)
> > > + return EFI_LOAD_ERROR;
> > > +
> > > + status = efi_allocate_pages(initrd_size, &initrd_addr, max);
> > > + if (status != EFI_SUCCESS)
> > > + return status;
> >
> > Not sure about the efi_allocate_pages() wrapper (?); the UEFI service
> > could return EFI_OUT_OF_RESOURCES.
> >
>
> Hmm, guess I was a bit sloppy with the return codes. The important
> thing is that EFI_NOT_FOUND is only returned in the one specifically
> defined case.
>
For the record [in case no respin+resend is needed for other reasons],
I intend to update the comment block as below, and keep the code as
is:
* @load_addr: pointer to store the address where the initrd was loaded
* @load_size: pointer to store the size of the loaded initrd
* @max: upper limit for the initrd memory allocation
- * @return: %EFI_SUCCESS if the initrd was loaded successfully, in
which case
- * @load_addr and @load_size are assigned accordingly
- * %EFI_NOT_FOUND if no LoadFile2 protocol exists on the initrd
- * device path
+ * @return: %EFI_SUCCESS if the initrd was loaded successfully, in which
+ * case @load_addr and @load_size are assigned accordingly
+ * %EFI_NOT_FOUND if no LoadFile2 protocol exists on the initrd
+ * device path
+ * %EFI_INVALID_PARAMETER if load_addr == NULL or load_size == NULL
+ * %EFI_OUT_OF_RESOURCES if memory allocation failed
* %EFI_LOAD_ERROR in all other cases
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] efi/libstub: Add support for loading the initrd from a device path
2020-02-17 10:22 ` Ard Biesheuvel
@ 2020-02-17 10:26 ` Laszlo Ersek
2020-02-17 11:09 ` Ilias Apalodimas
1 sibling, 0 replies; 13+ messages in thread
From: Laszlo Ersek @ 2020-02-17 10:26 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-efi, Linux Kernel Mailing List, Leif Lindholm, Peter Jones,
Matthew Garrett, Alexander Graf, Ilias Apalodimas,
Heinrich Schuchardt, Daniel Kiper, Arvind Sankar,
James Bottomley, Lukas Wunner
On 02/17/20 11:22, Ard Biesheuvel wrote:
> On Mon, 17 Feb 2020 at 10:23, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Mon, 17 Feb 2020 at 10:15, Laszlo Ersek <lersek@redhat.com> wrote:
>>>
>>> On 02/16/20 15:11, Ard Biesheuvel wrote:
>>>> There are currently two ways to specify the initrd to be passed to the
>>>> Linux kernel when booting via the EFI stub:
>>>> - it can be passed as a initrd= command line option when doing a pure PE
>>>> boot (as opposed to the EFI handover protocol that exists for x86)
>>>> - otherwise, the bootloader or firmware can load the initrd into memory,
>>>> and pass the address and size via the bootparams struct (x86) or
>>>> device tree (ARM)
>>>>
>>>> In the first case, we are limited to loading from the same file system
>>>> that the kernel was loaded from, and it is also problematic in a trusted
>>>> boot context, given that we cannot easily protect the command line from
>>>> tampering without either adding complicated white/blacklisting of boot
>>>> arguments or locking down the command line altogether.
>>>>
>>>> In the second case, we force the bootloader to duplicate knowledge about
>>>> the boot protocol which is already encoded in the stub, and which may be
>>>> subject to change over time, e.g., bootparams struct definitions, memory
>>>> allocation/alignment requirements for the placement of the initrd etc etc.
>>>> In the ARM case, it also requires the bootloader to modify the hardware
>>>> description provided by the firmware, as it is passed in the same file.
>>>> On systems where the initrd is measured after loading, it creates a time
>>>> window where the initrd contents might be manipulated in memory before
>>>> handing over to the kernel.
>>>>
>>>> Address these concerns by adding support for loading the initrd into
>>>> memory by invoking the EFI LoadFile2 protocol installed on a vendor
>>>> GUIDed device path that specifically designates a Linux initrd.
>>>> This addresses the above concerns, by putting the EFI stub in charge of
>>>> placement in memory and of passing the base and size to the kernel proper
>>>> (via whatever means it desires) while still leaving it up to the firmware
>>>> or bootloader to obtain the file contents, potentially from other file
>>>> systems than the one the kernel itself was loaded from. On platforms that
>>>> implement measured boot, it permits the firmware to take the measurement
>>>> right before the kernel actually consumes the contents.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>> ---
>>>> drivers/firmware/efi/libstub/arm-stub.c | 15 +++-
>>>> drivers/firmware/efi/libstub/efi-stub-helper.c | 82 ++++++++++++++++++++
>>>> drivers/firmware/efi/libstub/efistub.h | 4 +
>>>> drivers/firmware/efi/libstub/x86-stub.c | 23 ++++++
>>>> include/linux/efi.h | 1 +
>>>> 5 files changed, 122 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>>>> index 2edc673ea06c..4bae620b95b9 100644
>>>> --- a/drivers/firmware/efi/libstub/arm-stub.c
>>>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>>>> @@ -160,6 +160,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
>>>> enum efi_secureboot_mode secure_boot;
>>>> struct screen_info *si;
>>>> efi_properties_table_t *prop_tbl;
>>>> + unsigned long max_addr;
>>>>
>>>> sys_table = sys_table_arg;
>>>>
>>>> @@ -258,10 +259,18 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table_arg,
>>>> if (!fdt_addr)
>>>> pr_efi("Generating empty DTB\n");
>>>>
>>>> - status = efi_load_initrd(image, &initrd_addr, &initrd_size, ULONG_MAX,
>>>> - efi_get_max_initrd_addr(dram_base, *image_addr));
>>>> + max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
>>>> + status = efi_load_initrd_dev_path(&initrd_addr, &initrd_size, max_addr);
>>>> + if (status == EFI_SUCCESS) {
>>>> + pr_efi("Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path\n");
>>>> + } else if (status == EFI_NOT_FOUND) {
>>>> + status = efi_load_initrd(image, &initrd_addr, &initrd_size,
>>>> + ULONG_MAX, max_addr);
>>>> + if (status == EFI_SUCCESS)
>>>> + pr_efi("Loaded initrd from command line option\n");
>>>> + }
>>>> if (status != EFI_SUCCESS)
>>>> - pr_efi_err("Failed initrd from command line!\n");
>>>> + pr_efi_err("Failed to load initrd!\n");
>>>>
>>>> efi_random_get_seed();
>>>>
>>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>>> index 49008ac88b63..e37afe2c752e 100644
>>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>>> @@ -299,3 +299,85 @@ void efi_char16_printk(efi_char16_t *str)
>>>> efi_call_proto(efi_table_attr(efi_system_table(), con_out),
>>>> output_string, str);
>>>> }
>>>> +
>>>> +/*
>>>> + * The LINUX_EFI_INITRD_MEDIA_GUID vendor media device path below provides a way
>>>> + * for the firmware or bootloader to expose the initrd data directly to the stub
>>>> + * via the trivial LoadFile2 protocol, which is defined in the UEFI spec, and is
>>>> + * very easy to implement. It is a simple Linux initrd specific conduit between
>>>> + * kernel and firmware, allowing us to put the EFI stub (being part of the
>>>> + * kernel) in charge of where and when to load the initrd, while leaving it up
>>>> + * to the firmware to decide whether it needs to expose its filesystem hierarchy
>>>> + * via EFI protocols.
>>>> + */
>>>> +static const struct {
>>>> + struct efi_vendor_dev_path vendor;
>>>> + struct efi_generic_dev_path end;
>>>> +} __packed initrd_dev_path = {
>>>> + {
>>>> + EFI_DEV_MEDIA,
>>>> + EFI_DEV_MEDIA_VENDOR,
>>>> + sizeof(struct efi_vendor_dev_path),
>>>> + LINUX_EFI_INITRD_MEDIA_GUID
>>>> + }, {
>>>> + EFI_DEV_END_PATH,
>>>> + EFI_DEV_END_ENTIRE,
>>>> + sizeof(struct efi_generic_dev_path)
>>>> + }
>>>> +};
>>>> +
>>>> +/**
>>>> + * efi_load_initrd_dev_path - load the initrd from the Linux initrd device path
>>>> + * @load_addr: pointer to store the address where the initrd was loaded
>>>> + * @load_size: pointer to store the size of the loaded initrd
>>>> + * @max: upper limit for the initrd memory allocation
>>>> + * @return: %EFI_SUCCESS if the initrd was loaded successfully, in which case
>>>> + * @load_addr and @load_size are assigned accordingly
>>>> + * %EFI_NOT_FOUND if no LoadFile2 protocol exists on the initrd
>>>> + * device path
>>>> + * %EFI_LOAD_ERROR in all other cases
>>>
>>> [*]
>>>
>>>> + */
>>>> +efi_status_t efi_load_initrd_dev_path(unsigned long *load_addr,
>>>> + unsigned long *load_size,
>>>> + unsigned long max)
>>>> +{
>>>> + efi_guid_t lf2_proto_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
>>>> + efi_device_path_protocol_t *dp;
>>>> + efi_load_file2_protocol_t *lf2;
>>>> + unsigned long initrd_addr;
>>>> + unsigned long initrd_size;
>>>> + efi_handle_t handle;
>>>> + efi_status_t status;
>>>> +
>>>> + if (!load_addr || !load_size)
>>>> + return EFI_INVALID_PARAMETER;
>>>
>>> Doesn't return EFI_LOAD_ERROR.
>>>
>>>> +
>>>> + dp = (efi_device_path_protocol_t *)&initrd_dev_path;
>>>> + status = efi_bs_call(locate_device_path, &lf2_proto_guid, &dp, &handle);
>>>> + if (status != EFI_SUCCESS)
>>>> + return status;
>>>
>>> Seems safe (the only plausible error could be EFI_NOT_FOUND).
>>>
>>>> +
>>>> + status = efi_bs_call(handle_protocol, handle, &lf2_proto_guid,
>>>> + (void **)&lf2);
>>>> + if (status != EFI_SUCCESS)
>>>> + return status;
>>>
>>> Interesting case; this should never fail... but note, if it does, it
>>> returns EFI_UNSUPPORTED, not EFI_NOT_FOUND (if the protocol is missing
>>> from the handle).
>>>
>>>> +
>>>> + status = efi_call_proto(lf2, load_file, dp, false, &initrd_size, NULL);
>>>> + if (status != EFI_BUFFER_TOO_SMALL)
>>>> + return EFI_LOAD_ERROR;
>>>> +
>>>> + status = efi_allocate_pages(initrd_size, &initrd_addr, max);
>>>> + if (status != EFI_SUCCESS)
>>>> + return status;
>>>
>>> Not sure about the efi_allocate_pages() wrapper (?); the UEFI service
>>> could return EFI_OUT_OF_RESOURCES.
>>>
>>
>> Hmm, guess I was a bit sloppy with the return codes. The important
>> thing is that EFI_NOT_FOUND is only returned in the one specifically
>> defined case.
>>
>
> For the record [in case no respin+resend is needed for other reasons],
> I intend to update the comment block as below, and keep the code as
> is:
>
>
> * @load_addr: pointer to store the address where the initrd was loaded
> * @load_size: pointer to store the size of the loaded initrd
> * @max: upper limit for the initrd memory allocation
> - * @return: %EFI_SUCCESS if the initrd was loaded successfully, in
> which case
> - * @load_addr and @load_size are assigned accordingly
> - * %EFI_NOT_FOUND if no LoadFile2 protocol exists on the initrd
> - * device path
> + * @return: %EFI_SUCCESS if the initrd was loaded successfully, in which
> + * case @load_addr and @load_size are assigned accordingly
> + * %EFI_NOT_FOUND if no LoadFile2 protocol exists on the initrd
> + * device path
> + * %EFI_INVALID_PARAMETER if load_addr == NULL or load_size == NULL
> + * %EFI_OUT_OF_RESOURCES if memory allocation failed
> * %EFI_LOAD_ERROR in all other cases
>
Right, I agree that updating the comments suffices for consistency; the
code is not wrong, only the comments and the code don't match.
So with that
Acked-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] efi/libstub: Add support for loading the initrd from a device path
2020-02-17 10:22 ` Ard Biesheuvel
2020-02-17 10:26 ` Laszlo Ersek
@ 2020-02-17 11:09 ` Ilias Apalodimas
1 sibling, 0 replies; 13+ messages in thread
From: Ilias Apalodimas @ 2020-02-17 11:09 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Laszlo Ersek, linux-efi, Linux Kernel Mailing List,
Leif Lindholm, Peter Jones, Matthew Garrett, Alexander Graf,
Heinrich Schuchardt, Daniel Kiper, Arvind Sankar,
James Bottomley, Lukas Wunner
Hi Ard,
[...]
> > > > + return EFI_INVALID_PARAMETER;
> > >
> > > Doesn't return EFI_LOAD_ERROR.
> > >
> > > > +
> > > > + dp = (efi_device_path_protocol_t *)&initrd_dev_path;
> > > > + status = efi_bs_call(locate_device_path, &lf2_proto_guid, &dp, &handle);
> > > > + if (status != EFI_SUCCESS)
> > > > + return status;
> > >
> > > Seems safe (the only plausible error could be EFI_NOT_FOUND).
> > >
> > > > +
> > > > + status = efi_bs_call(handle_protocol, handle, &lf2_proto_guid,
> > > > + (void **)&lf2);
> > > > + if (status != EFI_SUCCESS)
> > > > + return status;
> > >
> > > Interesting case; this should never fail... but note, if it does, it
> > > returns EFI_UNSUPPORTED, not EFI_NOT_FOUND (if the protocol is missing
> > > from the handle).
> > >
> > > > +
> > > > + status = efi_call_proto(lf2, load_file, dp, false, &initrd_size, NULL);
> > > > + if (status != EFI_BUFFER_TOO_SMALL)
> > > > + return EFI_LOAD_ERROR;
> > > > +
> > > > + status = efi_allocate_pages(initrd_size, &initrd_addr, max);
> > > > + if (status != EFI_SUCCESS)
> > > > + return status;
> > >
> > > Not sure about the efi_allocate_pages() wrapper (?); the UEFI service
> > > could return EFI_OUT_OF_RESOURCES.
> > >
> >
> > Hmm, guess I was a bit sloppy with the return codes. The important
> > thing is that EFI_NOT_FOUND is only returned in the one specifically
> > defined case.
> >
>
> For the record [in case no respin+resend is needed for other reasons],
> I intend to update the comment block as below, and keep the code as
> is:
>
Yes i think this makes more sense the return codes are already correct and the
fallback is properly triggered.
For what it's worth
Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>
> * @load_addr: pointer to store the address where the initrd was loaded
> * @load_size: pointer to store the size of the loaded initrd
> * @max: upper limit for the initrd memory allocation
> - * @return: %EFI_SUCCESS if the initrd was loaded successfully, in
> which case
> - * @load_addr and @load_size are assigned accordingly
> - * %EFI_NOT_FOUND if no LoadFile2 protocol exists on the initrd
> - * device path
> + * @return: %EFI_SUCCESS if the initrd was loaded successfully, in which
> + * case @load_addr and @load_size are assigned accordingly
> + * %EFI_NOT_FOUND if no LoadFile2 protocol exists on the initrd
> + * device path
> + * %EFI_INVALID_PARAMETER if load_addr == NULL or load_size == NULL
> + * %EFI_OUT_OF_RESOURCES if memory allocation failed
> * %EFI_LOAD_ERROR in all other cases
Regards
/Ilias
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/3] arch-agnostic initrd loading method for EFI systems
2020-02-16 14:11 [PATCH v2 0/3] arch-agnostic initrd loading method for EFI systems Ard Biesheuvel
` (2 preceding siblings ...)
2020-02-16 14:11 ` [PATCH v2 3/3] efi/libstub: Take noinitrd cmdline argument into account for devpath initrd Ard Biesheuvel
@ 2020-02-17 19:10 ` Bhupesh Sharma
2020-02-17 19:23 ` Ard Biesheuvel
3 siblings, 1 reply; 13+ messages in thread
From: Bhupesh Sharma @ 2020-02-17 19:10 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-efi, Linux Kernel Mailing List, Laszlo Ersek, leif,
Peter Jones, mjg59, agraf, ilias.apalodimas, xypron.glpk,
daniel.kiper, nivedita, James.Bottomley, lukas
Hi Ard,
On Sun, Feb 16, 2020 at 7:41 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> This series introduces an arch agnostic way of loading the initrd into
> memory from the EFI stub. This addresses a number of shortcomings that
> affect the current implementations that exist across architectures:
>
> - The initrd=<file> command line option can only load files that reside
> on the same file system that the kernel itself was loaded from, which
> requires the bootloader or firmware to expose that file system via the
> appropriate EFI protocol, which is not always feasible. From the kernel
> side, this protocol is problematic since it is incompatible with mixed
> mode on x86 (this is due to the fact that some of its methods have
> prototypes that are difficult to marshall)
>
> - The approach that is ordinarily taken by GRUB is to load the initrd into
> memory, and pass it to the kernel proper via the bootparams structure or
> via the device tree. This requires the boot loader to have an understanding
> of those structures, which are not always set in stone, and of the policies
> around where the initrd may be loaded into memory. In the ARM case, it
> requires GRUB to modify the hardware description provided by the firmware,
> given that the initrd base and offset in memory are passed via the same
> data structure. It also creates a time window where the initrd data sits
> in memory, and can potentially be corrupted before the kernel is booted.
>
> Considering that we will soon have new users of these interfaces (EFI for
> kvmtool on ARM, RISC-V in u-boot, etc), it makes sense to add a generic
> interface now, before having another wave of bespoke arch specific code
> coming in.
>
> Another aspect to take into account is that support for UEFI secure boot
> and measured boot is being taken into the upstream, and being able to
> rely on the PE entry point for booting any architecture makes the GRUB
> vs shim story much cleaner, as we should be able to rely on LoadImage
> and StartImage on all architectures, while retaining the ability to
> load initrds from anywhere.
>
> Note that these patches depend on a fair amount of cleanup work that I
> am targetting for v5.7. Branch can be found at:
> https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/log/?h=next
>
> A PoC implementation for OVMF and ArmVirtQemu (OVMF for ARM aka AAVMF) can
> be found at https://github.com/ardbiesheuvel/edk2/commits/linux-efi-generic.
>
> A U-Boot implementation is under development as well, and can be found at
> https://github.com/apalos/u-boot/commits/efi_load_file_8
Thanks a lot for the patchset. I am still going through the patchset
and trying to understand how will it impact
kexec use-cases, namely:
1. kexec_load() and kecec_file_load(), use '--initrd=<file>' like
command line options.
2. We have several kexec based bootloader implementations (for example
linuxboot, see (a) and (b) below) that replaces specific firmware
functionality like the UEFI DXE phase with a Linux kernel and runtime
and find the initrd image (for example, uroot) from that same
filesystem. So these would need modification(s) similar to the OVMF
AAVMF and u-boot, right?
a. https://www.linuxboot.org/
b. https://github.com/linuxboot/linuxboot/blob/master/dxe/linuxboot.c
Thanks,
Bhupesh
> Changes since v1:
> - merge vendor media device path type definition with the existing device path
> definitions we already have, and rework the latter slightly to be more easily
> reusable
> - use 'dev_path' not 'devpath' consistently
> - pass correct FilePath value to LoadFile2 (i.e., the device path pointer that
> was advanced to point to the 'end' node by locate_device_path())
> - add kerneldoc comment to efi_load_initrd_dev_path()
> - take care to only return EFI_NOT_FOUND from efi_load_initrd_dev_path() if the
> LoadFile2 protocol does not exist on the LINUX_EFI_INITRD_MEDIA_GUID device
> path - this makes the logic whether to fallback to the command line method
> more robust
>
> Cc: lersek@redhat.com
> Cc: leif@nuviainc.com
> Cc: pjones@redhat.com
> Cc: mjg59@google.com
> Cc: agraf@csgraf.de
> Cc: ilias.apalodimas@linaro.org
> Cc: xypron.glpk@gmx.de
> Cc: daniel.kiper@oracle.com
> Cc: nivedita@alum.mit.edu
> Cc: James.Bottomley@hansenpartnership.com
> Cc: lukas@wunner.de
>
> Ard Biesheuvel (3):
> efi/dev-path-parser: Add struct definition for vendor type device path
> nodes
> efi/libstub: Add support for loading the initrd from a device path
> efi/libstub: Take noinitrd cmdline argument into account for devpath
> initrd
>
> drivers/firmware/efi/apple-properties.c | 8 +-
> drivers/firmware/efi/dev-path-parser.c | 38 ++++----
> drivers/firmware/efi/libstub/arm-stub.c | 20 ++++-
> .../firmware/efi/libstub/efi-stub-helper.c | 89 +++++++++++++++++++
> drivers/firmware/efi/libstub/efistub.h | 5 ++
> drivers/firmware/efi/libstub/x86-stub.c | 47 ++++++++--
> include/linux/efi.h | 49 ++++++----
> 7 files changed, 201 insertions(+), 55 deletions(-)
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/3] arch-agnostic initrd loading method for EFI systems
2020-02-17 19:10 ` [PATCH v2 0/3] arch-agnostic initrd loading method for EFI systems Bhupesh Sharma
@ 2020-02-17 19:23 ` Ard Biesheuvel
2020-02-17 20:07 ` Bhupesh Sharma
0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2020-02-17 19:23 UTC (permalink / raw)
To: Bhupesh Sharma
Cc: linux-efi, Linux Kernel Mailing List, Laszlo Ersek,
Leif Lindholm, Peter Jones, Matthew Garrett, Alexander Graf,
Ilias Apalodimas, Heinrich Schuchardt, Daniel Kiper,
Arvind Sankar, James Bottomley, Lukas Wunner
On Mon, 17 Feb 2020 at 20:11, Bhupesh Sharma <bhsharma@redhat.com> wrote:
>
> Hi Ard,
>
> On Sun, Feb 16, 2020 at 7:41 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > This series introduces an arch agnostic way of loading the initrd into
> > memory from the EFI stub. This addresses a number of shortcomings that
> > affect the current implementations that exist across architectures:
> >
> > - The initrd=<file> command line option can only load files that reside
> > on the same file system that the kernel itself was loaded from, which
> > requires the bootloader or firmware to expose that file system via the
> > appropriate EFI protocol, which is not always feasible. From the kernel
> > side, this protocol is problematic since it is incompatible with mixed
> > mode on x86 (this is due to the fact that some of its methods have
> > prototypes that are difficult to marshall)
> >
> > - The approach that is ordinarily taken by GRUB is to load the initrd into
> > memory, and pass it to the kernel proper via the bootparams structure or
> > via the device tree. This requires the boot loader to have an understanding
> > of those structures, which are not always set in stone, and of the policies
> > around where the initrd may be loaded into memory. In the ARM case, it
> > requires GRUB to modify the hardware description provided by the firmware,
> > given that the initrd base and offset in memory are passed via the same
> > data structure. It also creates a time window where the initrd data sits
> > in memory, and can potentially be corrupted before the kernel is booted.
> >
> > Considering that we will soon have new users of these interfaces (EFI for
> > kvmtool on ARM, RISC-V in u-boot, etc), it makes sense to add a generic
> > interface now, before having another wave of bespoke arch specific code
> > coming in.
> >
> > Another aspect to take into account is that support for UEFI secure boot
> > and measured boot is being taken into the upstream, and being able to
> > rely on the PE entry point for booting any architecture makes the GRUB
> > vs shim story much cleaner, as we should be able to rely on LoadImage
> > and StartImage on all architectures, while retaining the ability to
> > load initrds from anywhere.
> >
> > Note that these patches depend on a fair amount of cleanup work that I
> > am targetting for v5.7. Branch can be found at:
> > https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/log/?h=next
> >
> > A PoC implementation for OVMF and ArmVirtQemu (OVMF for ARM aka AAVMF) can
> > be found at https://github.com/ardbiesheuvel/edk2/commits/linux-efi-generic.
> >
> > A U-Boot implementation is under development as well, and can be found at
> > https://github.com/apalos/u-boot/commits/efi_load_file_8
>
Hello Bhupesh,
> Thanks a lot for the patchset. I am still going through the patchset
> and trying to understand how will it impact
> kexec use-cases, namely:
>
> 1. kexec_load() and kecec_file_load(), use '--initrd=<file>' like
> command line options.
>
These should not be affected at all, since they don't go through the EFI stub.
> 2. We have several kexec based bootloader implementations (for example
> linuxboot, see (a) and (b) below) that replaces specific firmware
> functionality like the UEFI DXE phase with a Linux kernel and runtime
> and find the initrd image (for example, uroot) from that same
> filesystem. So these would need modification(s) similar to the OVMF
> AAVMF and u-boot, right?
>
These changes only affect the interaction between the EFI firmware and
the EFI stub. Anything that relies on the raw x86 boot protocol (jump
to offset 0x0 for 32-bit or offset 0x200 for 64-bit) or the EFI
handover protocol (find the boot offset in the setup header and jump
into the image at the discovered offset) will work as before, although
the EFI handover protocol is something I would like to get rid of at
some point in the future. Kexec typically mimics the handover between
the EFI stub and the kernel, not the EFI firmware and the EFI stub, so
I would not expect kexec to be affected at all by any of this.
In general, the intent is to make kexec idempotent, i.e., to make the
first boot as similar as we can to subsequent kexec boots, which is
why we are pushing back against adding bespoke interfaces to pass ACPI
rsdp pointer, smbios table addresses etc.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/3] arch-agnostic initrd loading method for EFI systems
2020-02-17 19:23 ` Ard Biesheuvel
@ 2020-02-17 20:07 ` Bhupesh Sharma
0 siblings, 0 replies; 13+ messages in thread
From: Bhupesh Sharma @ 2020-02-17 20:07 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-efi, Linux Kernel Mailing List, Laszlo Ersek,
Leif Lindholm, Peter Jones, Matthew Garrett, Alexander Graf,
Ilias Apalodimas, Heinrich Schuchardt, Daniel Kiper,
Arvind Sankar, James Bottomley, Lukas Wunner
On Tue, Feb 18, 2020 at 12:53 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 17 Feb 2020 at 20:11, Bhupesh Sharma <bhsharma@redhat.com> wrote:
> >
> > Hi Ard,
> >
> > On Sun, Feb 16, 2020 at 7:41 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > This series introduces an arch agnostic way of loading the initrd into
> > > memory from the EFI stub. This addresses a number of shortcomings that
> > > affect the current implementations that exist across architectures:
> > >
> > > - The initrd=<file> command line option can only load files that reside
> > > on the same file system that the kernel itself was loaded from, which
> > > requires the bootloader or firmware to expose that file system via the
> > > appropriate EFI protocol, which is not always feasible. From the kernel
> > > side, this protocol is problematic since it is incompatible with mixed
> > > mode on x86 (this is due to the fact that some of its methods have
> > > prototypes that are difficult to marshall)
> > >
> > > - The approach that is ordinarily taken by GRUB is to load the initrd into
> > > memory, and pass it to the kernel proper via the bootparams structure or
> > > via the device tree. This requires the boot loader to have an understanding
> > > of those structures, which are not always set in stone, and of the policies
> > > around where the initrd may be loaded into memory. In the ARM case, it
> > > requires GRUB to modify the hardware description provided by the firmware,
> > > given that the initrd base and offset in memory are passed via the same
> > > data structure. It also creates a time window where the initrd data sits
> > > in memory, and can potentially be corrupted before the kernel is booted.
> > >
> > > Considering that we will soon have new users of these interfaces (EFI for
> > > kvmtool on ARM, RISC-V in u-boot, etc), it makes sense to add a generic
> > > interface now, before having another wave of bespoke arch specific code
> > > coming in.
> > >
> > > Another aspect to take into account is that support for UEFI secure boot
> > > and measured boot is being taken into the upstream, and being able to
> > > rely on the PE entry point for booting any architecture makes the GRUB
> > > vs shim story much cleaner, as we should be able to rely on LoadImage
> > > and StartImage on all architectures, while retaining the ability to
> > > load initrds from anywhere.
> > >
> > > Note that these patches depend on a fair amount of cleanup work that I
> > > am targetting for v5.7. Branch can be found at:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/log/?h=next
> > >
> > > A PoC implementation for OVMF and ArmVirtQemu (OVMF for ARM aka AAVMF) can
> > > be found at https://github.com/ardbiesheuvel/edk2/commits/linux-efi-generic.
> > >
> > > A U-Boot implementation is under development as well, and can be found at
> > > https://github.com/apalos/u-boot/commits/efi_load_file_8
> >
>
> Hello Bhupesh,
>
> > Thanks a lot for the patchset. I am still going through the patchset
> > and trying to understand how will it impact
> > kexec use-cases, namely:
> >
> > 1. kexec_load() and kecec_file_load(), use '--initrd=<file>' like
> > command line options.
> >
>
> These should not be affected at all, since they don't go through the EFI stub.
>
> > 2. We have several kexec based bootloader implementations (for example
> > linuxboot, see (a) and (b) below) that replaces specific firmware
> > functionality like the UEFI DXE phase with a Linux kernel and runtime
> > and find the initrd image (for example, uroot) from that same
> > filesystem. So these would need modification(s) similar to the OVMF
> > AAVMF and u-boot, right?
> >
>
> These changes only affect the interaction between the EFI firmware and
> the EFI stub. Anything that relies on the raw x86 boot protocol (jump
> to offset 0x0 for 32-bit or offset 0x200 for 64-bit) or the EFI
> handover protocol (find the boot offset in the setup header and jump
> into the image at the discovered offset) will work as before, although
> the EFI handover protocol is something I would like to get rid of at
> some point in the future. Kexec typically mimics the handover between
> the EFI stub and the kernel, not the EFI firmware and the EFI stub, so
> I would not expect kexec to be affected at all by any of this.
Thanks a lot for the clarification, Ard. You are right, currently
there are several kexec based bootloader implementations which follow
a non-standard (?) way of handing over the control from EFI firmware
to the Linux kernel.
While we cannot be sure about all, but in future we can try and move
some of the popular ones to more standard ways of using Linux kenrel +
kexec as a partial firmware replacement.
> In general, the intent is to make kexec idempotent, i.e., to make the
> first boot as similar as we can to subsequent kexec boots, which is
> why we are pushing back against adding bespoke interfaces to pass ACPI
> rsdp pointer, smbios table addresses etc.
Got it. Thanks a lot for your clarification.
Regards,
Bhupesh
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-02-17 20:08 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-16 14:11 [PATCH v2 0/3] arch-agnostic initrd loading method for EFI systems Ard Biesheuvel
2020-02-16 14:11 ` [PATCH v2 1/3] efi/dev-path-parser: Add struct definition for vendor type device path nodes Ard Biesheuvel
2020-02-16 14:11 ` [PATCH v2 2/3] efi/libstub: Add support for loading the initrd from a device path Ard Biesheuvel
2020-02-17 9:15 ` Laszlo Ersek
2020-02-17 9:23 ` Ard Biesheuvel
2020-02-17 10:01 ` Laszlo Ersek
2020-02-17 10:22 ` Ard Biesheuvel
2020-02-17 10:26 ` Laszlo Ersek
2020-02-17 11:09 ` Ilias Apalodimas
2020-02-16 14:11 ` [PATCH v2 3/3] efi/libstub: Take noinitrd cmdline argument into account for devpath initrd Ard Biesheuvel
2020-02-17 19:10 ` [PATCH v2 0/3] arch-agnostic initrd loading method for EFI systems Bhupesh Sharma
2020-02-17 19:23 ` Ard Biesheuvel
2020-02-17 20:07 ` Bhupesh Sharma
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).