[1/2] efi/libstub: add support for loading the initrd from a device path

Commit Message

Ard Biesheuvel Feb. 6, 2020, 2:03 p.m. UTC

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 <[e-mail]>
---
 drivers/firmware/efi/libstub/arm-stub.c        | 16 +++--
 drivers/firmware/efi/libstub/efi-stub-helper.c | 65 ++++++++++++++++++++
 drivers/firmware/efi/libstub/efistub.h         | 12 ++++
 drivers/firmware/efi/libstub/x86-stub.c        | 36 ++++++++++-
 include/linux/efi.h                            |  1 +
 5 files changed, 123 insertions(+), 7 deletions(-)

Comments

Heinrich Schuchardt Feb. 6, 2020, 6:26 p.m. UTC | #1

On 2/6/20 3:03 PM, 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

Hello Ard,

"same file system" is not a limitation of using a command line
parameter. Any device path can be passed as a string.

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

Not relying on the command line for finding the initrd image does not
secure the integrity and the validity of the initrd image.

A signature on the initrd image could solve the security problem you
describe. It would not require non-Linux software to be changed, and
would provide much better security.

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

A firmware implementing the UEFI standard will not be aware of any
initrd image as such an object does not exist in the standard. It was a
wise decision that the UEFI standard is operating system agnostic
(accomodating BSD, Linux, Windows, etc.). So the firmware (EDK2, U-Boot,
etc.) seems to be out of scope for providing a Linux specific
EFI_LOAD_FILE2_PROTOCOL.

When booting via GRUB it will be GRUB knowing which initrd to load.

Please, indicate which software you expect to expose the initrd related
EFI_LOAD_FILE2_PROTOCOL.

Using an UEFI variable for passing the initrd device path would be a
leaner solution on the bootloader side than requiring an extra
EFI_LOAD_FILE2_PROTOCOL implementation.

>
> Signed-off-by: Ard Biesheuvel <[e-mail]>
> ---
>   drivers/firmware/efi/libstub/arm-stub.c        | 16 +++--
>   drivers/firmware/efi/libstub/efi-stub-helper.c | 65 ++++++++++++++++++++
>   drivers/firmware/efi/libstub/efistub.h         | 12 ++++
>   drivers/firmware/efi/libstub/x86-stub.c        | 36 ++++++++++-
>   include/linux/efi.h                            |  1 +
>   5 files changed, 123 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index c7b091f50e55..1db943c1ba2b 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -157,6 +157,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;
>
> @@ -255,11 +256,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, ULONG_MAX,
> -				 efi_get_max_initrd_addr(dram_base, *image_addr),
> -				 &initrd_addr, &initrd_size);
> +	max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
> +	status = efi_load_initrd_devpath(&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, ULONG_MAX, max_addr,
> +					 &initrd_addr, &initrd_size);
> +		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 8e60a39df3c5..eaf45ea749b3 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -323,3 +323,68 @@ void efi_char16_printk(efi_char16_t *str)
>   	efi_call_proto(efi_table_attr(efi_system_table(), con_out),
>   		       output_string, str);
>   }
> +
> +static const struct {
> +	struct efi_vendor_dev_path	vendor;
> +	struct efi_generic_dev_path	end;
> +} __packed initrd_devpath = {
> +	{
> +		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_status_t efi_load_initrd_devpath(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_devpath;
> +	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;

You require here that there is a handle exposing the device path
protocol with the initrd specific device path. On the same handle the
EFI_LOAD_FILE2_PROTOCOL must be installed which will load the initrd
file when called with the same device path.

An alternative implementation would simple loop over all instances of
the EFI_LOAD_FILE2_PROTOCOL and try to load the initrd.

It would be worthwhile to describe the requirements on the
implementation of the EFI_LOAD_FILE2_PROTOCOL in a code comment and in
the documentation.

Unfortunately the documentation of UEFI has been duplicated into two files:

Documentation/arm/uefi.rst
Documentation/x86/x86_64/uefi.rst

Best regards

Heinrich

> +
> +	initrd_size = 0;
> +	status = efi_call_proto(lf2, load_file,
> +				(efi_device_path_protocol_t *)&initrd_devpath,
> +				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,
> +				(efi_device_path_protocol_t *)&initrd_devpath,
> +				false, &initrd_size, (void *)initrd_addr);
> +	if (status != EFI_SUCCESS) {
> +		efi_free(initrd_size, initrd_addr);
> +		return status;
> +	}
> +
> +	*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 99e93fd76ec5..fbf9f9442eed 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -566,6 +566,14 @@ union efi_load_file_protocol {
>   	} mixed_mode;
>   };
>
> +struct efi_vendor_dev_path {
> +	u8		type;
> +	u8		sub_type;
> +	u16		length;
> +	efi_guid_t	vendorguid;
> +	u8		vendordata[];
> +} __packed;
> +
>   void efi_pci_disable_bridge_busmaster(void);
>
>   typedef efi_status_t (*efi_exit_boot_map_processing)(
> @@ -651,4 +659,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
>   			     unsigned long *load_addr,
>   			     unsigned long *load_size);
>
> +efi_status_t efi_load_initrd_devpath(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 f3e2ff31b624..7f38f95676dd 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -419,9 +419,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>   	if (status != EFI_SUCCESS)
>   		goto fail2;
>
> -	status = efi_load_initrd(image, hdr->initrd_addr_max,
> -				 above4g ? ULONG_MAX : hdr->initrd_addr_max,
> -				 &ramdisk_addr, &ramdisk_size);
> +	/*
> +	 * The initrd loaded from the Linux initrd vendor device
> +	 * path should take precedence, as we don't want the
> +	 * [unverified] command line to override the initrd
> +	 * supplied by the [potentially verified] firmware.
> +	 */
> +	status = efi_load_initrd_devpath(&ramdisk_addr, &ramdisk_size,
> +					 above4g ? ULONG_MAX
> +						 : hdr->initrd_addr_max);
> +	if (status == EFI_NOT_FOUND)
> +		status = efi_load_initrd(image, hdr->initrd_addr_max,
> +					 above4g ? ULONG_MAX
> +						 : hdr->initrd_addr_max,
> +					 &ramdisk_addr, &ramdisk_size);
>   	if (status != EFI_SUCCESS)
>   		goto fail2;
>   	hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
> @@ -732,6 +743,25 @@ struct boot_params *efi_main(efi_handle_t handle,
>   			 ((u64)boot_params->ext_cmd_line_ptr << 32));
>   	efi_parse_options((char *)cmdline_paddr);
>
> +	if (!hdr->ramdisk_size && !boot_params->ext_ramdisk_size) {
> +		unsigned long max = hdr->initrd_addr_max;
> +		unsigned long addr, size;
> +
> +		if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
> +			max = ULONG_MAX;
> +
> +		status = efi_load_initrd_devpath(&addr, &size, max);
> +		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_devpath() 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 9ccf313fe9de..75c83c322c40 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)
>

Ilias Apalodimas Feb. 6, 2020, 6:46 p.m. UTC | #2

Hi Heinrich,

I actually like the approach.

On Thu, Feb 06, 2020 at 07:26:20PM +0100, Heinrich Schuchardt wrote:
[...] 
> > 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.
> 
> Not relying on the command line for finding the initrd image does not
> secure the integrity and the validity of the initrd image.

This patch isn't supposed to protect you against a bogus initrd and I don't
think it should be the bootloaders job to verify that. The kernel already has
tools to do that. 

> 
> A signature on the initrd image could solve the security problem you
> describe. It would not require non-Linux software to be changed, and
> would provide much better security.
> 

The problem with parsing the kernel command line is that at the moment,
different options are supported by different architectures. A quick grep for 
CONFIG_CMDLINE_(FORCE/OVERWRITE/EXTEND/FROM_BOOTLOADER) will give you an idea.

What you can do in U-Boot currently is not have an environment in any of the
flashes and set the bootdelay to 0, in order to prevent the user from 
changing that command line.

> > 
> > 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.
> 
> A firmware implementing the UEFI standard will not be aware of any
> initrd image as such an object does not exist in the standard. It was a
> wise decision that the UEFI standard is operating system agnostic
> (accomodating BSD, Linux, Windows, etc.). So the firmware (EDK2, U-Boot,
> etc.) seems to be out of scope for providing a Linux specific
> EFI_LOAD_FILE2_PROTOCOL.
> 
> When booting via GRUB it will be GRUB knowing which initrd to load.

What about booting the kernel directly?

> 
> Please, indicate which software you expect to expose the initrd related
> EFI_LOAD_FILE2_PROTOCOL.

I have an implementation for this on U-Boot which works. The file and device are
hardcoded at the moment, but the rest of the functionality works fine. I'll
share it with you once I clean it up a bit. 

> 
> Using an UEFI variable for passing the initrd device path would be a
> leaner solution on the bootloader side than requiring an extra
> EFI_LOAD_FILE2_PROTOCOL implementation.
> 

Thanks
/Ilias

> > 
> > Signed-off-by: Ard Biesheuvel <[e-mail]>
> > ---
> >   drivers/firmware/efi/libstub/arm-stub.c        | 16 +++--
> >   drivers/firmware/efi/libstub/efi-stub-helper.c | 65 ++++++++++++++++++++
> >   drivers/firmware/efi/libstub/efistub.h         | 12 ++++
> >   drivers/firmware/efi/libstub/x86-stub.c        | 36 ++++++++++-
> >   include/linux/efi.h                            |  1 +
> >   5 files changed, 123 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> > index c7b091f50e55..1db943c1ba2b 100644
> > --- a/drivers/firmware/efi/libstub/arm-stub.c
> > +++ b/drivers/firmware/efi/libstub/arm-stub.c
> > @@ -157,6 +157,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;
> > 
> > @@ -255,11 +256,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, ULONG_MAX,
> > -				 efi_get_max_initrd_addr(dram_base, *image_addr),
> > -				 &initrd_addr, &initrd_size);
> > +	max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
> > +	status = efi_load_initrd_devpath(&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, ULONG_MAX, max_addr,
> > +					 &initrd_addr, &initrd_size);
> > +		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 8e60a39df3c5..eaf45ea749b3 100644
> > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > @@ -323,3 +323,68 @@ void efi_char16_printk(efi_char16_t *str)
> >   	efi_call_proto(efi_table_attr(efi_system_table(), con_out),
> >   		       output_string, str);
> >   }
> > +
> > +static const struct {
> > +	struct efi_vendor_dev_path	vendor;
> > +	struct efi_generic_dev_path	end;
> > +} __packed initrd_devpath = {
> > +	{
> > +		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_status_t efi_load_initrd_devpath(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_devpath;
> > +	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;
> 
> You require here that there is a handle exposing the device path
> protocol with the initrd specific device path. On the same handle the
> EFI_LOAD_FILE2_PROTOCOL must be installed which will load the initrd
> file when called with the same device path.
> 
> An alternative implementation would simple loop over all instances of
> the EFI_LOAD_FILE2_PROTOCOL and try to load the initrd.
> 
> It would be worthwhile to describe the requirements on the
> implementation of the EFI_LOAD_FILE2_PROTOCOL in a code comment and in
> the documentation.
> 
> Unfortunately the documentation of UEFI has been duplicated into two files:
> 
> Documentation/arm/uefi.rst
> Documentation/x86/x86_64/uefi.rst
> 
> Best regards
> 
> Heinrich
> 
> > +
> > +	initrd_size = 0;
> > +	status = efi_call_proto(lf2, load_file,
> > +				(efi_device_path_protocol_t *)&initrd_devpath,
> > +				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,
> > +				(efi_device_path_protocol_t *)&initrd_devpath,
> > +				false, &initrd_size, (void *)initrd_addr);
> > +	if (status != EFI_SUCCESS) {
> > +		efi_free(initrd_size, initrd_addr);
> > +		return status;
> > +	}
> > +
> > +	*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 99e93fd76ec5..fbf9f9442eed 100644
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -566,6 +566,14 @@ union efi_load_file_protocol {
> >   	} mixed_mode;
> >   };
> > 
> > +struct efi_vendor_dev_path {
> > +	u8		type;
> > +	u8		sub_type;
> > +	u16		length;
> > +	efi_guid_t	vendorguid;
> > +	u8		vendordata[];
> > +} __packed;
> > +
> >   void efi_pci_disable_bridge_busmaster(void);
> > 
> >   typedef efi_status_t (*efi_exit_boot_map_processing)(
> > @@ -651,4 +659,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
> >   			     unsigned long *load_addr,
> >   			     unsigned long *load_size);
> > 
> > +efi_status_t efi_load_initrd_devpath(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 f3e2ff31b624..7f38f95676dd 100644
> > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > @@ -419,9 +419,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> >   	if (status != EFI_SUCCESS)
> >   		goto fail2;
> > 
> > -	status = efi_load_initrd(image, hdr->initrd_addr_max,
> > -				 above4g ? ULONG_MAX : hdr->initrd_addr_max,
> > -				 &ramdisk_addr, &ramdisk_size);
> > +	/*
> > +	 * The initrd loaded from the Linux initrd vendor device
> > +	 * path should take precedence, as we don't want the
> > +	 * [unverified] command line to override the initrd
> > +	 * supplied by the [potentially verified] firmware.
> > +	 */
> > +	status = efi_load_initrd_devpath(&ramdisk_addr, &ramdisk_size,
> > +					 above4g ? ULONG_MAX
> > +						 : hdr->initrd_addr_max);
> > +	if (status == EFI_NOT_FOUND)
> > +		status = efi_load_initrd(image, hdr->initrd_addr_max,
> > +					 above4g ? ULONG_MAX
> > +						 : hdr->initrd_addr_max,
> > +					 &ramdisk_addr, &ramdisk_size);
> >   	if (status != EFI_SUCCESS)
> >   		goto fail2;
> >   	hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
> > @@ -732,6 +743,25 @@ struct boot_params *efi_main(efi_handle_t handle,
> >   			 ((u64)boot_params->ext_cmd_line_ptr << 32));
> >   	efi_parse_options((char *)cmdline_paddr);
> > 
> > +	if (!hdr->ramdisk_size && !boot_params->ext_ramdisk_size) {
> > +		unsigned long max = hdr->initrd_addr_max;
> > +		unsigned long addr, size;
> > +
> > +		if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
> > +			max = ULONG_MAX;
> > +
> > +		status = efi_load_initrd_devpath(&addr, &size, max);
> > +		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_devpath() 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 9ccf313fe9de..75c83c322c40 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)
> > 
>

Heinrich Schuchardt Feb. 6, 2020, 7:15 p.m. UTC | #3

On 2/6/20 7:46 PM, Ilias Apalodimas wrote:
> Hi Heinrich,
>
> I actually like the approach.
>
> On Thu, Feb 06, 2020 at 07:26:20PM +0100, Heinrich Schuchardt wrote:
> [...]
>>> 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.
>>
>> Not relying on the command line for finding the initrd image does not
>> secure the integrity and the validity of the initrd image.
>
> This patch isn't supposed to protect you against a bogus initrd and I don't
> think it should be the bootloaders job to verify that. The kernel already has
> tools to do that.

How do you expect a bootloader identify if the initrd is compatible with
the kernel?

>
>>
>> A signature on the initrd image could solve the security problem you
>> describe. It would not require non-Linux software to be changed, and
>> would provide much better security.
>>
>
> The problem with parsing the kernel command line is that at the moment,
> different options are supported by different architectures. A quick grep for
> CONFIG_CMDLINE_(FORCE/OVERWRITE/EXTEND/FROM_BOOTLOADER) will give you an idea.
>
> What you can do in U-Boot currently is not have an environment in any of the
> flashes and set the bootdelay to 0, in order to prevent the user from
> changing that command line.

If you don't have an environment or boot script how would
update-initramfs set the path of the initrd when it is updated?

Using a UEFI variable seems to be the natural choice.

>
>>>
>>> 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.
>>
>> A firmware implementing the UEFI standard will not be aware of any
>> initrd image as such an object does not exist in the standard. It was a
>> wise decision that the UEFI standard is operating system agnostic
>> (accomodating BSD, Linux, Windows, etc.). So the firmware (EDK2, U-Boot,
>> etc.) seems to be out of scope for providing a Linux specific
>> EFI_LOAD_FILE2_PROTOCOL.
>>
>> When booting via GRUB it will be GRUB knowing which initrd to load.
>
> What about booting the kernel directly?
>
>>
>> Please, indicate which software you expect to expose the initrd related
>> EFI_LOAD_FILE2_PROTOCOL.
>
> I have an implementation for this on U-Boot which works. The file and device are
> hardcoded at the moment, but the rest of the functionality works fine. I'll
> share it with you once I clean it up a bit.

Using a UEFI variable for passing the intird device path to Linux does
not require any change in U-Boot and is compatible with the UEFI
implementations of existing hardware like the laptop on which I am
writing this email.

Best regards

Heinrich

>
>>
>> Using an UEFI variable for passing the initrd device path would be a
>> leaner solution on the bootloader side than requiring an extra
>> EFI_LOAD_FILE2_PROTOCOL implementation.
>>
>
> Thanks
> /Ilias
>
>>>
>>> Signed-off-by: Ard Biesheuvel <[e-mail]>
>>> ---
>>>    drivers/firmware/efi/libstub/arm-stub.c        | 16 +++--
>>>    drivers/firmware/efi/libstub/efi-stub-helper.c | 65 ++++++++++++++++++++
>>>    drivers/firmware/efi/libstub/efistub.h         | 12 ++++
>>>    drivers/firmware/efi/libstub/x86-stub.c        | 36 ++++++++++-
>>>    include/linux/efi.h                            |  1 +
>>>    5 files changed, 123 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>>> index c7b091f50e55..1db943c1ba2b 100644
>>> --- a/drivers/firmware/efi/libstub/arm-stub.c
>>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>>> @@ -157,6 +157,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;
>>>
>>> @@ -255,11 +256,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, ULONG_MAX,
>>> -				 efi_get_max_initrd_addr(dram_base, *image_addr),
>>> -				 &initrd_addr, &initrd_size);
>>> +	max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
>>> +	status = efi_load_initrd_devpath(&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, ULONG_MAX, max_addr,
>>> +					 &initrd_addr, &initrd_size);
>>> +		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 8e60a39df3c5..eaf45ea749b3 100644
>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>> @@ -323,3 +323,68 @@ void efi_char16_printk(efi_char16_t *str)
>>>    	efi_call_proto(efi_table_attr(efi_system_table(), con_out),
>>>    		       output_string, str);
>>>    }
>>> +
>>> +static const struct {
>>> +	struct efi_vendor_dev_path	vendor;
>>> +	struct efi_generic_dev_path	end;
>>> +} __packed initrd_devpath = {
>>> +	{
>>> +		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_status_t efi_load_initrd_devpath(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_devpath;
>>> +	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;
>>
>> You require here that there is a handle exposing the device path
>> protocol with the initrd specific device path. On the same handle the
>> EFI_LOAD_FILE2_PROTOCOL must be installed which will load the initrd
>> file when called with the same device path.
>>
>> An alternative implementation would simple loop over all instances of
>> the EFI_LOAD_FILE2_PROTOCOL and try to load the initrd.
>>
>> It would be worthwhile to describe the requirements on the
>> implementation of the EFI_LOAD_FILE2_PROTOCOL in a code comment and in
>> the documentation.
>>
>> Unfortunately the documentation of UEFI has been duplicated into two files:
>>
>> Documentation/arm/uefi.rst
>> Documentation/x86/x86_64/uefi.rst
>>
>> Best regards
>>
>> Heinrich
>>
>>> +
>>> +	initrd_size = 0;
>>> +	status = efi_call_proto(lf2, load_file,
>>> +				(efi_device_path_protocol_t *)&initrd_devpath,
>>> +				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,
>>> +				(efi_device_path_protocol_t *)&initrd_devpath,
>>> +				false, &initrd_size, (void *)initrd_addr);
>>> +	if (status != EFI_SUCCESS) {
>>> +		efi_free(initrd_size, initrd_addr);
>>> +		return status;
>>> +	}
>>> +
>>> +	*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 99e93fd76ec5..fbf9f9442eed 100644
>>> --- a/drivers/firmware/efi/libstub/efistub.h
>>> +++ b/drivers/firmware/efi/libstub/efistub.h
>>> @@ -566,6 +566,14 @@ union efi_load_file_protocol {
>>>    	} mixed_mode;
>>>    };
>>>
>>> +struct efi_vendor_dev_path {
>>> +	u8		type;
>>> +	u8		sub_type;
>>> +	u16		length;
>>> +	efi_guid_t	vendorguid;
>>> +	u8		vendordata[];
>>> +} __packed;
>>> +
>>>    void efi_pci_disable_bridge_busmaster(void);
>>>
>>>    typedef efi_status_t (*efi_exit_boot_map_processing)(
>>> @@ -651,4 +659,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
>>>    			     unsigned long *load_addr,
>>>    			     unsigned long *load_size);
>>>
>>> +efi_status_t efi_load_initrd_devpath(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 f3e2ff31b624..7f38f95676dd 100644
>>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>>> @@ -419,9 +419,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>>>    	if (status != EFI_SUCCESS)
>>>    		goto fail2;
>>>
>>> -	status = efi_load_initrd(image, hdr->initrd_addr_max,
>>> -				 above4g ? ULONG_MAX : hdr->initrd_addr_max,
>>> -				 &ramdisk_addr, &ramdisk_size);
>>> +	/*
>>> +	 * The initrd loaded from the Linux initrd vendor device
>>> +	 * path should take precedence, as we don't want the
>>> +	 * [unverified] command line to override the initrd
>>> +	 * supplied by the [potentially verified] firmware.
>>> +	 */
>>> +	status = efi_load_initrd_devpath(&ramdisk_addr, &ramdisk_size,
>>> +					 above4g ? ULONG_MAX
>>> +						 : hdr->initrd_addr_max);
>>> +	if (status == EFI_NOT_FOUND)
>>> +		status = efi_load_initrd(image, hdr->initrd_addr_max,
>>> +					 above4g ? ULONG_MAX
>>> +						 : hdr->initrd_addr_max,
>>> +					 &ramdisk_addr, &ramdisk_size);
>>>    	if (status != EFI_SUCCESS)
>>>    		goto fail2;
>>>    	hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
>>> @@ -732,6 +743,25 @@ struct boot_params *efi_main(efi_handle_t handle,
>>>    			 ((u64)boot_params->ext_cmd_line_ptr << 32));
>>>    	efi_parse_options((char *)cmdline_paddr);
>>>
>>> +	if (!hdr->ramdisk_size && !boot_params->ext_ramdisk_size) {
>>> +		unsigned long max = hdr->initrd_addr_max;
>>> +		unsigned long addr, size;
>>> +
>>> +		if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
>>> +			max = ULONG_MAX;
>>> +
>>> +		status = efi_load_initrd_devpath(&addr, &size, max);
>>> +		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_devpath() 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 9ccf313fe9de..75c83c322c40 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)
>>>
>>

Ilias Apalodimas Feb. 6, 2020, 8:09 p.m. UTC | #4

On Thu, Feb 06, 2020 at 08:15:11PM +0100, Heinrich Schuchardt wrote:
> On 2/6/20 7:46 PM, Ilias Apalodimas wrote:
> > Hi Heinrich,
> > 
> > I actually like the approach.
> > 
> > On Thu, Feb 06, 2020 at 07:26:20PM +0100, Heinrich Schuchardt wrote:
> > [...]
> > > > 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.
> > > 
> > > Not relying on the command line for finding the initrd image does not
> > > secure the integrity and the validity of the initrd image.
> > 
> > This patch isn't supposed to protect you against a bogus initrd and I don't
> > think it should be the bootloaders job to verify that. The kernel already has
> > tools to do that.
> 
> How do you expect a bootloader identify if the initrd is compatible with
> the kernel?

Uh? Were exactly did i say that i expect that to happen?

> 
> > 
> > > 
> > > A signature on the initrd image could solve the security problem you
> > > describe. It would not require non-Linux software to be changed, and
> > > would provide much better security.
> > > 
> > 
> > The problem with parsing the kernel command line is that at the moment,
> > different options are supported by different architectures. A quick grep for
> > CONFIG_CMDLINE_(FORCE/OVERWRITE/EXTEND/FROM_BOOTLOADER) will give you an idea.
> > 
> > What you can do in U-Boot currently is not have an environment in any of the
> > flashes and set the bootdelay to 0, in order to prevent the user from
> > changing that command line.
> 
> If you don't have an environment or boot script how would
> update-initramfs set the path of the initrd when it is updated?

The path isn't hardcoded in any code here is it?
This specifies a way for the linux stub to load the actual file. It's pretty a
callback to the firmware. Were the firmware will find and how it will load it 
eventually is implementation specific. 

> 
> Using a UEFI variable seems to be the natural choice.
> 

You might as well use that to specify were you should load the file from.
The Loadfile2 (with the specified guid)  implementation of the firmware will 
take care of that.

> > 
> > > > 
> > > > 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.
> > > 
> > > A firmware implementing the UEFI standard will not be aware of any
> > > initrd image as such an object does not exist in the standard. It was a
> > > wise decision that the UEFI standard is operating system agnostic
> > > (accomodating BSD, Linux, Windows, etc.). So the firmware (EDK2, U-Boot,
> > > etc.) seems to be out of scope for providing a Linux specific
> > > EFI_LOAD_FILE2_PROTOCOL.
> > > 
> > > When booting via GRUB it will be GRUB knowing which initrd to load.
> > 
> > What about booting the kernel directly?
> > 
> > > 
> > > Please, indicate which software you expect to expose the initrd related
> > > EFI_LOAD_FILE2_PROTOCOL.
> > 
> > I have an implementation for this on U-Boot which works. The file and device are
> > hardcoded at the moment, but the rest of the functionality works fine. I'll
> > share it with you once I clean it up a bit.
> 
> Using a UEFI variable for passing the intird device path to Linux does
> not require any change in U-Boot and is compatible with the UEFI
> implementations of existing hardware like the laptop on which I am
> writing this email.

This still has the same issues we have now, uefi variable, kernel command line
or whatever, it won't be common across architectures.

Thanks
/Ilias
> 
> Best regards
> 
> Heinrich
> 
> > 
> > > 
> > > Using an UEFI variable for passing the initrd device path would be a
> > > leaner solution on the bootloader side than requiring an extra
> > > EFI_LOAD_FILE2_PROTOCOL implementation.
> > > 
> > 
> > Thanks
> > /Ilias
> > 
> > > > 
> > > > Signed-off-by: Ard Biesheuvel <[e-mail]>
> > > > ---
> > > >    drivers/firmware/efi/libstub/arm-stub.c        | 16 +++--
> > > >    drivers/firmware/efi/libstub/efi-stub-helper.c | 65 ++++++++++++++++++++
> > > >    drivers/firmware/efi/libstub/efistub.h         | 12 ++++
> > > >    drivers/firmware/efi/libstub/x86-stub.c        | 36 ++++++++++-
> > > >    include/linux/efi.h                            |  1 +
> > > >    5 files changed, 123 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> > > > index c7b091f50e55..1db943c1ba2b 100644
> > > > --- a/drivers/firmware/efi/libstub/arm-stub.c
> > > > +++ b/drivers/firmware/efi/libstub/arm-stub.c
> > > > @@ -157,6 +157,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;
> > > > 
> > > > @@ -255,11 +256,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, ULONG_MAX,
> > > > -				 efi_get_max_initrd_addr(dram_base, *image_addr),
> > > > -				 &initrd_addr, &initrd_size);
> > > > +	max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
> > > > +	status = efi_load_initrd_devpath(&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, ULONG_MAX, max_addr,
> > > > +					 &initrd_addr, &initrd_size);
> > > > +		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 8e60a39df3c5..eaf45ea749b3 100644
> > > > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > > @@ -323,3 +323,68 @@ void efi_char16_printk(efi_char16_t *str)
> > > >    	efi_call_proto(efi_table_attr(efi_system_table(), con_out),
> > > >    		       output_string, str);
> > > >    }
> > > > +
> > > > +static const struct {
> > > > +	struct efi_vendor_dev_path	vendor;
> > > > +	struct efi_generic_dev_path	end;
> > > > +} __packed initrd_devpath = {
> > > > +	{
> > > > +		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_status_t efi_load_initrd_devpath(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_devpath;
> > > > +	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;
> > > 
> > > You require here that there is a handle exposing the device path
> > > protocol with the initrd specific device path. On the same handle the
> > > EFI_LOAD_FILE2_PROTOCOL must be installed which will load the initrd
> > > file when called with the same device path.
> > > 
> > > An alternative implementation would simple loop over all instances of
> > > the EFI_LOAD_FILE2_PROTOCOL and try to load the initrd.
> > > 
> > > It would be worthwhile to describe the requirements on the
> > > implementation of the EFI_LOAD_FILE2_PROTOCOL in a code comment and in
> > > the documentation.
> > > 
> > > Unfortunately the documentation of UEFI has been duplicated into two files:
> > > 
> > > Documentation/arm/uefi.rst
> > > Documentation/x86/x86_64/uefi.rst
> > > 
> > > Best regards
> > > 
> > > Heinrich
> > > 
> > > > +
> > > > +	initrd_size = 0;
> > > > +	status = efi_call_proto(lf2, load_file,
> > > > +				(efi_device_path_protocol_t *)&initrd_devpath,
> > > > +				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,
> > > > +				(efi_device_path_protocol_t *)&initrd_devpath,
> > > > +				false, &initrd_size, (void *)initrd_addr);
> > > > +	if (status != EFI_SUCCESS) {
> > > > +		efi_free(initrd_size, initrd_addr);
> > > > +		return status;
> > > > +	}
> > > > +
> > > > +	*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 99e93fd76ec5..fbf9f9442eed 100644
> > > > --- a/drivers/firmware/efi/libstub/efistub.h
> > > > +++ b/drivers/firmware/efi/libstub/efistub.h
> > > > @@ -566,6 +566,14 @@ union efi_load_file_protocol {
> > > >    	} mixed_mode;
> > > >    };
> > > > 
> > > > +struct efi_vendor_dev_path {
> > > > +	u8		type;
> > > > +	u8		sub_type;
> > > > +	u16		length;
> > > > +	efi_guid_t	vendorguid;
> > > > +	u8		vendordata[];
> > > > +} __packed;
> > > > +
> > > >    void efi_pci_disable_bridge_busmaster(void);
> > > > 
> > > >    typedef efi_status_t (*efi_exit_boot_map_processing)(
> > > > @@ -651,4 +659,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
> > > >    			     unsigned long *load_addr,
> > > >    			     unsigned long *load_size);
> > > > 
> > > > +efi_status_t efi_load_initrd_devpath(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 f3e2ff31b624..7f38f95676dd 100644
> > > > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > > > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > > > @@ -419,9 +419,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> > > >    	if (status != EFI_SUCCESS)
> > > >    		goto fail2;
> > > > 
> > > > -	status = efi_load_initrd(image, hdr->initrd_addr_max,
> > > > -				 above4g ? ULONG_MAX : hdr->initrd_addr_max,
> > > > -				 &ramdisk_addr, &ramdisk_size);
> > > > +	/*
> > > > +	 * The initrd loaded from the Linux initrd vendor device
> > > > +	 * path should take precedence, as we don't want the
> > > > +	 * [unverified] command line to override the initrd
> > > > +	 * supplied by the [potentially verified] firmware.
> > > > +	 */
> > > > +	status = efi_load_initrd_devpath(&ramdisk_addr, &ramdisk_size,
> > > > +					 above4g ? ULONG_MAX
> > > > +						 : hdr->initrd_addr_max);
> > > > +	if (status == EFI_NOT_FOUND)
> > > > +		status = efi_load_initrd(image, hdr->initrd_addr_max,
> > > > +					 above4g ? ULONG_MAX
> > > > +						 : hdr->initrd_addr_max,
> > > > +					 &ramdisk_addr, &ramdisk_size);
> > > >    	if (status != EFI_SUCCESS)
> > > >    		goto fail2;
> > > >    	hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
> > > > @@ -732,6 +743,25 @@ struct boot_params *efi_main(efi_handle_t handle,
> > > >    			 ((u64)boot_params->ext_cmd_line_ptr << 32));
> > > >    	efi_parse_options((char *)cmdline_paddr);
> > > > 
> > > > +	if (!hdr->ramdisk_size && !boot_params->ext_ramdisk_size) {
> > > > +		unsigned long max = hdr->initrd_addr_max;
> > > > +		unsigned long addr, size;
> > > > +
> > > > +		if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
> > > > +			max = ULONG_MAX;
> > > > +
> > > > +		status = efi_load_initrd_devpath(&addr, &size, max);
> > > > +		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_devpath() 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 9ccf313fe9de..75c83c322c40 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)
> > > > 
> > > 
>

Ard Biesheuvel Feb. 6, 2020, 10:35 p.m. UTC | #5

On Thu, 6 Feb 2020 at 18:26, Heinrich Schuchardt <[e-mail]> wrote:
>
> On 2/6/20 3:03 PM, 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
>
> Hello Ard,
>
> "same file system" is not a limitation of using a command line
> parameter. Any device path can be passed as a string.
>

What do you mean? The current implementation opens the volume via the
loaded_image_info struct, and finds the file based on its filename,
not on a device path. So how can it access files on other file
systems?


> > 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.
>
> Not relying on the command line for finding the initrd image does not
> secure the integrity and the validity of the initrd image.
>

It does. It ensures that [signed] bootloader code is in charge of
providing the initrd at the point during the boot where the kernel is
ready to consume this data.

> A signature on the initrd image could solve the security problem you
> describe. It would not require non-Linux software to be changed, and
> would provide much better security.
>

A signed initrd would be useful, too, but it doesn't fix the whole problem.

Linux does not support signed initrds today, and this feature permits
a firmware implementation to do something very similar, i.e., it
permits the bootloader to perform the verification as it is passed to
the kernel.


> >
> > 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.
>
> A firmware implementing the UEFI standard will not be aware of any
> initrd image as such an object does not exist in the standard. It was a
> wise decision that the UEFI standard is operating system agnostic
> (accomodating BSD, Linux, Windows, etc.). So the firmware (EDK2, U-Boot,
> etc.) seems to be out of scope for providing a Linux specific
> EFI_LOAD_FILE2_PROTOCOL.
>

You know we are currently patching bootparams struct and DTs to
provide the initrd information, right? And we have code that knows
about low/high memory limits, alignment, etc, that is different per
architecture.

> When booting via GRUB it will be GRUB knowing which initrd to load.
>

Exactly, which is why GRUB will implement this protocol. That way, it
does not have to touch the DT at all, or create a bootparams struct
from setup data and inspect the various flags about placement,
alignment, preferred addresses, etc.

> Please, indicate which software you expect to expose the initrd related
> EFI_LOAD_FILE2_PROTOCOL.
>

The primary use case is GRUB and other intermediate loaders, since it
would remove any need for these components to know any such details.
My aim is to make the next architecture that gets added to GRUB for
EFI boot 100% generic.

> Using an UEFI variable for passing the initrd device path would be a
> leaner solution on the bootloader side than requiring an extra
> EFI_LOAD_FILE2_PROTOCOL implementation.
>

This would also require kernel changes, since we don't currently load
initrds from arbitrary device paths. The EFI_FILE_PROTOCOL is much
more complicated than needed, and it doesn't work well with mixed
mode. It also requires GRUB to expose the filesystem it loads the
initrd from via EFI protocols, which is currently unnecessary and
therefore not implemented.

Also, using an EFI variable defeats the purpose. The whole point of
this is making it more likely that the kernel loaded the initrd that
the bootloader or firmware intended it to load, and having a piece of
simple [signed] code that implements this is the easiest way to
achieve that.

For u-boot, it should be trivial to implement a simple LoadFile2
protocol wrapper around EFI_FILE_PROTOCOL that can be installed on a
handle that also carries EFI_FILE_PROTOCOL.

> >
> > Signed-off-by: Ard Biesheuvel <[e-mail]>
> > ---
> >   drivers/firmware/efi/libstub/arm-stub.c        | 16 +++--
> >   drivers/firmware/efi/libstub/efi-stub-helper.c | 65 ++++++++++++++++++++
> >   drivers/firmware/efi/libstub/efistub.h         | 12 ++++
> >   drivers/firmware/efi/libstub/x86-stub.c        | 36 ++++++++++-
> >   include/linux/efi.h                            |  1 +
> >   5 files changed, 123 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> > index c7b091f50e55..1db943c1ba2b 100644
> > --- a/drivers/firmware/efi/libstub/arm-stub.c
> > +++ b/drivers/firmware/efi/libstub/arm-stub.c
> > @@ -157,6 +157,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;
> >
> > @@ -255,11 +256,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, ULONG_MAX,
> > -                              efi_get_max_initrd_addr(dram_base, *image_addr),
> > -                              &initrd_addr, &initrd_size);
> > +     max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
> > +     status = efi_load_initrd_devpath(&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, ULONG_MAX, max_addr,
> > +                                      &initrd_addr, &initrd_size);
> > +             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 8e60a39df3c5..eaf45ea749b3 100644
> > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > @@ -323,3 +323,68 @@ void efi_char16_printk(efi_char16_t *str)
> >       efi_call_proto(efi_table_attr(efi_system_table(), con_out),
> >                      output_string, str);
> >   }
> > +
> > +static const struct {
> > +     struct efi_vendor_dev_path      vendor;
> > +     struct efi_generic_dev_path     end;
> > +} __packed initrd_devpath = {
> > +     {
> > +             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_status_t efi_load_initrd_devpath(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_devpath;
> > +     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;
>
> You require here that there is a handle exposing the device path
> protocol with the initrd specific device path. On the same handle the
> EFI_LOAD_FILE2_PROTOCOL must be installed which will load the initrd
> file when called with the same device path.
>

Exactly.

> An alternative implementation would simple loop over all instances of
> the EFI_LOAD_FILE2_PROTOCOL and try to load the initrd.
>

How do I distinguish the initrd from other EFI_LOAD_FILE2_PROTOCOL
instances? For instance, PCI option ROMs are also exposed as
EFI_LOAD_FILE2_PROTOCOL.

> It would be worthwhile to describe the requirements on the
> implementation of the EFI_LOAD_FILE2_PROTOCOL in a code comment and in
> the documentation.
>
> Unfortunately the documentation of UEFI has been duplicated into two files:
>
> Documentation/arm/uefi.rst
> Documentation/x86/x86_64/uefi.rst
>

Yes, that is a good point. I will work on factoring out the generic
parts and share them.


Thanks for the review,
Ard.



>
> > +
> > +     initrd_size = 0;
> > +     status = efi_call_proto(lf2, load_file,
> > +                             (efi_device_path_protocol_t *)&initrd_devpath,
> > +                             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,
> > +                             (efi_device_path_protocol_t *)&initrd_devpath,
> > +                             false, &initrd_size, (void *)initrd_addr);
> > +     if (status != EFI_SUCCESS) {
> > +             efi_free(initrd_size, initrd_addr);
> > +             return status;
> > +     }
> > +
> > +     *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 99e93fd76ec5..fbf9f9442eed 100644
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -566,6 +566,14 @@ union efi_load_file_protocol {
> >       } mixed_mode;
> >   };
> >
> > +struct efi_vendor_dev_path {
> > +     u8              type;
> > +     u8              sub_type;
> > +     u16             length;
> > +     efi_guid_t      vendorguid;
> > +     u8              vendordata[];
> > +} __packed;
> > +
> >   void efi_pci_disable_bridge_busmaster(void);
> >
> >   typedef efi_status_t (*efi_exit_boot_map_processing)(
> > @@ -651,4 +659,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
> >                            unsigned long *load_addr,
> >                            unsigned long *load_size);
> >
> > +efi_status_t efi_load_initrd_devpath(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 f3e2ff31b624..7f38f95676dd 100644
> > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > @@ -419,9 +419,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> >       if (status != EFI_SUCCESS)
> >               goto fail2;
> >
> > -     status = efi_load_initrd(image, hdr->initrd_addr_max,
> > -                              above4g ? ULONG_MAX : hdr->initrd_addr_max,
> > -                              &ramdisk_addr, &ramdisk_size);
> > +     /*
> > +      * The initrd loaded from the Linux initrd vendor device
> > +      * path should take precedence, as we don't want the
> > +      * [unverified] command line to override the initrd
> > +      * supplied by the [potentially verified] firmware.
> > +      */
> > +     status = efi_load_initrd_devpath(&ramdisk_addr, &ramdisk_size,
> > +                                      above4g ? ULONG_MAX
> > +                                              : hdr->initrd_addr_max);
> > +     if (status == EFI_NOT_FOUND)
> > +             status = efi_load_initrd(image, hdr->initrd_addr_max,
> > +                                      above4g ? ULONG_MAX
> > +                                              : hdr->initrd_addr_max,
> > +                                      &ramdisk_addr, &ramdisk_size);
> >       if (status != EFI_SUCCESS)
> >               goto fail2;
> >       hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
> > @@ -732,6 +743,25 @@ struct boot_params *efi_main(efi_handle_t handle,
> >                        ((u64)boot_params->ext_cmd_line_ptr << 32));
> >       efi_parse_options((char *)cmdline_paddr);
> >
> > +     if (!hdr->ramdisk_size && !boot_params->ext_ramdisk_size) {
> > +             unsigned long max = hdr->initrd_addr_max;
> > +             unsigned long addr, size;
> > +
> > +             if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
> > +                     max = ULONG_MAX;
> > +
> > +             status = efi_load_initrd_devpath(&addr, &size, max);
> > +             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_devpath() 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 9ccf313fe9de..75c83c322c40 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)
> >
>

Heinrich Schuchardt Feb. 6, 2020, 10:49 p.m. UTC | #6

On 2/6/20 9:09 PM, Ilias Apalodimas wrote:
> On Thu, Feb 06, 2020 at 08:15:11PM +0100, Heinrich Schuchardt wrote:
>> On 2/6/20 7:46 PM, Ilias Apalodimas wrote:
>>> Hi Heinrich,
>>>
>>> I actually like the approach.
>>>
>>> On Thu, Feb 06, 2020 at 07:26:20PM +0100, Heinrich Schuchardt wrote:
>>> [...]
>>>>> 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.
>>>>
>>>> Not relying on the command line for finding the initrd image does not
>>>> secure the integrity and the validity of the initrd image.
>>>
>>> This patch isn't supposed to protect you against a bogus initrd and I don't
>>> think it should be the bootloaders job to verify that. The kernel already has
>>> tools to do that.
>>
>> How do you expect a bootloader identify if the initrd is compatible with
>> the kernel?
>
> Uh? Were exactly did i say that i expect that to happen?
>
>>
>>>
>>>>
>>>> A signature on the initrd image could solve the security problem you
>>>> describe. It would not require non-Linux software to be changed, and
>>>> would provide much better security.
>>>>
>>>
>>> The problem with parsing the kernel command line is that at the moment,
>>> different options are supported by different architectures. A quick grep for
>>> CONFIG_CMDLINE_(FORCE/OVERWRITE/EXTEND/FROM_BOOTLOADER) will give you an idea.
>>>
>>> What you can do in U-Boot currently is not have an environment in any of the
>>> flashes and set the bootdelay to 0, in order to prevent the user from
>>> changing that command line.
>>
>> If you don't have an environment or boot script how would
>> update-initramfs set the path of the initrd when it is updated?
>
> The path isn't hardcoded in any code here is it?
> This specifies a way for the linux stub to load the actual file. It's pretty a
> callback to the firmware. Were the firmware will find and how it will load it
> eventually is implementation specific.

"Implementation specific" - This does not sound like anything you would
want to have in mainline Linux, U-Boot, or EDK2.

>
>>
>> Using a UEFI variable seems to be the natural choice.
>>
>
> You might as well use that to specify were you should load the file from.
> The Loadfile2 (with the specified guid)  implementation of the firmware will
> take care of that.
>

If we have a UEFI variable, the Linux kernel can use it to find the
handle with the file system and load initrd via the
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.

This way we stay within the existing UEFI specification and avoid
anything "implementation specific" in the firmware.

If you want extra security, Linux can use an authenticated variable.

>>>
>>>>>
>>>>> 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.
>>>>
>>>> A firmware implementing the UEFI standard will not be aware of any
>>>> initrd image as such an object does not exist in the standard. It was a
>>>> wise decision that the UEFI standard is operating system agnostic
>>>> (accomodating BSD, Linux, Windows, etc.). So the firmware (EDK2, U-Boot,
>>>> etc.) seems to be out of scope for providing a Linux specific
>>>> EFI_LOAD_FILE2_PROTOCOL.
>>>>
>>>> When booting via GRUB it will be GRUB knowing which initrd to load.
>>>
>>> What about booting the kernel directly?
>>>
>>>>
>>>> Please, indicate which software you expect to expose the initrd related
>>>> EFI_LOAD_FILE2_PROTOCOL.
>>>
>>> I have an implementation for this on U-Boot which works. The file and device are
>>> hardcoded at the moment, but the rest of the functionality works fine. I'll
>>> share it with you once I clean it up a bit.
>>
>> Using a UEFI variable for passing the intird device path to Linux does
>> not require any change in U-Boot and is compatible with the UEFI
>> implementations of existing hardware like the laptop on which I am
>> writing this email.
>
> This still has the same issues we have now, uefi variable, kernel command line
> or whatever, it won't be common across architectures.

This would be a bad design choice by Linux. I cannot see why a UEFI
variable should not be interpreted in a consistent way inside Linux to
load a file via the EFI_SIMPLE_FILE_PROTOCOL.

>
> Thanks
> /Ilias
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>>
>>>> Using an UEFI variable for passing the initrd device path would be a
>>>> leaner solution on the bootloader side than requiring an extra
>>>> EFI_LOAD_FILE2_PROTOCOL implementation.
>>>>
>>>
>>> Thanks
>>> /Ilias
>>>
>>>>>
>>>>> Signed-off-by: Ard Biesheuvel <[e-mail]>
>>>>> ---
>>>>>     drivers/firmware/efi/libstub/arm-stub.c        | 16 +++--
>>>>>     drivers/firmware/efi/libstub/efi-stub-helper.c | 65 ++++++++++++++++++++
>>>>>     drivers/firmware/efi/libstub/efistub.h         | 12 ++++
>>>>>     drivers/firmware/efi/libstub/x86-stub.c        | 36 ++++++++++-
>>>>>     include/linux/efi.h                            |  1 +
>>>>>     5 files changed, 123 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>>>>> index c7b091f50e55..1db943c1ba2b 100644
>>>>> --- a/drivers/firmware/efi/libstub/arm-stub.c
>>>>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>>>>> @@ -157,6 +157,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;
>>>>>
>>>>> @@ -255,11 +256,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, ULONG_MAX,
>>>>> -				 efi_get_max_initrd_addr(dram_base, *image_addr),
>>>>> -				 &initrd_addr, &initrd_size);
>>>>> +	max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
>>>>> +	status = efi_load_initrd_devpath(&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, ULONG_MAX, max_addr,
>>>>> +					 &initrd_addr, &initrd_size);

If I delete the initrd that otherwise would be loaded by the
EFI_LOAD_FILE2_PROTOCOL I end up with the old behavior. So where is the
security gain provided by this patch?

Best regards

Heinrich


>>>>> +		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 8e60a39df3c5..eaf45ea749b3 100644
>>>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>>>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>>>> @@ -323,3 +323,68 @@ void efi_char16_printk(efi_char16_t *str)
>>>>>     	efi_call_proto(efi_table_attr(efi_system_table(), con_out),
>>>>>     		       output_string, str);
>>>>>     }
>>>>> +
>>>>> +static const struct {
>>>>> +	struct efi_vendor_dev_path	vendor;
>>>>> +	struct efi_generic_dev_path	end;
>>>>> +} __packed initrd_devpath = {
>>>>> +	{
>>>>> +		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_status_t efi_load_initrd_devpath(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_devpath;
>>>>> +	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;
>>>>
>>>> You require here that there is a handle exposing the device path
>>>> protocol with the initrd specific device path. On the same handle the
>>>> EFI_LOAD_FILE2_PROTOCOL must be installed which will load the initrd
>>>> file when called with the same device path.
>>>>
>>>> An alternative implementation would simple loop over all instances of
>>>> the EFI_LOAD_FILE2_PROTOCOL and try to load the initrd.
>>>>
>>>> It would be worthwhile to describe the requirements on the
>>>> implementation of the EFI_LOAD_FILE2_PROTOCOL in a code comment and in
>>>> the documentation.
>>>>
>>>> Unfortunately the documentation of UEFI has been duplicated into two files:
>>>>
>>>> Documentation/arm/uefi.rst
>>>> Documentation/x86/x86_64/uefi.rst
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>> +
>>>>> +	initrd_size = 0;
>>>>> +	status = efi_call_proto(lf2, load_file,
>>>>> +				(efi_device_path_protocol_t *)&initrd_devpath,
>>>>> +				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,
>>>>> +				(efi_device_path_protocol_t *)&initrd_devpath,
>>>>> +				false, &initrd_size, (void *)initrd_addr);
>>>>> +	if (status != EFI_SUCCESS) {
>>>>> +		efi_free(initrd_size, initrd_addr);
>>>>> +		return status;
>>>>> +	}
>>>>> +
>>>>> +	*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 99e93fd76ec5..fbf9f9442eed 100644
>>>>> --- a/drivers/firmware/efi/libstub/efistub.h
>>>>> +++ b/drivers/firmware/efi/libstub/efistub.h
>>>>> @@ -566,6 +566,14 @@ union efi_load_file_protocol {
>>>>>     	} mixed_mode;
>>>>>     };
>>>>>
>>>>> +struct efi_vendor_dev_path {
>>>>> +	u8		type;
>>>>> +	u8		sub_type;
>>>>> +	u16		length;
>>>>> +	efi_guid_t	vendorguid;
>>>>> +	u8		vendordata[];
>>>>> +} __packed;
>>>>> +
>>>>>     void efi_pci_disable_bridge_busmaster(void);
>>>>>
>>>>>     typedef efi_status_t (*efi_exit_boot_map_processing)(
>>>>> @@ -651,4 +659,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
>>>>>     			     unsigned long *load_addr,
>>>>>     			     unsigned long *load_size);
>>>>>
>>>>> +efi_status_t efi_load_initrd_devpath(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 f3e2ff31b624..7f38f95676dd 100644
>>>>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>>>>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>>>>> @@ -419,9 +419,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>>>>>     	if (status != EFI_SUCCESS)
>>>>>     		goto fail2;
>>>>>
>>>>> -	status = efi_load_initrd(image, hdr->initrd_addr_max,
>>>>> -				 above4g ? ULONG_MAX : hdr->initrd_addr_max,
>>>>> -				 &ramdisk_addr, &ramdisk_size);
>>>>> +	/*
>>>>> +	 * The initrd loaded from the Linux initrd vendor device
>>>>> +	 * path should take precedence, as we don't want the
>>>>> +	 * [unverified] command line to override the initrd
>>>>> +	 * supplied by the [potentially verified] firmware.
>>>>> +	 */
>>>>> +	status = efi_load_initrd_devpath(&ramdisk_addr, &ramdisk_size,
>>>>> +					 above4g ? ULONG_MAX
>>>>> +						 : hdr->initrd_addr_max);
>>>>> +	if (status == EFI_NOT_FOUND)
>>>>> +		status = efi_load_initrd(image, hdr->initrd_addr_max,
>>>>> +					 above4g ? ULONG_MAX
>>>>> +						 : hdr->initrd_addr_max,
>>>>> +					 &ramdisk_addr, &ramdisk_size);
>>>>>     	if (status != EFI_SUCCESS)
>>>>>     		goto fail2;
>>>>>     	hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
>>>>> @@ -732,6 +743,25 @@ struct boot_params *efi_main(efi_handle_t handle,
>>>>>     			 ((u64)boot_params->ext_cmd_line_ptr << 32));
>>>>>     	efi_parse_options((char *)cmdline_paddr);
>>>>>
>>>>> +	if (!hdr->ramdisk_size && !boot_params->ext_ramdisk_size) {
>>>>> +		unsigned long max = hdr->initrd_addr_max;
>>>>> +		unsigned long addr, size;
>>>>> +
>>>>> +		if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
>>>>> +			max = ULONG_MAX;
>>>>> +
>>>>> +		status = efi_load_initrd_devpath(&addr, &size, max);
>>>>> +		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_devpath() 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 9ccf313fe9de..75c83c322c40 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)
>>>>>
>>>>
>>

Heinrich Schuchardt Feb. 7, 2020, 12:01 a.m. UTC | #7

On 2/6/20 11:35 PM, Ard Biesheuvel wrote:
> On Thu, 6 Feb 2020 at 18:26, Heinrich Schuchardt <[e-mail]> wrote:
>>
>> On 2/6/20 3:03 PM, 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
>>
>> Hello Ard,
>>
>> "same file system" is not a limitation of using a command line
>> parameter. Any device path can be passed as a string.
>>
>
> What do you mean? The current implementation opens the volume via the
> loaded_image_info struct, and finds the file based on its filename,
> not on a device path. So how can it access files on other file
> systems?
>

I talked aobut the information can be transmitted in a parameter not
about the restrictions the current Linux command line implementation has.

If you would pass a complete device path as parameter, you could use the
UEFI API to find the device with block file system and use the
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL to load the file.

>
>>> 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.
>>
>> Not relying on the command line for finding the initrd image does not
>> secure the integrity and the validity of the initrd image.
>>
>
> It does. It ensures that [signed] bootloader code is in charge of
> providing the initrd at the point during the boot where the kernel is
> ready to consume this data.
>
>> A signature on the initrd image could solve the security problem you
>> describe. It would not require non-Linux software to be changed, and
>> would provide much better security.
>>
>
> A signed initrd would be useful, too, but it doesn't fix the whole problem.
>
> Linux does not support signed initrds today, and this feature permits
> a firmware implementation to do something very similar, i.e., it
> permits the bootloader to perform the verification as it is passed to
> the kernel.

One source of changed initrds is update-initramfs called in an operating
system update process.

How shall a bootloader verify the new initrd if we do not define what a
signed initrd looks like?

Would a bootloader be in a better position to verify an intird then
Linux which already has code for file verification used when loading
modules?

>
>
>>>
>>> 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.
>>
>> A firmware implementing the UEFI standard will not be aware of any
>> initrd image as such an object does not exist in the standard. It was a
>> wise decision that the UEFI standard is operating system agnostic
>> (accomodating BSD, Linux, Windows, etc.). So the firmware (EDK2, U-Boot,
>> etc.) seems to be out of scope for providing a Linux specific
>> EFI_LOAD_FILE2_PROTOCOL.
>>
>
> You know we are currently patching bootparams struct and DTs to
> provide the initrd information, right? And we have code that knows
> about low/high memory limits, alignment, etc, that is different per
> architecture.
>
>> When booting via GRUB it will be GRUB knowing which initrd to load.
>>
>
> Exactly, which is why GRUB will implement this protocol. That way, it
> does not have to touch the DT at all, or create a bootparams struct
> from setup data and inspect the various flags about placement,
> alignment, preferred addresses, etc.
>
>> Please, indicate which software you expect to expose the initrd related
>> EFI_LOAD_FILE2_PROTOCOL.
>>
>
> The primary use case is GRUB and other intermediate loaders, since it
> would remove any need for these components to know any such details.
> My aim is to make the next architecture that gets added to GRUB for
> EFI boot 100% generic.
>
>> Using an UEFI variable for passing the initrd device path would be a
>> leaner solution on the bootloader side than requiring an extra
>> EFI_LOAD_FILE2_PROTOCOL implementation.
>>
>
> This would also require kernel changes, since we don't currently load
> initrds from arbitrary device paths. The EFI_FILE_PROTOCOL is much
> more complicated than needed, and it doesn't work well with mixed
> mode. It also requires GRUB to expose the filesystem it loads the
> initrd from via EFI protocols, which is currently unnecessary and
> therefore not implemented.

This means you move the complexity of EFI_FILE_PROTOCOL from Linux to GRUB.

I would not have a problem if this would only touch GRUB. But if listen
to Ilias we are replacing one implementation in Linux by one in GRUB and
one in U-Boot and one in EDK2 and one in any other firmware.

>
> Also, using an EFI variable defeats the purpose. The whole point of
> this is making it more likely that the kernel loaded the initrd that
> the bootloader or firmware intended it to load, and having a piece of
> simple [signed] code that implements this is the easiest way to
> achieve that.

At least on my Debian system it is the operating system creating initrd
and defining which initrd matches which kernel. GRUB simply assumes that
files ending on the same version number match. Therefore I would say
Linux hopes that GRUB loads what Linux intended.

The chain of trust would not be broken if the kernel were responsible
for loading the initrd and for checking if it matches the kernel. Linux
already does this for the kernel modules in initrd.

>
> For u-boot, it should be trivial to implement a simple LoadFile2
> protocol wrapper around EFI_FILE_PROTOCOL that can be installed on a
> handle that also carries EFI_FILE_PROTOCOL.
>

A U-Boot implementation of the EFI_LOAD_FILE2_PROTOCOL would need a
device path variable to find the block device and to open the
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL before accessing the file.

Linux would not be needing more lines and we would not repeat the same
code in GRUB, U-Boot, EDK2, etc.

As said Linux updates the initrd often. If that file is not signed by
Linux in a well defined way, do not expect any security at all.

If Linux does not tell which kernel matches which initrd, how should
U-Boot know? - How do you plan to transfer this piece of information?

>>>
>>> Signed-off-by: Ard Biesheuvel <[e-mail]>
>>> ---
>>>    drivers/firmware/efi/libstub/arm-stub.c        | 16 +++--
>>>    drivers/firmware/efi/libstub/efi-stub-helper.c | 65 ++++++++++++++++++++
>>>    drivers/firmware/efi/libstub/efistub.h         | 12 ++++
>>>    drivers/firmware/efi/libstub/x86-stub.c        | 36 ++++++++++-
>>>    include/linux/efi.h                            |  1 +
>>>    5 files changed, 123 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>>> index c7b091f50e55..1db943c1ba2b 100644
>>> --- a/drivers/firmware/efi/libstub/arm-stub.c
>>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>>> @@ -157,6 +157,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;
>>>
>>> @@ -255,11 +256,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, ULONG_MAX,
>>> -                              efi_get_max_initrd_addr(dram_base, *image_addr),
>>> -                              &initrd_addr, &initrd_size);
>>> +     max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
>>> +     status = efi_load_initrd_devpath(&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, ULONG_MAX, max_addr,
>>> +                                      &initrd_addr, &initrd_size);
>>> +             if (status == EFI_SUCCESS)
>>> +                     pr_efi("Loaded initrd from command line option\n");
>>> +     }

As already mentioned in another mail: If the initrd intended to be
loaded by the the EFI_LOAD_FILE2_PROTOCOL, you fall back to the old
behavior. So the security is not increased by this patch.

>>>        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 8e60a39df3c5..eaf45ea749b3 100644
>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>> @@ -323,3 +323,68 @@ void efi_char16_printk(efi_char16_t *str)
>>>        efi_call_proto(efi_table_attr(efi_system_table(), con_out),
>>>                       output_string, str);
>>>    }
>>> +
>>> +static const struct {
>>> +     struct efi_vendor_dev_path      vendor;
>>> +     struct efi_generic_dev_path     end;
>>> +} __packed initrd_devpath = {
>>> +     {
>>> +             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_status_t efi_load_initrd_devpath(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_devpath;
>>> +     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;
>>
>> You require here that there is a handle exposing the device path
>> protocol with the initrd specific device path. On the same handle the
>> EFI_LOAD_FILE2_PROTOCOL must be installed which will load the initrd
>> file when called with the same device path.
>>
>
> Exactly.
>
>> An alternative implementation would simple loop over all instances of
>> the EFI_LOAD_FILE2_PROTOCOL and try to load the initrd.
>>
>
> How do I distinguish the initrd from other EFI_LOAD_FILE2_PROTOCOL
> instances? For instance, PCI option ROMs are also exposed as
> EFI_LOAD_FILE2_PROTOCOL.

These would respond with EFI_NOT_FOUND. The advantage of looping over
all instances could be that the same handle could be used to provide
multiple files. But how this is implemented is the least of my worries.

Best regards

Heinrich

>
>> It would be worthwhile to describe the requirements on the
>> implementation of the EFI_LOAD_FILE2_PROTOCOL in a code comment and in
>> the documentation.
>>
>> Unfortunately the documentation of UEFI has been duplicated into two files:
>>
>> Documentation/arm/uefi.rst
>> Documentation/x86/x86_64/uefi.rst
>>
>
> Yes, that is a good point. I will work on factoring out the generic
> parts and share them.
>
>
> Thanks for the review,
> Ard.
>
>
>
>>
>>> +
>>> +     initrd_size = 0;
>>> +     status = efi_call_proto(lf2, load_file,
>>> +                             (efi_device_path_protocol_t *)&initrd_devpath,
>>> +                             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,
>>> +                             (efi_device_path_protocol_t *)&initrd_devpath,
>>> +                             false, &initrd_size, (void *)initrd_addr);
>>> +     if (status != EFI_SUCCESS) {
>>> +             efi_free(initrd_size, initrd_addr);
>>> +             return status;
>>> +     }
>>> +
>>> +     *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 99e93fd76ec5..fbf9f9442eed 100644
>>> --- a/drivers/firmware/efi/libstub/efistub.h
>>> +++ b/drivers/firmware/efi/libstub/efistub.h
>>> @@ -566,6 +566,14 @@ union efi_load_file_protocol {
>>>        } mixed_mode;
>>>    };
>>>
>>> +struct efi_vendor_dev_path {
>>> +     u8              type;
>>> +     u8              sub_type;
>>> +     u16             length;
>>> +     efi_guid_t      vendorguid;
>>> +     u8              vendordata[];
>>> +} __packed;
>>> +
>>>    void efi_pci_disable_bridge_busmaster(void);
>>>
>>>    typedef efi_status_t (*efi_exit_boot_map_processing)(
>>> @@ -651,4 +659,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
>>>                             unsigned long *load_addr,
>>>                             unsigned long *load_size);
>>>
>>> +efi_status_t efi_load_initrd_devpath(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 f3e2ff31b624..7f38f95676dd 100644
>>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>>> @@ -419,9 +419,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>>>        if (status != EFI_SUCCESS)
>>>                goto fail2;
>>>
>>> -     status = efi_load_initrd(image, hdr->initrd_addr_max,
>>> -                              above4g ? ULONG_MAX : hdr->initrd_addr_max,
>>> -                              &ramdisk_addr, &ramdisk_size);
>>> +     /*
>>> +      * The initrd loaded from the Linux initrd vendor device
>>> +      * path should take precedence, as we don't want the
>>> +      * [unverified] command line to override the initrd
>>> +      * supplied by the [potentially verified] firmware.
>>> +      */
>>> +     status = efi_load_initrd_devpath(&ramdisk_addr, &ramdisk_size,
>>> +                                      above4g ? ULONG_MAX
>>> +                                              : hdr->initrd_addr_max);
>>> +     if (status == EFI_NOT_FOUND)
>>> +             status = efi_load_initrd(image, hdr->initrd_addr_max,
>>> +                                      above4g ? ULONG_MAX
>>> +                                              : hdr->initrd_addr_max,
>>> +                                      &ramdisk_addr, &ramdisk_size);
>>>        if (status != EFI_SUCCESS)
>>>                goto fail2;
>>>        hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
>>> @@ -732,6 +743,25 @@ struct boot_params *efi_main(efi_handle_t handle,
>>>                         ((u64)boot_params->ext_cmd_line_ptr << 32));
>>>        efi_parse_options((char *)cmdline_paddr);
>>>
>>> +     if (!hdr->ramdisk_size && !boot_params->ext_ramdisk_size) {
>>> +             unsigned long max = hdr->initrd_addr_max;
>>> +             unsigned long addr, size;
>>> +
>>> +             if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
>>> +                     max = ULONG_MAX;
>>> +
>>> +             status = efi_load_initrd_devpath(&addr, &size, max);
>>> +             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_devpath() 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 9ccf313fe9de..75c83c322c40 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)
>>>
>>

Ard Biesheuvel Feb. 7, 2020, 12:21 a.m. UTC | #8

On Fri, 7 Feb 2020 at 00:01, Heinrich Schuchardt <[e-mail]> wrote:
>
> On 2/6/20 11:35 PM, Ard Biesheuvel wrote:
> > On Thu, 6 Feb 2020 at 18:26, Heinrich Schuchardt <[e-mail]> wrote:
> >>
> >> On 2/6/20 3:03 PM, 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
> >>
> >> Hello Ard,
> >>
> >> "same file system" is not a limitation of using a command line
> >> parameter. Any device path can be passed as a string.
> >>
> >
> > What do you mean? The current implementation opens the volume via the
> > loaded_image_info struct, and finds the file based on its filename,
> > not on a device path. So how can it access files on other file
> > systems?
> >
>
> I talked aobut the information can be transmitted in a parameter not
> about the restrictions the current Linux command line implementation has.
>
> If you would pass a complete device path as parameter, you could use the
> UEFI API to find the device with block file system and use the
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL to load the file.
>

Yes, but that still requires the bootloader to expose the file system
via EFI protocols, and it requires some degree of trust in the
contents of the command line. The command line in Linux is a bit hairy
in any case, but allowing it to override the very first thing that
gets executed is something we should try to avoid.


> >
> >>> 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.
> >>
> >> Not relying on the command line for finding the initrd image does not
> >> secure the integrity and the validity of the initrd image.
> >>
> >
> > It does. It ensures that [signed] bootloader code is in charge of
> > providing the initrd at the point during the boot where the kernel is
> > ready to consume this data.
> >
> >> A signature on the initrd image could solve the security problem you
> >> describe. It would not require non-Linux software to be changed, and
> >> would provide much better security.
> >>
> >
> > A signed initrd would be useful, too, but it doesn't fix the whole problem.
> >
> > Linux does not support signed initrds today, and this feature permits
> > a firmware implementation to do something very similar, i.e., it
> > permits the bootloader to perform the verification as it is passed to
> > the kernel.
>
> One source of changed initrds is update-initramfs called in an operating
> system update process.
>
> How shall a bootloader verify the new initrd if we do not define what a
> signed initrd looks like?
>

Initrd signing is probably not feasible for this reason. But measuring
it, and using PCR value prediction to reseal keys when they change is
something we should be able to do in this context.

For that reason, it would be preferable for the command line not to
have control over whether or not an initrd gets loaded, and which.

> Would a bootloader be in a better position to verify an intird then
> Linux which already has code for file verification used when loading
> modules?
>

The bootloader already has code for file verification when loading the
kernel, so I fail to see your point here.

> >
> >
> >>>
> >>> 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.
> >>
> >> A firmware implementing the UEFI standard will not be aware of any
> >> initrd image as such an object does not exist in the standard. It was a
> >> wise decision that the UEFI standard is operating system agnostic
> >> (accomodating BSD, Linux, Windows, etc.). So the firmware (EDK2, U-Boot,
> >> etc.) seems to be out of scope for providing a Linux specific
> >> EFI_LOAD_FILE2_PROTOCOL.
> >>
> >
> > You know we are currently patching bootparams struct and DTs to
> > provide the initrd information, right? And we have code that knows
> > about low/high memory limits, alignment, etc, that is different per
> > architecture.
> >
> >> When booting via GRUB it will be GRUB knowing which initrd to load.
> >>
> >
> > Exactly, which is why GRUB will implement this protocol. That way, it
> > does not have to touch the DT at all, or create a bootparams struct
> > from setup data and inspect the various flags about placement,
> > alignment, preferred addresses, etc.
> >
> >> Please, indicate which software you expect to expose the initrd related
> >> EFI_LOAD_FILE2_PROTOCOL.
> >>
> >
> > The primary use case is GRUB and other intermediate loaders, since it
> > would remove any need for these components to know any such details.
> > My aim is to make the next architecture that gets added to GRUB for
> > EFI boot 100% generic.
> >
> >> Using an UEFI variable for passing the initrd device path would be a
> >> leaner solution on the bootloader side than requiring an extra
> >> EFI_LOAD_FILE2_PROTOCOL implementation.
> >>
> >
> > This would also require kernel changes, since we don't currently load
> > initrds from arbitrary device paths. The EFI_FILE_PROTOCOL is much
> > more complicated than needed, and it doesn't work well with mixed
> > mode. It also requires GRUB to expose the filesystem it loads the
> > initrd from via EFI protocols, which is currently unnecessary and
> > therefore not implemented.
>
> This means you move the complexity of EFI_FILE_PROTOCOL from Linux to GRUB.
>

No. I am not interested in EFI_FILE_PROTOCOL, only in LoadFile2, which
is a single method that needs to be implemented.

> I would not have a problem if this would only touch GRUB. But if listen
> to Ilias we are replacing one implementation in Linux by one in GRUB and
> one in U-Boot and one in EDK2 and one in any other firmware.
>

If u-boot will be used to boot RISC-V in EFI mode without GRUB, then I
expect that we will need an implementation of this in u-boot.

> >
> > Also, using an EFI variable defeats the purpose. The whole point of
> > this is making it more likely that the kernel loaded the initrd that
> > the bootloader or firmware intended it to load, and having a piece of
> > simple [signed] code that implements this is the easiest way to
> > achieve that.
>
> At least on my Debian system it is the operating system creating initrd
> and defining which initrd matches which kernel. GRUB simply assumes that
> files ending on the same version number match. Therefore I would say
> Linux hopes that GRUB loads what Linux intended.
>
> The chain of trust would not be broken if the kernel were responsible
> for loading the initrd and for checking if it matches the kernel. Linux
> already does this for the kernel modules in initrd.
>

We can still sign the initrd and Linux can verify the signature. What
I am after is an interface that does not require the initrd to
originate from a EFI file system protocol, and which doesn't require
the loaded initrd to sit in memory for an unspecified amount of time
and its information passed via DT properties or bootparams structs.

So invoking EFI_FILE_PROTOCOL directly is not going to work,
regardless of whether we get the devicepath from the command line or
from a EFI variable.

> >
> > For u-boot, it should be trivial to implement a simple LoadFile2
> > protocol wrapper around EFI_FILE_PROTOCOL that can be installed on a
> > handle that also carries EFI_FILE_PROTOCOL.
> >
>
> A U-Boot implementation of the EFI_LOAD_FILE2_PROTOCOL would need a
> device path variable to find the block device and to open the
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL before accessing the file.
>
> Linux would not be needing more lines and we would not repeat the same
> code in GRUB, U-Boot, EDK2, etc.
>
> As said Linux updates the initrd often. If that file is not signed by
> Linux in a well defined way, do not expect any security at all.
>

It is not only about security. The primary goal is to remove the need
for arch specific knowledge in the firmware about DT, bootparams and
initrd allocation policies without being forced to load the initrd
from a filesystem that is exposed via a EFI protocol.

> If Linux does not tell which kernel matches which initrd, how should
> U-Boot know? - How do you plan to transfer this piece of information?
>

How does u-boot know today? If you can tell it which initrd to load
into memory, why is it difficult to put that same data into a buffer
in response to a protocol invocation?


> >>>
> >>> Signed-off-by: Ard Biesheuvel <[e-mail]>
> >>> ---
> >>>    drivers/firmware/efi/libstub/arm-stub.c        | 16 +++--
> >>>    drivers/firmware/efi/libstub/efi-stub-helper.c | 65 ++++++++++++++++++++
> >>>    drivers/firmware/efi/libstub/efistub.h         | 12 ++++
> >>>    drivers/firmware/efi/libstub/x86-stub.c        | 36 ++++++++++-
> >>>    include/linux/efi.h                            |  1 +
> >>>    5 files changed, 123 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> >>> index c7b091f50e55..1db943c1ba2b 100644
> >>> --- a/drivers/firmware/efi/libstub/arm-stub.c
> >>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> >>> @@ -157,6 +157,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;
> >>>
> >>> @@ -255,11 +256,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, ULONG_MAX,
> >>> -                              efi_get_max_initrd_addr(dram_base, *image_addr),
> >>> -                              &initrd_addr, &initrd_size);
> >>> +     max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
> >>> +     status = efi_load_initrd_devpath(&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, ULONG_MAX, max_addr,
> >>> +                                      &initrd_addr, &initrd_size);
> >>> +             if (status == EFI_SUCCESS)
> >>> +                     pr_efi("Loaded initrd from command line option\n");
> >>> +     }
>
> As already mentioned in another mail: If the initrd intended to be
> loaded by the the EFI_LOAD_FILE2_PROTOCOL, you fall back to the old
> behavior. So the security is not increased by this patch.
>

If the bootloader is signed, it is justified to place a higher degree
of trust in the code we run than in the [unsigned] data we pass as the
command line.

> >>>        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 8e60a39df3c5..eaf45ea749b3 100644
> >>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> >>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> >>> @@ -323,3 +323,68 @@ void efi_char16_printk(efi_char16_t *str)
> >>>        efi_call_proto(efi_table_attr(efi_system_table(), con_out),
> >>>                       output_string, str);
> >>>    }
> >>> +
> >>> +static const struct {
> >>> +     struct efi_vendor_dev_path      vendor;
> >>> +     struct efi_generic_dev_path     end;
> >>> +} __packed initrd_devpath = {
> >>> +     {
> >>> +             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_status_t efi_load_initrd_devpath(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_devpath;
> >>> +     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;
> >>
> >> You require here that there is a handle exposing the device path
> >> protocol with the initrd specific device path. On the same handle the
> >> EFI_LOAD_FILE2_PROTOCOL must be installed which will load the initrd
> >> file when called with the same device path.
> >>
> >
> > Exactly.
> >
> >> An alternative implementation would simple loop over all instances of
> >> the EFI_LOAD_FILE2_PROTOCOL and try to load the initrd.
> >>
> >
> > How do I distinguish the initrd from other EFI_LOAD_FILE2_PROTOCOL
> > instances? For instance, PCI option ROMs are also exposed as
> > EFI_LOAD_FILE2_PROTOCOL.
>
> These would respond with EFI_NOT_FOUND. The advantage of looping over
> all instances could be that the same handle could be used to provide
> multiple files. But how this is implemented is the least of my worries.
>
> Best regards
>
> Heinrich
>
> >
> >> It would be worthwhile to describe the requirements on the
> >> implementation of the EFI_LOAD_FILE2_PROTOCOL in a code comment and in
> >> the documentation.
> >>
> >> Unfortunately the documentation of UEFI has been duplicated into two files:
> >>
> >> Documentation/arm/uefi.rst
> >> Documentation/x86/x86_64/uefi.rst
> >>
> >
> > Yes, that is a good point. I will work on factoring out the generic
> > parts and share them.
> >
> >
> > Thanks for the review,
> > Ard.
> >
> >
> >
> >>
> >>> +
> >>> +     initrd_size = 0;
> >>> +     status = efi_call_proto(lf2, load_file,
> >>> +                             (efi_device_path_protocol_t *)&initrd_devpath,
> >>> +                             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,
> >>> +                             (efi_device_path_protocol_t *)&initrd_devpath,
> >>> +                             false, &initrd_size, (void *)initrd_addr);
> >>> +     if (status != EFI_SUCCESS) {
> >>> +             efi_free(initrd_size, initrd_addr);
> >>> +             return status;
> >>> +     }
> >>> +
> >>> +     *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 99e93fd76ec5..fbf9f9442eed 100644
> >>> --- a/drivers/firmware/efi/libstub/efistub.h
> >>> +++ b/drivers/firmware/efi/libstub/efistub.h
> >>> @@ -566,6 +566,14 @@ union efi_load_file_protocol {
> >>>        } mixed_mode;
> >>>    };
> >>>
> >>> +struct efi_vendor_dev_path {
> >>> +     u8              type;
> >>> +     u8              sub_type;
> >>> +     u16             length;
> >>> +     efi_guid_t      vendorguid;
> >>> +     u8              vendordata[];
> >>> +} __packed;
> >>> +
> >>>    void efi_pci_disable_bridge_busmaster(void);
> >>>
> >>>    typedef efi_status_t (*efi_exit_boot_map_processing)(
> >>> @@ -651,4 +659,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
> >>>                             unsigned long *load_addr,
> >>>                             unsigned long *load_size);
> >>>
> >>> +efi_status_t efi_load_initrd_devpath(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 f3e2ff31b624..7f38f95676dd 100644
> >>> --- a/drivers/firmware/efi/libstub/x86-stub.c
> >>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> >>> @@ -419,9 +419,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> >>>        if (status != EFI_SUCCESS)
> >>>                goto fail2;
> >>>
> >>> -     status = efi_load_initrd(image, hdr->initrd_addr_max,
> >>> -                              above4g ? ULONG_MAX : hdr->initrd_addr_max,
> >>> -                              &ramdisk_addr, &ramdisk_size);
> >>> +     /*
> >>> +      * The initrd loaded from the Linux initrd vendor device
> >>> +      * path should take precedence, as we don't want the
> >>> +      * [unverified] command line to override the initrd
> >>> +      * supplied by the [potentially verified] firmware.
> >>> +      */
> >>> +     status = efi_load_initrd_devpath(&ramdisk_addr, &ramdisk_size,
> >>> +                                      above4g ? ULONG_MAX
> >>> +                                              : hdr->initrd_addr_max);
> >>> +     if (status == EFI_NOT_FOUND)
> >>> +             status = efi_load_initrd(image, hdr->initrd_addr_max,
> >>> +                                      above4g ? ULONG_MAX
> >>> +                                              : hdr->initrd_addr_max,
> >>> +                                      &ramdisk_addr, &ramdisk_size);
> >>>        if (status != EFI_SUCCESS)
> >>>                goto fail2;
> >>>        hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
> >>> @@ -732,6 +743,25 @@ struct boot_params *efi_main(efi_handle_t handle,
> >>>                         ((u64)boot_params->ext_cmd_line_ptr << 32));
> >>>        efi_parse_options((char *)cmdline_paddr);
> >>>
> >>> +     if (!hdr->ramdisk_size && !boot_params->ext_ramdisk_size) {
> >>> +             unsigned long max = hdr->initrd_addr_max;
> >>> +             unsigned long addr, size;
> >>> +
> >>> +             if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
> >>> +                     max = ULONG_MAX;
> >>> +
> >>> +             status = efi_load_initrd_devpath(&addr, &size, max);
> >>> +             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_devpath() 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 9ccf313fe9de..75c83c322c40 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)
> >>>
> >>
>

Heinrich Schuchardt Feb. 7, 2020, 12:57 a.m. UTC | #9

On 2/7/20 1:21 AM, Ard Biesheuvel wrote:
> On Fri, 7 Feb 2020 at 00:01, Heinrich Schuchardt <[e-mail]> wrote:
>>
>> On 2/6/20 11:35 PM, Ard Biesheuvel wrote:
>>> On Thu, 6 Feb 2020 at 18:26, Heinrich Schuchardt <[e-mail]> wrote:
>>>>
>>>> On 2/6/20 3:03 PM, 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
>>>>
>>>> Hello Ard,
>>>>
>>>> "same file system" is not a limitation of using a command line
>>>> parameter. Any device path can be passed as a string.
>>>>
>>>
>>> What do you mean? The current implementation opens the volume via the
>>> loaded_image_info struct, and finds the file based on its filename,
>>> not on a device path. So how can it access files on other file
>>> systems?
>>>
>>
>> I talked aobut the information can be transmitted in a parameter not
>> about the restrictions the current Linux command line implementation has.
>>
>> If you would pass a complete device path as parameter, you could use the
>> UEFI API to find the device with block file system and use the
>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL to load the file.
>>
>
> Yes, but that still requires the bootloader to expose the file system
> via EFI protocols, and it requires some degree of trust in the
> contents of the command line. The command line in Linux is a bit hairy
> in any case, but allowing it to override the very first thing that
> gets executed is something we should try to avoid.
>
>
>>>
>>>>> 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.
>>>>
>>>> Not relying on the command line for finding the initrd image does not
>>>> secure the integrity and the validity of the initrd image.
>>>>
>>>
>>> It does. It ensures that [signed] bootloader code is in charge of
>>> providing the initrd at the point during the boot where the kernel is
>>> ready to consume this data.
>>>
>>>> A signature on the initrd image could solve the security problem you
>>>> describe. It would not require non-Linux software to be changed, and
>>>> would provide much better security.
>>>>
>>>
>>> A signed initrd would be useful, too, but it doesn't fix the whole problem.
>>>
>>> Linux does not support signed initrds today, and this feature permits
>>> a firmware implementation to do something very similar, i.e., it
>>> permits the bootloader to perform the verification as it is passed to
>>> the kernel.
>>
>> One source of changed initrds is update-initramfs called in an operating
>> system update process.
>>
>> How shall a bootloader verify the new initrd if we do not define what a
>> signed initrd looks like?
>>
>
> Initrd signing is probably not feasible for this reason. But measuring
> it, and using PCR value prediction to reseal keys when they change is
> something we should be able to do in this context.
>
> For that reason, it would be preferable for the command line not to
> have control over whether or not an initrd gets loaded, and which.
>
>> Would a bootloader be in a better position to verify an intird then
>> Linux which already has code for file verification used when loading
>> modules?
>>
>
> The bootloader already has code for file verification when loading the
> kernel, so I fail to see your point here.
>
>>>
>>>
>>>>>
>>>>> 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.
>>>>
>>>> A firmware implementing the UEFI standard will not be aware of any
>>>> initrd image as such an object does not exist in the standard. It was a
>>>> wise decision that the UEFI standard is operating system agnostic
>>>> (accomodating BSD, Linux, Windows, etc.). So the firmware (EDK2, U-Boot,
>>>> etc.) seems to be out of scope for providing a Linux specific
>>>> EFI_LOAD_FILE2_PROTOCOL.
>>>>
>>>
>>> You know we are currently patching bootparams struct and DTs to
>>> provide the initrd information, right? And we have code that knows
>>> about low/high memory limits, alignment, etc, that is different per
>>> architecture.
>>>
>>>> When booting via GRUB it will be GRUB knowing which initrd to load.
>>>>
>>>
>>> Exactly, which is why GRUB will implement this protocol. That way, it
>>> does not have to touch the DT at all, or create a bootparams struct
>>> from setup data and inspect the various flags about placement,
>>> alignment, preferred addresses, etc.
>>>
>>>> Please, indicate which software you expect to expose the initrd related
>>>> EFI_LOAD_FILE2_PROTOCOL.
>>>>
>>>
>>> The primary use case is GRUB and other intermediate loaders, since it
>>> would remove any need for these components to know any such details.
>>> My aim is to make the next architecture that gets added to GRUB for
>>> EFI boot 100% generic.
>>>
>>>> Using an UEFI variable for passing the initrd device path would be a
>>>> leaner solution on the bootloader side than requiring an extra
>>>> EFI_LOAD_FILE2_PROTOCOL implementation.
>>>>
>>>
>>> This would also require kernel changes, since we don't currently load
>>> initrds from arbitrary device paths. The EFI_FILE_PROTOCOL is much
>>> more complicated than needed, and it doesn't work well with mixed
>>> mode. It also requires GRUB to expose the filesystem it loads the
>>> initrd from via EFI protocols, which is currently unnecessary and
>>> therefore not implemented.
>>
>> This means you move the complexity of EFI_FILE_PROTOCOL from Linux to GRUB.
>>
>
> No. I am not interested in EFI_FILE_PROTOCOL, only in LoadFile2, which
> is a single method that needs to be implemented.

I said you move complexity because GRUB will need to use the
EFI_FILE_PROTOCOL to do the job that you do not want to do in Linux.

>
>> I would not have a problem if this would only touch GRUB. But if listen
>> to Ilias we are replacing one implementation in Linux by one in GRUB and
>> one in U-Boot and one in EDK2 and one in any other firmware.
>>
>
> If u-boot will be used to boot RISC-V in EFI mode without GRUB, then I
> expect that we will need an implementation of this in u-boot.

What sets RISC-V apart? GRUB for RISC-V is available.

>
>>>
>>> Also, using an EFI variable defeats the purpose. The whole point of
>>> this is making it more likely that the kernel loaded the initrd that
>>> the bootloader or firmware intended it to load, and having a piece of
>>> simple [signed] code that implements this is the easiest way to
>>> achieve that.
>>
>> At least on my Debian system it is the operating system creating initrd
>> and defining which initrd matches which kernel. GRUB simply assumes that
>> files ending on the same version number match. Therefore I would say
>> Linux hopes that GRUB loads what Linux intended.
>>
>> The chain of trust would not be broken if the kernel were responsible
>> for loading the initrd and for checking if it matches the kernel. Linux
>> already does this for the kernel modules in initrd.
>>
>
> We can still sign the initrd and Linux can verify the signature. What
> I am after is an interface that does not require the initrd to
> originate from a EFI file system protocol, and which doesn't require
> the loaded initrd to sit in memory for an unspecified amount of time
> and its information passed via DT properties or bootparams structs.
>
> So invoking EFI_FILE_PROTOCOL directly is not going to work,
> regardless of whether we get the devicepath from the command line or
> from a EFI variable.

What do you mean by "is not going to work"?

With the device path you can find the handle implementing the
EFI_SIMPLE_FIL_SYSTEM_PROTOCOL.

>
>>>
>>> For u-boot, it should be trivial to implement a simple LoadFile2
>>> protocol wrapper around EFI_FILE_PROTOCOL that can be installed on a
>>> handle that also carries EFI_FILE_PROTOCOL.
>>>
>>
>> A U-Boot implementation of the EFI_LOAD_FILE2_PROTOCOL would need a
>> device path variable to find the block device and to open the
>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL before accessing the file.
>>
>> Linux would not be needing more lines and we would not repeat the same
>> code in GRUB, U-Boot, EDK2, etc.
>>
>> As said Linux updates the initrd often. If that file is not signed by
>> Linux in a well defined way, do not expect any security at all.
>>
>
> It is not only about security. The primary goal is to remove the need
> for arch specific knowledge in the firmware about DT, bootparams and
> initrd allocation policies without being forced to load the initrd
> from a filesystem that is exposed via a EFI protocol.

Where are device-trees touched by this patch?

When booting via UEFI there is no need for knowledge of initrd
allocation policies in U-Boot because up to now Linux or GRUB or iPXE
load initrd.

Furthermore I need no knowledge of bootparams in U-Boot once we properly
we support UEFI variables at runtime because grub-update will pass the
command line in one of the Bootxxxx UEFI variables.

But most importantly I do not have to implement anything Linux specific
in U-Boot for booting via UEFI up to now.

>
>> If Linux does not tell which kernel matches which initrd, how should
>> U-Boot know? - How do you plan to transfer this piece of information?
>>
>
> How does u-boot know today? If you can tell it which initrd to load
> into memory, why is it difficult to put that same data into a buffer
> in response to a protocol invocation?
>

When booting via UEFI U-Boot never touches initrd.

When booting via GRUB, GRUB will know what to load and take care.

When calling Linux directly you have to provide a script boot.scr to set
the bootargs environment variable that will be passed to Linux via the
loaded file image protocol. And you will need a script in an
update-initramfs hook to update boot.scr everytime that you create a new
initrd.

Or you define a UEFI variable Bootxxxx and the bootmanager in U-Boot
will use that value. Unfortunately runtime variable support is still
missing so you currently cannot use efibootmgr to update Bootxxxx in U-Boot.

Essentially for UEFI booting Linux is always the source of the kernel
command line if you suppress editing in U-Boot and GRUB.

>
>>>>>
>>>>> Signed-off-by: Ard Biesheuvel <[e-mail]>
>>>>> ---
>>>>>     drivers/firmware/efi/libstub/arm-stub.c        | 16 +++--
>>>>>     drivers/firmware/efi/libstub/efi-stub-helper.c | 65 ++++++++++++++++++++
>>>>>     drivers/firmware/efi/libstub/efistub.h         | 12 ++++
>>>>>     drivers/firmware/efi/libstub/x86-stub.c        | 36 ++++++++++-
>>>>>     include/linux/efi.h                            |  1 +
>>>>>     5 files changed, 123 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>>>>> index c7b091f50e55..1db943c1ba2b 100644
>>>>> --- a/drivers/firmware/efi/libstub/arm-stub.c
>>>>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>>>>> @@ -157,6 +157,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;
>>>>>
>>>>> @@ -255,11 +256,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, ULONG_MAX,
>>>>> -                              efi_get_max_initrd_addr(dram_base, *image_addr),
>>>>> -                              &initrd_addr, &initrd_size);
>>>>> +     max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
>>>>> +     status = efi_load_initrd_devpath(&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, ULONG_MAX, max_addr,
>>>>> +                                      &initrd_addr, &initrd_size);
>>>>> +             if (status == EFI_SUCCESS)
>>>>> +                     pr_efi("Loaded initrd from command line option\n");
>>>>> +     }
>>
>> As already mentioned in another mail: If the initrd intended to be
>> loaded by the the EFI_LOAD_FILE2_PROTOCOL, you fall back to the old
>> behavior. So the security is not increased by this patch.
>>
>
> If the bootloader is signed, it is justified to place a higher degree
> of trust in the code we run than in the [unsigned] data we pass as the
> command line.

Your patch claims to fend off a specific threat scenario: A user puts an
untrusted initrd on the disk and references it in the Linux command line.

If he is able to do so with your current bootloader (signed or not
signed), he most probably will also be able to delete a good initrd from
the filesystem and thus force your code into the unsafe path.

That is why I say that with the current fallback logic this patch
achieves no increase in security. Of cause you could remove the fallback
logic. But in this case your Linux will not boot with any legacy
bootloader or firmware.

Best regards

Heinrich

>
>>>>>         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 8e60a39df3c5..eaf45ea749b3 100644
>>>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>>>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>>>> @@ -323,3 +323,68 @@ void efi_char16_printk(efi_char16_t *str)
>>>>>         efi_call_proto(efi_table_attr(efi_system_table(), con_out),
>>>>>                        output_string, str);
>>>>>     }
>>>>> +
>>>>> +static const struct {
>>>>> +     struct efi_vendor_dev_path      vendor;
>>>>> +     struct efi_generic_dev_path     end;
>>>>> +} __packed initrd_devpath = {
>>>>> +     {
>>>>> +             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_status_t efi_load_initrd_devpath(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_devpath;
>>>>> +     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;
>>>>
>>>> You require here that there is a handle exposing the device path
>>>> protocol with the initrd specific device path. On the same handle the
>>>> EFI_LOAD_FILE2_PROTOCOL must be installed which will load the initrd
>>>> file when called with the same device path.
>>>>
>>>
>>> Exactly.
>>>
>>>> An alternative implementation would simple loop over all instances of
>>>> the EFI_LOAD_FILE2_PROTOCOL and try to load the initrd.
>>>>
>>>
>>> How do I distinguish the initrd from other EFI_LOAD_FILE2_PROTOCOL
>>> instances? For instance, PCI option ROMs are also exposed as
>>> EFI_LOAD_FILE2_PROTOCOL.
>>
>> These would respond with EFI_NOT_FOUND. The advantage of looping over
>> all instances could be that the same handle could be used to provide
>> multiple files. But how this is implemented is the least of my worries.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>> It would be worthwhile to describe the requirements on the
>>>> implementation of the EFI_LOAD_FILE2_PROTOCOL in a code comment and in
>>>> the documentation.
>>>>
>>>> Unfortunately the documentation of UEFI has been duplicated into two files:
>>>>
>>>> Documentation/arm/uefi.rst
>>>> Documentation/x86/x86_64/uefi.rst
>>>>
>>>
>>> Yes, that is a good point. I will work on factoring out the generic
>>> parts and share them.
>>>
>>>
>>> Thanks for the review,
>>> Ard.
>>>
>>>
>>>
>>>>
>>>>> +
>>>>> +     initrd_size = 0;
>>>>> +     status = efi_call_proto(lf2, load_file,
>>>>> +                             (efi_device_path_protocol_t *)&initrd_devpath,
>>>>> +                             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,
>>>>> +                             (efi_device_path_protocol_t *)&initrd_devpath,
>>>>> +                             false, &initrd_size, (void *)initrd_addr);
>>>>> +     if (status != EFI_SUCCESS) {
>>>>> +             efi_free(initrd_size, initrd_addr);
>>>>> +             return status;
>>>>> +     }
>>>>> +
>>>>> +     *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 99e93fd76ec5..fbf9f9442eed 100644
>>>>> --- a/drivers/firmware/efi/libstub/efistub.h
>>>>> +++ b/drivers/firmware/efi/libstub/efistub.h
>>>>> @@ -566,6 +566,14 @@ union efi_load_file_protocol {
>>>>>         } mixed_mode;
>>>>>     };
>>>>>
>>>>> +struct efi_vendor_dev_path {
>>>>> +     u8              type;
>>>>> +     u8              sub_type;
>>>>> +     u16             length;
>>>>> +     efi_guid_t      vendorguid;
>>>>> +     u8              vendordata[];
>>>>> +} __packed;
>>>>> +
>>>>>     void efi_pci_disable_bridge_busmaster(void);
>>>>>
>>>>>     typedef efi_status_t (*efi_exit_boot_map_processing)(
>>>>> @@ -651,4 +659,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
>>>>>                              unsigned long *load_addr,
>>>>>                              unsigned long *load_size);
>>>>>
>>>>> +efi_status_t efi_load_initrd_devpath(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 f3e2ff31b624..7f38f95676dd 100644
>>>>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>>>>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>>>>> @@ -419,9 +419,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>>>>>         if (status != EFI_SUCCESS)
>>>>>                 goto fail2;
>>>>>
>>>>> -     status = efi_load_initrd(image, hdr->initrd_addr_max,
>>>>> -                              above4g ? ULONG_MAX : hdr->initrd_addr_max,
>>>>> -                              &ramdisk_addr, &ramdisk_size);
>>>>> +     /*
>>>>> +      * The initrd loaded from the Linux initrd vendor device
>>>>> +      * path should take precedence, as we don't want the
>>>>> +      * [unverified] command line to override the initrd
>>>>> +      * supplied by the [potentially verified] firmware.
>>>>> +      */
>>>>> +     status = efi_load_initrd_devpath(&ramdisk_addr, &ramdisk_size,
>>>>> +                                      above4g ? ULONG_MAX
>>>>> +                                              : hdr->initrd_addr_max);
>>>>> +     if (status == EFI_NOT_FOUND)
>>>>> +             status = efi_load_initrd(image, hdr->initrd_addr_max,
>>>>> +                                      above4g ? ULONG_MAX
>>>>> +                                              : hdr->initrd_addr_max,
>>>>> +                                      &ramdisk_addr, &ramdisk_size);
>>>>>         if (status != EFI_SUCCESS)
>>>>>                 goto fail2;
>>>>>         hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
>>>>> @@ -732,6 +743,25 @@ struct boot_params *efi_main(efi_handle_t handle,
>>>>>                          ((u64)boot_params->ext_cmd_line_ptr << 32));
>>>>>         efi_parse_options((char *)cmdline_paddr);
>>>>>
>>>>> +     if (!hdr->ramdisk_size && !boot_params->ext_ramdisk_size) {
>>>>> +             unsigned long max = hdr->initrd_addr_max;
>>>>> +             unsigned long addr, size;
>>>>> +
>>>>> +             if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
>>>>> +                     max = ULONG_MAX;
>>>>> +
>>>>> +             status = efi_load_initrd_devpath(&addr, &size, max);
>>>>> +             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_devpath() 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 9ccf313fe9de..75c83c322c40 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)
>>>>>
>>>>
>>

Ilias Apalodimas Feb. 7, 2020, 7:35 a.m. UTC | #10

Hi Heinrich

[...]
> > > 
> > > If you don't have an environment or boot script how would
> > > update-initramfs set the path of the initrd when it is updated?
> > 
> > The path isn't hardcoded in any code here is it?
> > This specifies a way for the linux stub to load the actual file. It's pretty a
> > callback to the firmware. Were the firmware will find and how it will load it
> > eventually is implementation specific.
> 
> "Implementation specific" - This does not sound like anything you would
> want to have in mainline Linux, U-Boot, or EDK2.
> 

And it isn't. The *only* thing that's specific to the firmware itself, is
how/where to find the file. The whole handover mechanism after 
that is generic for everyone.

> > 
> > > 
> > > Using a UEFI variable seems to be the natural choice.
> > > 
> > 
> > You might as well use that to specify were you should load the file from.
> > The Loadfile2 (with the specified guid)  implementation of the firmware will
> > take care of that.
> > 
> 
> If we have a UEFI variable, the Linux kernel can use it to find the
> handle with the file system and load initrd via the
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
> 
> This way we stay within the existing UEFI specification and avoid
> anything "implementation specific" in the firmware.

Is this going to limit the directories we can place the file or not?

> 
> If you want extra security, Linux can use an authenticated variable.
> 
> > > > 
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > A firmware implementing the UEFI standard will not be aware of any
> > > > > initrd image as such an object does not exist in the standard. It was a
> > > > > wise decision that the UEFI standard is operating system agnostic
> > > > > (accomodating BSD, Linux, Windows, etc.). So the firmware (EDK2, U-Boot,
> > > > > etc.) seems to be out of scope for providing a Linux specific
> > > > > EFI_LOAD_FILE2_PROTOCOL.
> > > > > 
> > > > > When booting via GRUB it will be GRUB knowing which initrd to load.
> > > > 
> > > > What about booting the kernel directly?
> > > > 
> > > > > 
> > > > > Please, indicate which software you expect to expose the initrd related
> > > > > EFI_LOAD_FILE2_PROTOCOL.
> > > > 
> > > > I have an implementation for this on U-Boot which works. The file and device are
> > > > hardcoded at the moment, but the rest of the functionality works fine. I'll
> > > > share it with you once I clean it up a bit.
> > > 
> > > Using a UEFI variable for passing the intird device path to Linux does
> > > not require any change in U-Boot and is compatible with the UEFI
> > > implementations of existing hardware like the laptop on which I am
> > > writing this email.
> > 
> > This still has the same issues we have now, uefi variable, kernel command line
> > or whatever, it won't be common across architectures.
> 
> This would be a bad design choice by Linux. I cannot see why a UEFI
> variable should not be interpreted in a consistent way inside Linux to
> load a file via the EFI_SIMPLE_FILE_PROTOCOL.
> 
> > 
> > Thanks
> > /Ilias
> > > 
> > > Best regards
> > > 
> > > Heinrich
> > > 
> > > > 
> > > > > > -	status = efi_load_initrd(image, ULONG_MAX,
> > > > > > -				 efi_get_max_initrd_addr(dram_base, *image_addr),
> > > > > > -				 &initrd_addr, &initrd_size);
> > > > > > +	max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
> > > > > > +	status = efi_load_initrd_devpath(&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, ULONG_MAX, max_addr,
> > > > > > +					 &initrd_addr, &initrd_size);
> 
> If I delete the initrd that otherwise would be loaded by the
> EFI_LOAD_FILE2_PROTOCOL I end up with the old behavior. So where is the
> security gain provided by this patch?

If you delete the initrd, there is no initrd to load :)

Thanks
/Ilias

Ard Biesheuvel Feb. 7, 2020, 8:12 a.m. UTC | #11

On Fri, 7 Feb 2020 at 00:58, Heinrich Schuchardt <[e-mail]> wrote:
>
> On 2/7/20 1:21 AM, Ard Biesheuvel wrote:
> > On Fri, 7 Feb 2020 at 00:01, Heinrich Schuchardt <[e-mail]> wrote:
> >>
> >> On 2/6/20 11:35 PM, Ard Biesheuvel wrote:
> >>> On Thu, 6 Feb 2020 at 18:26, Heinrich Schuchardt <[e-mail]> wrote:
...
> >>>> Please, indicate which software you expect to expose the initrd related
> >>>> EFI_LOAD_FILE2_PROTOCOL.
> >>>>
> >>>
> >>> The primary use case is GRUB and other intermediate loaders, since it
> >>> would remove any need for these components to know any such details.
> >>> My aim is to make the next architecture that gets added to GRUB for
> >>> EFI boot 100% generic.
> >>>
> >>>> Using an UEFI variable for passing the initrd device path would be a
> >>>> leaner solution on the bootloader side than requiring an extra
> >>>> EFI_LOAD_FILE2_PROTOCOL implementation.
> >>>>
> >>>
> >>> This would also require kernel changes, since we don't currently load
> >>> initrds from arbitrary device paths. The EFI_FILE_PROTOCOL is much
> >>> more complicated than needed, and it doesn't work well with mixed
> >>> mode. It also requires GRUB to expose the filesystem it loads the
> >>> initrd from via EFI protocols, which is currently unnecessary and
> >>> therefore not implemented.
> >>
> >> This means you move the complexity of EFI_FILE_PROTOCOL from Linux to GRUB.
> >>
> >
> > No. I am not interested in EFI_FILE_PROTOCOL, only in LoadFile2, which
> > is a single method that needs to be implemented.
>
> I said you move complexity because GRUB will need to use the
> EFI_FILE_PROTOCOL to do the job that you do not want to do in Linux.
>
> >
> >> I would not have a problem if this would only touch GRUB. But if listen
> >> to Ilias we are replacing one implementation in Linux by one in GRUB and
> >> one in U-Boot and one in EDK2 and one in any other firmware.
> >>
> >
> > If u-boot will be used to boot RISC-V in EFI mode without GRUB, then I
> > expect that we will need an implementation of this in u-boot.
>
> What sets RISC-V apart? GRUB for RISC-V is available.
>

RISC-V EFI boot is not supported yet in upstream Linux.

> >
> >>>
> >>> Also, using an EFI variable defeats the purpose. The whole point of
> >>> this is making it more likely that the kernel loaded the initrd that
> >>> the bootloader or firmware intended it to load, and having a piece of
> >>> simple [signed] code that implements this is the easiest way to
> >>> achieve that.
> >>
> >> At least on my Debian system it is the operating system creating initrd
> >> and defining which initrd matches which kernel. GRUB simply assumes that
> >> files ending on the same version number match. Therefore I would say
> >> Linux hopes that GRUB loads what Linux intended.
> >>
> >> The chain of trust would not be broken if the kernel were responsible
> >> for loading the initrd and for checking if it matches the kernel. Linux
> >> already does this for the kernel modules in initrd.
> >>
> >
> > We can still sign the initrd and Linux can verify the signature. What
> > I am after is an interface that does not require the initrd to
> > originate from a EFI file system protocol, and which doesn't require
> > the loaded initrd to sit in memory for an unspecified amount of time
> > and its information passed via DT properties or bootparams structs.
> >
> > So invoking EFI_FILE_PROTOCOL directly is not going to work,
> > regardless of whether we get the devicepath from the command line or
> > from a EFI variable.
>
> What do you mean by "is not going to work"?
>
> With the device path you can find the handle implementing the
> EFI_SIMPLE_FIL_SYSTEM_PROTOCOL.
>
> >
> >>>
> >>> For u-boot, it should be trivial to implement a simple LoadFile2
> >>> protocol wrapper around EFI_FILE_PROTOCOL that can be installed on a
> >>> handle that also carries EFI_FILE_PROTOCOL.
> >>>
> >>
> >> A U-Boot implementation of the EFI_LOAD_FILE2_PROTOCOL would need a
> >> device path variable to find the block device and to open the
> >> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL before accessing the file.
> >>
> >> Linux would not be needing more lines and we would not repeat the same
> >> code in GRUB, U-Boot, EDK2, etc.
> >>
> >> As said Linux updates the initrd often. If that file is not signed by
> >> Linux in a well defined way, do not expect any security at all.
> >>
> >
> > It is not only about security. The primary goal is to remove the need
> > for arch specific knowledge in the firmware about DT, bootparams and
> > initrd allocation policies without being forced to load the initrd
> > from a filesystem that is exposed via a EFI protocol.
>
> Where are device-trees touched by this patch?
>
> When booting via UEFI there is no need for knowledge of initrd
> allocation policies in U-Boot because up to now Linux or GRUB or iPXE
> load initrd.
>
> Furthermore I need no knowledge of bootparams in U-Boot once we properly
> we support UEFI variables at runtime because grub-update will pass the
> command line in one of the Bootxxxx UEFI variables.
>
> But most importantly I do not have to implement anything Linux specific
> in U-Boot for booting via UEFI up to now.
>

Adding Linux specific stuff to u-boot is arguably more appropriate
than adding architecture specific stuff to EFI loaders that could
otherwise be entirely generic.

...
>
> Your patch claims to fend off a specific threat scenario: A user puts an
> untrusted initrd on the disk and references it in the Linux command line.
>
> If he is able to do so with your current bootloader (signed or not
> signed), he most probably will also be able to delete a good initrd from
> the filesystem and thus force your code into the unsafe path.
>
> That is why I say that with the current fallback logic this patch
> achieves no increase in security. Of cause you could remove the fallback
> logic. But in this case your Linux will not boot with any legacy
> bootloader or firmware.
>

If there is a better way to expose the initrd that
a) does not require the initrd to reside on a file system that is
accessible via EFI protocols, and
b) does not require the loader to know about arch specific policies
regarding the placement of the initrd in memory, and
c) does not leave a time window between the time that the initrd is
loaded/verified/measured by the firmware and the time that the kernel
gets handed the buffer

then I am happy to discuss it. This proposal is the best I could come
up with to achieve the above.

Laszlo Ersek Feb. 7, 2020, 9:48 a.m. UTC | #12

On 02/06/20 15:03, 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 <[e-mail]>
> ---
>  drivers/firmware/efi/libstub/arm-stub.c        | 16 +++--
>  drivers/firmware/efi/libstub/efi-stub-helper.c | 65 ++++++++++++++++++++
>  drivers/firmware/efi/libstub/efistub.h         | 12 ++++
>  drivers/firmware/efi/libstub/x86-stub.c        | 36 ++++++++++-
>  include/linux/efi.h                            |  1 +
>  5 files changed, 123 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index c7b091f50e55..1db943c1ba2b 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -157,6 +157,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;
>  
> @@ -255,11 +256,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, ULONG_MAX,
> -				 efi_get_max_initrd_addr(dram_base, *image_addr),
> -				 &initrd_addr, &initrd_size);
> +	max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
> +	status = efi_load_initrd_devpath(&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, ULONG_MAX, max_addr,
> +					 &initrd_addr, &initrd_size);

- So this seems to be fallback#1, for ARM, which looks good.

- Are you sure you only want to fall back to the old method on
EFI_NOT_FOUND? Wouldn't other return values from
efi_load_initrd_devpath() justify that too?

... After checking the boot services called in
efi_load_initrd_devpath(), this idea seems reasonable, but then I'd
suggest documenting the significance of returning EFI_NOT_FOUND near the
efi_load_initrd_devpath() function declaration, in "efistub.h".

> +		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 8e60a39df3c5..eaf45ea749b3 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -323,3 +323,68 @@ void efi_char16_printk(efi_char16_t *str)
>  	efi_call_proto(efi_table_attr(efi_system_table(), con_out),
>  		       output_string, str);
>  }
> +
> +static const struct {
> +	struct efi_vendor_dev_path	vendor;
> +	struct efi_generic_dev_path	end;
> +} __packed initrd_devpath = {
> +	{
> +		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_status_t efi_load_initrd_devpath(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_devpath;
> +	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;
> +
> +	initrd_size = 0;
> +	status = efi_call_proto(lf2, load_file,
> +				(efi_device_path_protocol_t *)&initrd_devpath,
> +				false, &initrd_size, NULL);

The second argument to EFI_LOAD_FILE2_PROTOCOL.LoadFile() is "FilePath",
specified as "The device specific path of the file to load". This means
it is supposed to be a (possibly empty) sequence of FILEPATH_DEVICE_PATH
nodes, terminated by and "End Entire Device Path" node. See

- 10.3.1 Generic Device Path Structures
- 10.3.5.4 File Path Media Device Path

in UEFI-2.8.

And "initrd_devpath" is not a device path like that; instead it's the
VenMedia device path that's installed on the handle that also carries
our LoadFile2 instance.

Now, I do see that this all theoretical here, as we don't expect the
LoadFile2 instance that we've found via our special
LINUX_EFI_INITRD_MEDIA_GUID VenMedia devpath to do *any* device-specific
filename / pathname parsing.

But in that case (i.e., given that the FilePath argument is totally
irrelevant), I think it's much clearer if we simply pass an empty device
path -- one that consists of a single "End Entire Device Path" node.

I've checked, and your ArmVirtQemu patch ignores the FilePath argument
too -- justifiedly so. I just think it's better to pass in a well-formed
device path, rather than NULL. Because, the FilePath parameter is not
marked OPTIONAL in the spec.

> +	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,
> +				(efi_device_path_protocol_t *)&initrd_devpath,
> +				false, &initrd_size, (void *)initrd_addr);

Same here.

> +	if (status != EFI_SUCCESS) {
> +		efi_free(initrd_size, initrd_addr);
> +		return status;
> +	}
> +
> +	*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 99e93fd76ec5..fbf9f9442eed 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -566,6 +566,14 @@ union efi_load_file_protocol {
>  	} mixed_mode;
>  };
>  
> +struct efi_vendor_dev_path {
> +	u8		type;
> +	u8		sub_type;
> +	u16		length;
> +	efi_guid_t	vendorguid;
> +	u8		vendordata[];
> +} __packed;
> +
>  void efi_pci_disable_bridge_busmaster(void);
>  
>  typedef efi_status_t (*efi_exit_boot_map_processing)(
> @@ -651,4 +659,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
>  			     unsigned long *load_addr,
>  			     unsigned long *load_size);
>  
> +efi_status_t efi_load_initrd_devpath(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 f3e2ff31b624..7f38f95676dd 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -419,9 +419,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
>  	if (status != EFI_SUCCESS)
>  		goto fail2;
>  
> -	status = efi_load_initrd(image, hdr->initrd_addr_max,
> -				 above4g ? ULONG_MAX : hdr->initrd_addr_max,
> -				 &ramdisk_addr, &ramdisk_size);
> +	/*
> +	 * The initrd loaded from the Linux initrd vendor device
> +	 * path should take precedence, as we don't want the
> +	 * [unverified] command line to override the initrd
> +	 * supplied by the [potentially verified] firmware.
> +	 */
> +	status = efi_load_initrd_devpath(&ramdisk_addr, &ramdisk_size,
> +					 above4g ? ULONG_MAX
> +						 : hdr->initrd_addr_max);
> +	if (status == EFI_NOT_FOUND)
> +		status = efi_load_initrd(image, hdr->initrd_addr_max,
> +					 above4g ? ULONG_MAX
> +						 : hdr->initrd_addr_max,
> +					 &ramdisk_addr, &ramdisk_size);

Fallback#2, for x86, also looks good.

>  	if (status != EFI_SUCCESS)
>  		goto fail2;
>  	hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
> @@ -732,6 +743,25 @@ struct boot_params *efi_main(efi_handle_t handle,
>  			 ((u64)boot_params->ext_cmd_line_ptr << 32));
>  	efi_parse_options((char *)cmdline_paddr);
>  
> +	if (!hdr->ramdisk_size && !boot_params->ext_ramdisk_size) {
> +		unsigned long max = hdr->initrd_addr_max;
> +		unsigned long addr, size;
> +
> +		if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
> +			max = ULONG_MAX;
> +
> +		status = efi_load_initrd_devpath(&addr, &size, max);
> +		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_devpath() failed!\n");
> +			goto fail;
> +		}
> +	}
> +

No fallback here; this is not a replacement for efi_load_initrd(), but a
brand new call. Why? (It's probably justified, I just don't know enough
about the kernel.)

>  	/*
>  	 * 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 9ccf313fe9de..75c83c322c40 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)
> 

Thanks,
Laszlo

Laszlo Ersek Feb. 7, 2020, 11:03 a.m. UTC | #13

On 02/06/20 19:26, Heinrich Schuchardt wrote:
> On 2/6/20 3:03 PM, Ard Biesheuvel wrote:

>> +    status = efi_bs_call(handle_protocol, handle, &lf2_proto_guid,
>> +                 (void **)&lf2);
>> +    if (status != EFI_SUCCESS)
>> +        return status;
> 
> You require here that there is a handle exposing the device path
> protocol with the initrd specific device path. On the same handle the
> EFI_LOAD_FILE2_PROTOCOL must be installed which will load the initrd
> file when called with the same device path.
> 
> An alternative implementation would simple loop over all instances of
> the EFI_LOAD_FILE2_PROTOCOL and try to load the initrd.

That's not a great idea IMO. EFI_LOAD_FILE2_PROTOCOL instances take
device-specific filenames / pathnames to load. If you pass any
particular pathname (e.g. "initrd" or "\\initrd") to random
EFI_LOAD_FILE2_PROTOCOL instance in the protocol database, there could
be undesired results / side effects. (It could cause network activity,
for example.)

Sticking with a VenMedia (i.e. GUID-ed) devpath is much safer; it
practically lets us define our own device-specific filename / pathname
space. (And in my other email, I suggested "use an empty devpath" for
device-specific pathname, because that's the simplest, it looks
spec-conformat, and it's safe, because our GUID makes the load attempt
unique already.)

I do agree with your question though: "Please, indicate which software
you expect to expose the initrd related EFI_LOAD_FILE2_PROTOCOL."

Thanks
Laszlo

Laszlo Ersek Feb. 7, 2020, 11:09 a.m. UTC | #14

On 02/06/20 23:35, Ard Biesheuvel wrote:
> On Thu, 6 Feb 2020 at 18:26, Heinrich Schuchardt <[e-mail]> wrote:

>> When booting via GRUB it will be GRUB knowing which initrd to load.
>>
> 
> Exactly, which is why GRUB will implement this protocol. That way, it
> does not have to touch the DT at all, or create a bootparams struct
> from setup data and inspect the various flags about placement,
> alignment, preferred addresses, etc.
> 
>> Please, indicate which software you expect to expose the initrd related
>> EFI_LOAD_FILE2_PROTOCOL.
>>
> 
> The primary use case is GRUB and other intermediate loaders, since it
> would remove any need for these components to know any such details.
> My aim is to make the next architecture that gets added to GRUB for
> EFI boot 100% generic.

Understood, thanks. It sounds plausible to me.

Laszlo

Ard Biesheuvel Feb. 7, 2020, 12:36 p.m. UTC | #15

On Fri, 7 Feb 2020 at 09:48, Laszlo Ersek <[e-mail]> wrote:
>
> On 02/06/20 15:03, 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 <[e-mail]>
> > ---
> >  drivers/firmware/efi/libstub/arm-stub.c        | 16 +++--
> >  drivers/firmware/efi/libstub/efi-stub-helper.c | 65 ++++++++++++++++++++
> >  drivers/firmware/efi/libstub/efistub.h         | 12 ++++
> >  drivers/firmware/efi/libstub/x86-stub.c        | 36 ++++++++++-
> >  include/linux/efi.h                            |  1 +
> >  5 files changed, 123 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> > index c7b091f50e55..1db943c1ba2b 100644
> > --- a/drivers/firmware/efi/libstub/arm-stub.c
> > +++ b/drivers/firmware/efi/libstub/arm-stub.c
> > @@ -157,6 +157,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;
> >
> > @@ -255,11 +256,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, ULONG_MAX,
> > -                              efi_get_max_initrd_addr(dram_base, *image_addr),
> > -                              &initrd_addr, &initrd_size);
> > +     max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
> > +     status = efi_load_initrd_devpath(&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, ULONG_MAX, max_addr,
> > +                                      &initrd_addr, &initrd_size);
>
> - So this seems to be fallback#1, for ARM, which looks good.
>
> - Are you sure you only want to fall back to the old method on
> EFI_NOT_FOUND? Wouldn't other return values from
> efi_load_initrd_devpath() justify that too?
>
> ... After checking the boot services called in
> efi_load_initrd_devpath(), this idea seems reasonable, but then I'd
> suggest documenting the significance of returning EFI_NOT_FOUND near the
> efi_load_initrd_devpath() function declaration, in "efistub.h".
>

Good point.

> > +             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 8e60a39df3c5..eaf45ea749b3 100644
> > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > @@ -323,3 +323,68 @@ void efi_char16_printk(efi_char16_t *str)
> >       efi_call_proto(efi_table_attr(efi_system_table(), con_out),
> >                      output_string, str);
> >  }
> > +
> > +static const struct {
> > +     struct efi_vendor_dev_path      vendor;
> > +     struct efi_generic_dev_path     end;
> > +} __packed initrd_devpath = {
> > +     {
> > +             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_status_t efi_load_initrd_devpath(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_devpath;
> > +     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;
> > +
> > +     initrd_size = 0;
> > +     status = efi_call_proto(lf2, load_file,
> > +                             (efi_device_path_protocol_t *)&initrd_devpath,
> > +                             false, &initrd_size, NULL);
>
> The second argument to EFI_LOAD_FILE2_PROTOCOL.LoadFile() is "FilePath",
> specified as "The device specific path of the file to load". This means
> it is supposed to be a (possibly empty) sequence of FILEPATH_DEVICE_PATH
> nodes, terminated by and "End Entire Device Path" node. See
>
> - 10.3.1 Generic Device Path Structures
> - 10.3.5.4 File Path Media Device Path
>
> in UEFI-2.8.
>
> And "initrd_devpath" is not a device path like that; instead it's the
> VenMedia device path that's installed on the handle that also carries
> our LoadFile2 instance.
>

OK, so you are saying this could be used to disambiguate which of
several files you may want to load from the initrd GUIDed device path?

> Now, I do see that this all theoretical here, as we don't expect the
> LoadFile2 instance that we've found via our special
> LINUX_EFI_INITRD_MEDIA_GUID VenMedia devpath to do *any* device-specific
> filename / pathname parsing.
>
> But in that case (i.e., given that the FilePath argument is totally
> irrelevant), I think it's much clearer if we simply pass an empty device
> path -- one that consists of a single "End Entire Device Path" node.
>
> I've checked, and your ArmVirtQemu patch ignores the FilePath argument
> too -- justifiedly so. I just think it's better to pass in a well-formed
> device path, rather than NULL. Because, the FilePath parameter is not
> marked OPTIONAL in the spec.
>

One thing that occurred to me is that we have to decide whether we
want to support the '10.3.5.8 Relative Offset Range' device path node
for this file, so that you could potentially load subranges of the
file. I don't see a use case for it right now, though.

But for my understanding, would the FilePath passed to LoadFile2 be
'Offset(...)+EndEntire' in that case? Or should it include the GUID
device path node as well?

> > +     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,
> > +                             (efi_device_path_protocol_t *)&initrd_devpath,
> > +                             false, &initrd_size, (void *)initrd_addr);
>
> Same here.
>
> > +     if (status != EFI_SUCCESS) {
> > +             efi_free(initrd_size, initrd_addr);
> > +             return status;
> > +     }
> > +
> > +     *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 99e93fd76ec5..fbf9f9442eed 100644
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -566,6 +566,14 @@ union efi_load_file_protocol {
> >       } mixed_mode;
> >  };
> >
> > +struct efi_vendor_dev_path {
> > +     u8              type;
> > +     u8              sub_type;
> > +     u16             length;
> > +     efi_guid_t      vendorguid;
> > +     u8              vendordata[];
> > +} __packed;
> > +
> >  void efi_pci_disable_bridge_busmaster(void);
> >
> >  typedef efi_status_t (*efi_exit_boot_map_processing)(
> > @@ -651,4 +659,8 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
> >                            unsigned long *load_addr,
> >                            unsigned long *load_size);
> >
> > +efi_status_t efi_load_initrd_devpath(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 f3e2ff31b624..7f38f95676dd 100644
> > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > @@ -419,9 +419,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> >       if (status != EFI_SUCCESS)
> >               goto fail2;
> >
> > -     status = efi_load_initrd(image, hdr->initrd_addr_max,
> > -                              above4g ? ULONG_MAX : hdr->initrd_addr_max,
> > -                              &ramdisk_addr, &ramdisk_size);
> > +     /*
> > +      * The initrd loaded from the Linux initrd vendor device
> > +      * path should take precedence, as we don't want the
> > +      * [unverified] command line to override the initrd
> > +      * supplied by the [potentially verified] firmware.
> > +      */
> > +     status = efi_load_initrd_devpath(&ramdisk_addr, &ramdisk_size,
> > +                                      above4g ? ULONG_MAX
> > +                                              : hdr->initrd_addr_max);
> > +     if (status == EFI_NOT_FOUND)
> > +             status = efi_load_initrd(image, hdr->initrd_addr_max,
> > +                                      above4g ? ULONG_MAX
> > +                                              : hdr->initrd_addr_max,
> > +                                      &ramdisk_addr, &ramdisk_size);
>
> Fallback#2, for x86, also looks good.
>
> >       if (status != EFI_SUCCESS)
> >               goto fail2;
> >       hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
> > @@ -732,6 +743,25 @@ struct boot_params *efi_main(efi_handle_t handle,
> >                        ((u64)boot_params->ext_cmd_line_ptr << 32));
> >       efi_parse_options((char *)cmdline_paddr);
> >
> > +     if (!hdr->ramdisk_size && !boot_params->ext_ramdisk_size) {
> > +             unsigned long max = hdr->initrd_addr_max;
> > +             unsigned long addr, size;
> > +
> > +             if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
> > +                     max = ULONG_MAX;
> > +
> > +             status = efi_load_initrd_devpath(&addr, &size, max);
> > +             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_devpath() failed!\n");
> > +                     goto fail;
> > +             }
> > +     }
> > +
>
> No fallback here; this is not a replacement for efi_load_initrd(), but a
> brand new call. Why? (It's probably justified, I just don't know enough
> about the kernel.)
>

Yes, it is. efi_main() is called after efi_pe_entry() in the native PE
boot case, and it is the only one being called when using the EFI
handover protocol.

So the expected new flow would be for efi_pe_entry() to run first and
load the initrd, in which case this code would not run. However, it
would be better to still support the EFI handover protocol when using
this method of loading the initrd, not only for convenience, but also
because we cannot call efi_pe_entry() when running in mixed mode.

Interestingly, LoadImage() can be used to load 64-bit images on 32-bit
EDK2 firmware today (and I have to check whether that holds for mixed
mode Mac hardware as well), only StartImage() will fail. It all
depends on whether we still care about mixed mode in the secure boot
context going forward, but if we do, we may be able to use LoadImage()
but then jump to this entry point (via startup_32 in head_32.S)

So the bottom line is that we have to keep this entry point alive for
the time being, but there is no reason to add initrd loading to it
using the old method, since we can't support it very well in mixed
mode (due to the prototypes of the protocols) and we don't support
that now anyway.

> >       /*
> >        * 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 9ccf313fe9de..75c83c322c40 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)
> >
>
> Thanks,
> Laszlo
>

Heinrich Schuchardt Feb. 7, 2020, 1:30 p.m. UTC | #16

On 2/7/20 9:12 AM, Ard Biesheuvel wrote:
> On Fri, 7 Feb 2020 at 00:58, Heinrich Schuchardt <[e-mail]> wrote:
>>
>> On 2/7/20 1:21 AM, Ard Biesheuvel wrote:
>>> On Fri, 7 Feb 2020 at 00:01, Heinrich Schuchardt <[e-mail]> wrote:
>>>>
>>>> On 2/6/20 11:35 PM, Ard Biesheuvel wrote:
>>>>> On Thu, 6 Feb 2020 at 18:26, Heinrich Schuchardt <[e-mail]> wrote:
> ...
>>>>>> Please, indicate which software you expect to expose the initrd related
>>>>>> EFI_LOAD_FILE2_PROTOCOL.
>>>>>>
>>>>>
>>>>> The primary use case is GRUB and other intermediate loaders, since it
>>>>> would remove any need for these components to know any such details.
>>>>> My aim is to make the next architecture that gets added to GRUB for
>>>>> EFI boot 100% generic.
>>>>>
>>>>>> Using an UEFI variable for passing the initrd device path would be a
>>>>>> leaner solution on the bootloader side than requiring an extra
>>>>>> EFI_LOAD_FILE2_PROTOCOL implementation.
>>>>>>
>>>>>
>>>>> This would also require kernel changes, since we don't currently load
>>>>> initrds from arbitrary device paths. The EFI_FILE_PROTOCOL is much
>>>>> more complicated than needed, and it doesn't work well with mixed
>>>>> mode. It also requires GRUB to expose the filesystem it loads the
>>>>> initrd from via EFI protocols, which is currently unnecessary and
>>>>> therefore not implemented.
>>>>
>>>> This means you move the complexity of EFI_FILE_PROTOCOL from Linux to GRUB.
>>>>
>>>
>>> No. I am not interested in EFI_FILE_PROTOCOL, only in LoadFile2, which
>>> is a single method that needs to be implemented.
>>
>> I said you move complexity because GRUB will need to use the
>> EFI_FILE_PROTOCOL to do the job that you do not want to do in Linux.
>>
>>>
>>>> I would not have a problem if this would only touch GRUB. But if listen
>>>> to Ilias we are replacing one implementation in Linux by one in GRUB and
>>>> one in U-Boot and one in EDK2 and one in any other firmware.
>>>>
>>>
>>> If u-boot will be used to boot RISC-V in EFI mode without GRUB, then I
>>> expect that we will need an implementation of this in u-boot.
>>
>> What sets RISC-V apart? GRUB for RISC-V is available.
  >>
>
> RISC-V EFI boot is not supported yet in upstream Linux.

It is currently prepared Atish Patra of WDC.

>
>>>
>>>>>
>>>>> Also, using an EFI variable defeats the purpose. The whole point of
>>>>> this is making it more likely that the kernel loaded the initrd that
>>>>> the bootloader or firmware intended it to load, and having a piece of
>>>>> simple [signed] code that implements this is the easiest way to
>>>>> achieve that.
>>>>
>>>> At least on my Debian system it is the operating system creating initrd
>>>> and defining which initrd matches which kernel. GRUB simply assumes that
>>>> files ending on the same version number match. Therefore I would say
>>>> Linux hopes that GRUB loads what Linux intended.
>>>>
>>>> The chain of trust would not be broken if the kernel were responsible
>>>> for loading the initrd and for checking if it matches the kernel. Linux
>>>> already does this for the kernel modules in initrd.
>>>>
>>>
>>> We can still sign the initrd and Linux can verify the signature. What
>>> I am after is an interface that does not require the initrd to
>>> originate from a EFI file system protocol, and which doesn't require
>>> the loaded initrd to sit in memory for an unspecified amount of time
>>> and its information passed via DT properties or bootparams structs.
>>>
>>> So invoking EFI_FILE_PROTOCOL directly is not going to work,
>>> regardless of whether we get the devicepath from the command line or
>>> from a EFI variable.
>>
>> What do you mean by "is not going to work"?
>>
>> With the device path you can find the handle implementing the
>> EFI_SIMPLE_FIL_SYSTEM_PROTOCOL.
>>
>>>
>>>>>
>>>>> For u-boot, it should be trivial to implement a simple LoadFile2
>>>>> protocol wrapper around EFI_FILE_PROTOCOL that can be installed on a
>>>>> handle that also carries EFI_FILE_PROTOCOL.
>>>>>
>>>>
>>>> A U-Boot implementation of the EFI_LOAD_FILE2_PROTOCOL would need a
>>>> device path variable to find the block device and to open the
>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL before accessing the file.
>>>>
>>>> Linux would not be needing more lines and we would not repeat the same
>>>> code in GRUB, U-Boot, EDK2, etc.
>>>>
>>>> As said Linux updates the initrd often. If that file is not signed by
>>>> Linux in a well defined way, do not expect any security at all.
>>>>
>>>
>>> It is not only about security. The primary goal is to remove the need
>>> for arch specific knowledge in the firmware about DT, bootparams and
>>> initrd allocation policies without being forced to load the initrd
>>> from a filesystem that is exposed via a EFI protocol.
>>
>> Where are device-trees touched by this patch?
>>
>> When booting via UEFI there is no need for knowledge of initrd
>> allocation policies in U-Boot because up to now Linux or GRUB or iPXE
>> load initrd.
>>
>> Furthermore I need no knowledge of bootparams in U-Boot once we properly
>> we support UEFI variables at runtime because grub-update will pass the
>> command line in one of the Bootxxxx UEFI variables.
>>
>> But most importantly I do not have to implement anything Linux specific
>> in U-Boot for booting via UEFI up to now.
>>
>
> Adding Linux specific stuff to u-boot is arguably more appropriate
> than adding architecture specific stuff to EFI loaders that could
> otherwise be entirely generic.
>
> ...
>>
>> Your patch claims to fend off a specific threat scenario: A user puts an
>> untrusted initrd on the disk and references it in the Linux command line.
>>
>> If he is able to do so with your current bootloader (signed or not
>> signed), he most probably will also be able to delete a good initrd from
>> the filesystem and thus force your code into the unsafe path.
>>
>> That is why I say that with the current fallback logic this patch
>> achieves no increase in security. Of cause you could remove the fallback
>> logic. But in this case your Linux will not boot with any legacy
>> bootloader or firmware.
>>
>
> If there is a better way to expose the initrd that
> a) does not require the initrd to reside on a file system that is
> accessible via EFI protocols, and
> b) does not require the loader to know about arch specific policies
> regarding the placement of the initrd in memory, and
> c) does not leave a time window between the time that the initrd is
> loaded/verified/measured by the firmware and the time that the kernel
> gets handed the buffer
>
> then I am happy to discuss it. This proposal is the best I could come
> up with to achieve the above.
>

Hello Ard,

I think part of our different views is that we are thinking about two
different use cases which both have their relevance:

If I understand you correctly, you are thinking about an embedded device
where the kernel and the initrd is essentially part of the firmware
provided by the device.

I am thinking of a system running a standard Linux distribution like
Debian where the initrd is generated by the operating system

In both use cases verifying the initrd is of importance.

Now concerning the requirements:

a) In U-Boot all file systems on block devices can be made accessible
via EFI protocols. Are you thinking about initrds that are not in a file
system?

b) My suggestion to use a UEFI variable for communicating the device
path would not require any arch specific policies either.

c) I proposed that the kernel does the verification. So there would be
equally nothing in between loading the file and its verification. Yet
responsibilities would be changed.

But possibly I missed some requirements you have in mind that I should
consider.

Best regards

Heinrich

Ard Biesheuvel Feb. 7, 2020, 1:58 p.m. UTC | #17

On Fri, 7 Feb 2020 at 13:30, Heinrich Schuchardt <[e-mail]> wrote:
>
>
>
> On 2/7/20 9:12 AM, Ard Biesheuvel wrote:
> > On Fri, 7 Feb 2020 at 00:58, Heinrich Schuchardt <[e-mail]> wrote:
> >>
> >> On 2/7/20 1:21 AM, Ard Biesheuvel wrote:
> >>> On Fri, 7 Feb 2020 at 00:01, Heinrich Schuchardt <[e-mail]> wrote:
> >>>>
> >>>> On 2/6/20 11:35 PM, Ard Biesheuvel wrote:
> >>>>> On Thu, 6 Feb 2020 at 18:26, Heinrich Schuchardt <[e-mail]> wrote:
> > ...
> >>>>>> Please, indicate which software you expect to expose the initrd related
> >>>>>> EFI_LOAD_FILE2_PROTOCOL.
> >>>>>>
> >>>>>
> >>>>> The primary use case is GRUB and other intermediate loaders, since it
> >>>>> would remove any need for these components to know any such details.
> >>>>> My aim is to make the next architecture that gets added to GRUB for
> >>>>> EFI boot 100% generic.
> >>>>>
> >>>>>> Using an UEFI variable for passing the initrd device path would be a
> >>>>>> leaner solution on the bootloader side than requiring an extra
> >>>>>> EFI_LOAD_FILE2_PROTOCOL implementation.
> >>>>>>
> >>>>>
> >>>>> This would also require kernel changes, since we don't currently load
> >>>>> initrds from arbitrary device paths. The EFI_FILE_PROTOCOL is much
> >>>>> more complicated than needed, and it doesn't work well with mixed
> >>>>> mode. It also requires GRUB to expose the filesystem it loads the
> >>>>> initrd from via EFI protocols, which is currently unnecessary and
> >>>>> therefore not implemented.
> >>>>
> >>>> This means you move the complexity of EFI_FILE_PROTOCOL from Linux to GRUB.
> >>>>
> >>>
> >>> No. I am not interested in EFI_FILE_PROTOCOL, only in LoadFile2, which
> >>> is a single method that needs to be implemented.
> >>
> >> I said you move complexity because GRUB will need to use the
> >> EFI_FILE_PROTOCOL to do the job that you do not want to do in Linux.
> >>
> >>>
> >>>> I would not have a problem if this would only touch GRUB. But if listen
> >>>> to Ilias we are replacing one implementation in Linux by one in GRUB and
> >>>> one in U-Boot and one in EDK2 and one in any other firmware.
> >>>>
> >>>
> >>> If u-boot will be used to boot RISC-V in EFI mode without GRUB, then I
> >>> expect that we will need an implementation of this in u-boot.
> >>
> >> What sets RISC-V apart? GRUB for RISC-V is available.
>   >>
> >
> > RISC-V EFI boot is not supported yet in upstream Linux.
>
> It is currently prepared Atish Patra of WDC.
>

Exactly. So it is not in the upstream yet, and I want to converge on a
sane generic interface before it gets merged.

> >
> >>>
> >>>>>
> >>>>> Also, using an EFI variable defeats the purpose. The whole point of
> >>>>> this is making it more likely that the kernel loaded the initrd that
> >>>>> the bootloader or firmware intended it to load, and having a piece of
> >>>>> simple [signed] code that implements this is the easiest way to
> >>>>> achieve that.
> >>>>
> >>>> At least on my Debian system it is the operating system creating initrd
> >>>> and defining which initrd matches which kernel. GRUB simply assumes that
> >>>> files ending on the same version number match. Therefore I would say
> >>>> Linux hopes that GRUB loads what Linux intended.
> >>>>
> >>>> The chain of trust would not be broken if the kernel were responsible
> >>>> for loading the initrd and for checking if it matches the kernel. Linux
> >>>> already does this for the kernel modules in initrd.
> >>>>
> >>>
> >>> We can still sign the initrd and Linux can verify the signature. What
> >>> I am after is an interface that does not require the initrd to
> >>> originate from a EFI file system protocol, and which doesn't require
> >>> the loaded initrd to sit in memory for an unspecified amount of time
> >>> and its information passed via DT properties or bootparams structs.
> >>>
> >>> So invoking EFI_FILE_PROTOCOL directly is not going to work,
> >>> regardless of whether we get the devicepath from the command line or
> >>> from a EFI variable.
> >>
> >> What do you mean by "is not going to work"?
> >>
> >> With the device path you can find the handle implementing the
> >> EFI_SIMPLE_FIL_SYSTEM_PROTOCOL.
> >>
> >>>
> >>>>>
> >>>>> For u-boot, it should be trivial to implement a simple LoadFile2
> >>>>> protocol wrapper around EFI_FILE_PROTOCOL that can be installed on a
> >>>>> handle that also carries EFI_FILE_PROTOCOL.
> >>>>>
> >>>>
> >>>> A U-Boot implementation of the EFI_LOAD_FILE2_PROTOCOL would need a
> >>>> device path variable to find the block device and to open the
> >>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL before accessing the file.
> >>>>
> >>>> Linux would not be needing more lines and we would not repeat the same
> >>>> code in GRUB, U-Boot, EDK2, etc.
> >>>>
> >>>> As said Linux updates the initrd often. If that file is not signed by
> >>>> Linux in a well defined way, do not expect any security at all.
> >>>>
> >>>
> >>> It is not only about security. The primary goal is to remove the need
> >>> for arch specific knowledge in the firmware about DT, bootparams and
> >>> initrd allocation policies without being forced to load the initrd
> >>> from a filesystem that is exposed via a EFI protocol.
> >>
> >> Where are device-trees touched by this patch?
> >>
> >> When booting via UEFI there is no need for knowledge of initrd
> >> allocation policies in U-Boot because up to now Linux or GRUB or iPXE
> >> load initrd.
> >>
> >> Furthermore I need no knowledge of bootparams in U-Boot once we properly
> >> we support UEFI variables at runtime because grub-update will pass the
> >> command line in one of the Bootxxxx UEFI variables.
> >>
> >> But most importantly I do not have to implement anything Linux specific
> >> in U-Boot for booting via UEFI up to now.
> >>
> >
> > Adding Linux specific stuff to u-boot is arguably more appropriate
> > than adding architecture specific stuff to EFI loaders that could
> > otherwise be entirely generic.
> >
> > ...
> >>
> >> Your patch claims to fend off a specific threat scenario: A user puts an
> >> untrusted initrd on the disk and references it in the Linux command line.
> >>
> >> If he is able to do so with your current bootloader (signed or not
> >> signed), he most probably will also be able to delete a good initrd from
> >> the filesystem and thus force your code into the unsafe path.
> >>
> >> That is why I say that with the current fallback logic this patch
> >> achieves no increase in security. Of cause you could remove the fallback
> >> logic. But in this case your Linux will not boot with any legacy
> >> bootloader or firmware.
> >>
> >
> > If there is a better way to expose the initrd that
> > a) does not require the initrd to reside on a file system that is
> > accessible via EFI protocols, and
> > b) does not require the loader to know about arch specific policies
> > regarding the placement of the initrd in memory, and
> > c) does not leave a time window between the time that the initrd is
> > loaded/verified/measured by the firmware and the time that the kernel
> > gets handed the buffer
> >
> > then I am happy to discuss it. This proposal is the best I could come
> > up with to achieve the above.
> >
>
> Hello Ard,
>
> I think part of our different views is that we are thinking about two
> different use cases which both have their relevance:
>
> If I understand you correctly, you are thinking about an embedded device
> where the kernel and the initrd is essentially part of the firmware
> provided by the device.
>
> I am thinking of a system running a standard Linux distribution like
> Debian where the initrd is generated by the operating system
>
> In both use cases verifying the initrd is of importance.
>
> Now concerning the requirements:
>
> a) In U-Boot all file systems on block devices can be made accessible
> via EFI protocols. Are you thinking about initrds that are not in a file
> system?
>

The typical GRUB deployment keeps the core GRUB itself (or the entire
thing if it is built as standalone) in the ESP, and the GRUB modules,
kernel images and initrds are in /boot, which is typically not a file
system that EFI understands. So in that case, initrd= does not work,
which is why GRUB loads the initrd into memory directly and passes the
base address and size via DT or bootparams structure.

> b) My suggestion to use a UEFI variable for communicating the device
> path would not require any arch specific policies either.
>

Passing the EFI device path is not going to help us since the initrd
may not be representable as a device path. That is the whole point,
actually - this series makes the initrd representable as a device
path, but in a simple way that doesn't rely on EFI_FILE_PROTOCOL but
only on EFI_LOAD_FILE2_PROTOCOL, which is *much* simpler.

> c) I proposed that the kernel does the verification. So there would be
> equally nothing in between loading the file and its verification. Yet
> responsibilities would be changed.
>
> But possibly I missed some requirements you have in mind that I should
> consider.
>

1) The assumption that the initrd can always be loaded from a EFI
device path directly does not hold.
2) Loading the initrd into memory and passing the address and size is
not acceptable.
3) Having a special device path that designates the initrd
specifically (which will be treated in a special way by the kernel)
makes it very easy for the boot firmware to attach policy to the
loading of the initrd that can be enforced right when the file is
being passed into the kernel.

Putting an arbitrary device path in a EFI variable doesn't address 1),
and it complicates 3), since you cannot easily distinguish whether the
file load that is occurring is the EFI stub loading the initrd at
boot.

Alexander Graf Feb. 7, 2020, 2:18 p.m. UTC | #18

On 07.02.20 13:58, Ard Biesheuvel wrote:
> On Fri, 7 Feb 2020 at 13:30, Heinrich Schuchardt <[e-mail]> wrote:
>>
>>
>> On 2/7/20 9:12 AM, Ard Biesheuvel wrote:
>>> On Fri, 7 Feb 2020 at 00:58, Heinrich Schuchardt <[e-mail]> wrote:
>>>> On 2/7/20 1:21 AM, Ard Biesheuvel wrote:
>>>>> On Fri, 7 Feb 2020 at 00:01, Heinrich Schuchardt <[e-mail]> wrote:
>>>>>> On 2/6/20 11:35 PM, Ard Biesheuvel wrote:
>>>>>>> On Thu, 6 Feb 2020 at 18:26, Heinrich Schuchardt <[e-mail]> wrote:
>>> ...
>>>>>>>> Please, indicate which software you expect to expose the initrd related
>>>>>>>> EFI_LOAD_FILE2_PROTOCOL.
>>>>>>>>
>>>>>>> The primary use case is GRUB and other intermediate loaders, since it
>>>>>>> would remove any need for these components to know any such details.
>>>>>>> My aim is to make the next architecture that gets added to GRUB for
>>>>>>> EFI boot 100% generic.
>>>>>>>
>>>>>>>> Using an UEFI variable for passing the initrd device path would be a
>>>>>>>> leaner solution on the bootloader side than requiring an extra
>>>>>>>> EFI_LOAD_FILE2_PROTOCOL implementation.
>>>>>>>>
>>>>>>> This would also require kernel changes, since we don't currently load
>>>>>>> initrds from arbitrary device paths. The EFI_FILE_PROTOCOL is much
>>>>>>> more complicated than needed, and it doesn't work well with mixed
>>>>>>> mode. It also requires GRUB to expose the filesystem it loads the
>>>>>>> initrd from via EFI protocols, which is currently unnecessary and
>>>>>>> therefore not implemented.
>>>>>> This means you move the complexity of EFI_FILE_PROTOCOL from Linux to GRUB.
>>>>>>
>>>>> No. I am not interested in EFI_FILE_PROTOCOL, only in LoadFile2, which
>>>>> is a single method that needs to be implemented.
>>>> I said you move complexity because GRUB will need to use the
>>>> EFI_FILE_PROTOCOL to do the job that you do not want to do in Linux.
>>>>
>>>>>> I would not have a problem if this would only touch GRUB. But if listen
>>>>>> to Ilias we are replacing one implementation in Linux by one in GRUB and
>>>>>> one in U-Boot and one in EDK2 and one in any other firmware.
>>>>>>
>>>>> If u-boot will be used to boot RISC-V in EFI mode without GRUB, then I
>>>>> expect that we will need an implementation of this in u-boot.
>>>> What sets RISC-V apart? GRUB for RISC-V is available.
>>    >>
>>> RISC-V EFI boot is not supported yet in upstream Linux.
>> It is currently prepared Atish Patra of WDC.
>>
> Exactly. So it is not in the upstream yet, and I want to converge on a
> sane generic interface before it gets merged.
>
>>>>>>> Also, using an EFI variable defeats the purpose. The whole point of
>>>>>>> this is making it more likely that the kernel loaded the initrd that
>>>>>>> the bootloader or firmware intended it to load, and having a piece of
>>>>>>> simple [signed] code that implements this is the easiest way to
>>>>>>> achieve that.
>>>>>> At least on my Debian system it is the operating system creating initrd
>>>>>> and defining which initrd matches which kernel. GRUB simply assumes that
>>>>>> files ending on the same version number match. Therefore I would say
>>>>>> Linux hopes that GRUB loads what Linux intended.
>>>>>>
>>>>>> The chain of trust would not be broken if the kernel were responsible
>>>>>> for loading the initrd and for checking if it matches the kernel. Linux
>>>>>> already does this for the kernel modules in initrd.
>>>>>>
>>>>> We can still sign the initrd and Linux can verify the signature. What
>>>>> I am after is an interface that does not require the initrd to
>>>>> originate from a EFI file system protocol, and which doesn't require
>>>>> the loaded initrd to sit in memory for an unspecified amount of time
>>>>> and its information passed via DT properties or bootparams structs.
>>>>>
>>>>> So invoking EFI_FILE_PROTOCOL directly is not going to work,
>>>>> regardless of whether we get the devicepath from the command line or
>>>>> from a EFI variable.
>>>> What do you mean by "is not going to work"?
>>>>
>>>> With the device path you can find the handle implementing the
>>>> EFI_SIMPLE_FIL_SYSTEM_PROTOCOL.
>>>>
>>>>>>> For u-boot, it should be trivial to implement a simple LoadFile2
>>>>>>> protocol wrapper around EFI_FILE_PROTOCOL that can be installed on a
>>>>>>> handle that also carries EFI_FILE_PROTOCOL.
>>>>>>>
>>>>>> A U-Boot implementation of the EFI_LOAD_FILE2_PROTOCOL would need a
>>>>>> device path variable to find the block device and to open the
>>>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL before accessing the file.
>>>>>>
>>>>>> Linux would not be needing more lines and we would not repeat the same
>>>>>> code in GRUB, U-Boot, EDK2, etc.
>>>>>>
>>>>>> As said Linux updates the initrd often. If that file is not signed by
>>>>>> Linux in a well defined way, do not expect any security at all.
>>>>>>
>>>>> It is not only about security. The primary goal is to remove the need
>>>>> for arch specific knowledge in the firmware about DT, bootparams and
>>>>> initrd allocation policies without being forced to load the initrd
>>>>> from a filesystem that is exposed via a EFI protocol.
>>>> Where are device-trees touched by this patch?
>>>>
>>>> When booting via UEFI there is no need for knowledge of initrd
>>>> allocation policies in U-Boot because up to now Linux or GRUB or iPXE
>>>> load initrd.
>>>>
>>>> Furthermore I need no knowledge of bootparams in U-Boot once we properly
>>>> we support UEFI variables at runtime because grub-update will pass the
>>>> command line in one of the Bootxxxx UEFI variables.
>>>>
>>>> But most importantly I do not have to implement anything Linux specific
>>>> in U-Boot for booting via UEFI up to now.
>>>>
>>> Adding Linux specific stuff to u-boot is arguably more appropriate
>>> than adding architecture specific stuff to EFI loaders that could
>>> otherwise be entirely generic.
>>>
>>> ...
>>>> Your patch claims to fend off a specific threat scenario: A user puts an
>>>> untrusted initrd on the disk and references it in the Linux command line.
>>>>
>>>> If he is able to do so with your current bootloader (signed or not
>>>> signed), he most probably will also be able to delete a good initrd from
>>>> the filesystem and thus force your code into the unsafe path.
>>>>
>>>> That is why I say that with the current fallback logic this patch
>>>> achieves no increase in security. Of cause you could remove the fallback
>>>> logic. But in this case your Linux will not boot with any legacy
>>>> bootloader or firmware.
>>>>
>>> If there is a better way to expose the initrd that
>>> a) does not require the initrd to reside on a file system that is
>>> accessible via EFI protocols, and
>>> b) does not require the loader to know about arch specific policies
>>> regarding the placement of the initrd in memory, and
>>> c) does not leave a time window between the time that the initrd is
>>> loaded/verified/measured by the firmware and the time that the kernel
>>> gets handed the buffer
>>>
>>> then I am happy to discuss it. This proposal is the best I could come
>>> up with to achieve the above.
>>>
>> Hello Ard,
>>
>> I think part of our different views is that we are thinking about two
>> different use cases which both have their relevance:
>>
>> If I understand you correctly, you are thinking about an embedded device
>> where the kernel and the initrd is essentially part of the firmware
>> provided by the device.
>>
>> I am thinking of a system running a standard Linux distribution like
>> Debian where the initrd is generated by the operating system
>>
>> In both use cases verifying the initrd is of importance.
>>
>> Now concerning the requirements:
>>
>> a) In U-Boot all file systems on block devices can be made accessible
>> via EFI protocols. Are you thinking about initrds that are not in a file
>> system?
>>
> The typical GRUB deployment keeps the core GRUB itself (or the entire
> thing if it is built as standalone) in the ESP, and the GRUB modules,
> kernel images and initrds are in /boot, which is typically not a file
> system that EFI understands. So in that case, initrd= does not work,
> which is why GRUB loads the initrd into memory directly and passes the
> base address and size via DT or bootparams structure.
>
>> b) My suggestion to use a UEFI variable for communicating the device
>> path would not require any arch specific policies either.
>>
> Passing the EFI device path is not going to help us since the initrd
> may not be representable as a device path. That is the whole point,
> actually - this series makes the initrd representable as a device
> path, but in a simple way that doesn't rely on EFI_FILE_PROTOCOL but
> only on EFI_LOAD_FILE2_PROTOCOL, which is *much* simpler.


So if we had support in grub to just export its own file systems as UEFI 
protocols, that problem would disappear, right? What other reasons are 
left to not just use normal file load operations from the Linux EFI stub?

IIRC you mentioned that the fwcfg -kernel and -initrd parameters are 
already exposed as pseudo filesystem inside AAVMF, so that one is solved.

I think that only leaves the UEFI shell case? But if you have a UEFI 
shell, then you can only load from an existing file system as well, no?

What I can't quite grasp yet is how you would handle multiple initrds 
with a single device path. How would that work?


>
>> c) I proposed that the kernel does the verification. So there would be
>> equally nothing in between loading the file and its verification. Yet
>> responsibilities would be changed.
>>
>> But possibly I missed some requirements you have in mind that I should
>> consider.
>>
> 1) The assumption that the initrd can always be loaded from a EFI
> device path directly does not hold.


Can you think of good reasons why this is true? I understand the grub 
one, but that's solvable. What other cases are there?


> 2) Loading the initrd into memory and passing the address and size is
> not acceptable.


This would basically be the option to pass the initrd as configuration 
table, right? The only reason that definitely goes against that one that 
I can think of right now is to avoid double copying?


> 3) Having a special device path that designates the initrd
> specifically (which will be treated in a special way by the kernel)
> makes it very easy for the boot firmware to attach policy to the
> loading of the initrd that can be enforced right when the file is
> being passed into the kernel.


I don't fully understand this one. Can you give examples for such 
policies? :)


> Putting an arbitrary device path in a EFI variable doesn't address 1),
> and it complicates 3), since you cannot easily distinguish whether the
> file load that is occurring is the EFI stub loading the initrd at
> boot.


Why do we need to distinguish? I'm missing creativity for the use case 
right now. For 1), we just need to make sure that boot loaders that 
implement their own file systems also expose those file systems as UEFI 
protocols, right?


That said, I don't think the proposed approach of using 
EFI_LOAD_FILE2_PROTOCOL is bad. Whatever we do, we always will need to 
treat initrds special in one way or another; at least by exposing a 
command to specify which one(s) you want to load.


Alex

Ard Biesheuvel Feb. 7, 2020, 3:30 p.m. UTC | #19

On Fri, 7 Feb 2020 at 14:18, Alexander Graf <[e-mail]> wrote:
>
>
> On 07.02.20 13:58, Ard Biesheuvel wrote:
> > On Fri, 7 Feb 2020 at 13:30, Heinrich Schuchardt <[e-mail]> wrote:
> >>
> >>
> >> On 2/7/20 9:12 AM, Ard Biesheuvel wrote:
> >>> On Fri, 7 Feb 2020 at 00:58, Heinrich Schuchardt <[e-mail]> wrote:
> >>>> On 2/7/20 1:21 AM, Ard Biesheuvel wrote:
> >>>>> On Fri, 7 Feb 2020 at 00:01, Heinrich Schuchardt <[e-mail]> wrote:
> >>>>>> On 2/6/20 11:35 PM, Ard Biesheuvel wrote:
> >>>>>>> On Thu, 6 Feb 2020 at 18:26, Heinrich Schuchardt <[e-mail]> wrote:
> >>> ...
> >>>>>>>> Please, indicate which software you expect to expose the initrd related
> >>>>>>>> EFI_LOAD_FILE2_PROTOCOL.
> >>>>>>>>
> >>>>>>> The primary use case is GRUB and other intermediate loaders, since it
> >>>>>>> would remove any need for these components to know any such details.
> >>>>>>> My aim is to make the next architecture that gets added to GRUB for
> >>>>>>> EFI boot 100% generic.
> >>>>>>>
> >>>>>>>> Using an UEFI variable for passing the initrd device path would be a
> >>>>>>>> leaner solution on the bootloader side than requiring an extra
> >>>>>>>> EFI_LOAD_FILE2_PROTOCOL implementation.
> >>>>>>>>
> >>>>>>> This would also require kernel changes, since we don't currently load
> >>>>>>> initrds from arbitrary device paths. The EFI_FILE_PROTOCOL is much
> >>>>>>> more complicated than needed, and it doesn't work well with mixed
> >>>>>>> mode. It also requires GRUB to expose the filesystem it loads the
> >>>>>>> initrd from via EFI protocols, which is currently unnecessary and
> >>>>>>> therefore not implemented.
> >>>>>> This means you move the complexity of EFI_FILE_PROTOCOL from Linux to GRUB.
> >>>>>>
> >>>>> No. I am not interested in EFI_FILE_PROTOCOL, only in LoadFile2, which
> >>>>> is a single method that needs to be implemented.
> >>>> I said you move complexity because GRUB will need to use the
> >>>> EFI_FILE_PROTOCOL to do the job that you do not want to do in Linux.
> >>>>
> >>>>>> I would not have a problem if this would only touch GRUB. But if listen
> >>>>>> to Ilias we are replacing one implementation in Linux by one in GRUB and
> >>>>>> one in U-Boot and one in EDK2 and one in any other firmware.
> >>>>>>
> >>>>> If u-boot will be used to boot RISC-V in EFI mode without GRUB, then I
> >>>>> expect that we will need an implementation of this in u-boot.
> >>>> What sets RISC-V apart? GRUB for RISC-V is available.
> >>    >>
> >>> RISC-V EFI boot is not supported yet in upstream Linux.
> >> It is currently prepared Atish Patra of WDC.
> >>
> > Exactly. So it is not in the upstream yet, and I want to converge on a
> > sane generic interface before it gets merged.
> >
> >>>>>>> Also, using an EFI variable defeats the purpose. The whole point of
> >>>>>>> this is making it more likely that the kernel loaded the initrd that
> >>>>>>> the bootloader or firmware intended it to load, and having a piece of
> >>>>>>> simple [signed] code that implements this is the easiest way to
> >>>>>>> achieve that.
> >>>>>> At least on my Debian system it is the operating system creating initrd
> >>>>>> and defining which initrd matches which kernel. GRUB simply assumes that
> >>>>>> files ending on the same version number match. Therefore I would say
> >>>>>> Linux hopes that GRUB loads what Linux intended.
> >>>>>>
> >>>>>> The chain of trust would not be broken if the kernel were responsible
> >>>>>> for loading the initrd and for checking if it matches the kernel. Linux
> >>>>>> already does this for the kernel modules in initrd.
> >>>>>>
> >>>>> We can still sign the initrd and Linux can verify the signature. What
> >>>>> I am after is an interface that does not require the initrd to
> >>>>> originate from a EFI file system protocol, and which doesn't require
> >>>>> the loaded initrd to sit in memory for an unspecified amount of time
> >>>>> and its information passed via DT properties or bootparams structs.
> >>>>>
> >>>>> So invoking EFI_FILE_PROTOCOL directly is not going to work,
> >>>>> regardless of whether we get the devicepath from the command line or
> >>>>> from a EFI variable.
> >>>> What do you mean by "is not going to work"?
> >>>>
> >>>> With the device path you can find the handle implementing the
> >>>> EFI_SIMPLE_FIL_SYSTEM_PROTOCOL.
> >>>>
> >>>>>>> For u-boot, it should be trivial to implement a simple LoadFile2
> >>>>>>> protocol wrapper around EFI_FILE_PROTOCOL that can be installed on a
> >>>>>>> handle that also carries EFI_FILE_PROTOCOL.
> >>>>>>>
> >>>>>> A U-Boot implementation of the EFI_LOAD_FILE2_PROTOCOL would need a
> >>>>>> device path variable to find the block device and to open the
> >>>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL before accessing the file.
> >>>>>>
> >>>>>> Linux would not be needing more lines and we would not repeat the same
> >>>>>> code in GRUB, U-Boot, EDK2, etc.
> >>>>>>
> >>>>>> As said Linux updates the initrd often. If that file is not signed by
> >>>>>> Linux in a well defined way, do not expect any security at all.
> >>>>>>
> >>>>> It is not only about security. The primary goal is to remove the need
> >>>>> for arch specific knowledge in the firmware about DT, bootparams and
> >>>>> initrd allocation policies without being forced to load the initrd
> >>>>> from a filesystem that is exposed via a EFI protocol.
> >>>> Where are device-trees touched by this patch?
> >>>>
> >>>> When booting via UEFI there is no need for knowledge of initrd
> >>>> allocation policies in U-Boot because up to now Linux or GRUB or iPXE
> >>>> load initrd.
> >>>>
> >>>> Furthermore I need no knowledge of bootparams in U-Boot once we properly
> >>>> we support UEFI variables at runtime because grub-update will pass the
> >>>> command line in one of the Bootxxxx UEFI variables.
> >>>>
> >>>> But most importantly I do not have to implement anything Linux specific
> >>>> in U-Boot for booting via UEFI up to now.
> >>>>
> >>> Adding Linux specific stuff to u-boot is arguably more appropriate
> >>> than adding architecture specific stuff to EFI loaders that could
> >>> otherwise be entirely generic.
> >>>
> >>> ...
> >>>> Your patch claims to fend off a specific threat scenario: A user puts an
> >>>> untrusted initrd on the disk and references it in the Linux command line.
> >>>>
> >>>> If he is able to do so with your current bootloader (signed or not
> >>>> signed), he most probably will also be able to delete a good initrd from
> >>>> the filesystem and thus force your code into the unsafe path.
> >>>>
> >>>> That is why I say that with the current fallback logic this patch
> >>>> achieves no increase in security. Of cause you could remove the fallback
> >>>> logic. But in this case your Linux will not boot with any legacy
> >>>> bootloader or firmware.
> >>>>
> >>> If there is a better way to expose the initrd that
> >>> a) does not require the initrd to reside on a file system that is
> >>> accessible via EFI protocols, and
> >>> b) does not require the loader to know about arch specific policies
> >>> regarding the placement of the initrd in memory, and
> >>> c) does not leave a time window between the time that the initrd is
> >>> loaded/verified/measured by the firmware and the time that the kernel
> >>> gets handed the buffer
> >>>
> >>> then I am happy to discuss it. This proposal is the best I could come
> >>> up with to achieve the above.
> >>>
> >> Hello Ard,
> >>
> >> I think part of our different views is that we are thinking about two
> >> different use cases which both have their relevance:
> >>
> >> If I understand you correctly, you are thinking about an embedded device
> >> where the kernel and the initrd is essentially part of the firmware
> >> provided by the device.
> >>
> >> I am thinking of a system running a standard Linux distribution like
> >> Debian where the initrd is generated by the operating system
> >>
> >> In both use cases verifying the initrd is of importance.
> >>
> >> Now concerning the requirements:
> >>
> >> a) In U-Boot all file systems on block devices can be made accessible
> >> via EFI protocols. Are you thinking about initrds that are not in a file
> >> system?
> >>
> > The typical GRUB deployment keeps the core GRUB itself (or the entire
> > thing if it is built as standalone) in the ESP, and the GRUB modules,
> > kernel images and initrds are in /boot, which is typically not a file
> > system that EFI understands. So in that case, initrd= does not work,
> > which is why GRUB loads the initrd into memory directly and passes the
> > base address and size via DT or bootparams structure.
> >
> >> b) My suggestion to use a UEFI variable for communicating the device
> >> path would not require any arch specific policies either.
> >>
> > Passing the EFI device path is not going to help us since the initrd
> > may not be representable as a device path. That is the whole point,
> > actually - this series makes the initrd representable as a device
> > path, but in a simple way that doesn't rely on EFI_FILE_PROTOCOL but
> > only on EFI_LOAD_FILE2_PROTOCOL, which is *much* simpler.
>
>
> So if we had support in grub to just export its own file systems as UEFI
> protocols, that problem would disappear, right? What other reasons are
> left to not just use normal file load operations from the Linux EFI stub?
>

1) complexity

"""
typedef struct {
  u64     size;
  u64     file_size;
  u64     phys_size;
  efi_time_t    create_time;
  efi_time_t    last_access_time;
  efi_time_t    modification_time;
  __aligned_u64   attribute;
  efi_char16_t    filename[];
} efi_file_info_t;

typedef struct efi_file_protocol efi_file_protocol_t;

struct efi_file_protocol {
  u64   revision;
  efi_status_t  (__efiapi *open)  (efi_file_protocol_t *,
             efi_file_protocol_t **,
             efi_char16_t *, u64, u64);
  efi_status_t  (__efiapi *close) (efi_file_protocol_t *);
  efi_status_t  (__efiapi *delete)  (efi_file_protocol_t *);
  efi_status_t  (__efiapi *read)  (efi_file_protocol_t *,
             unsigned long *, void *);
  efi_status_t  (__efiapi *write) (efi_file_protocol_t *,
             unsigned long, void *);
  efi_status_t  (__efiapi *get_position)(efi_file_protocol_t *, u64 *);
  efi_status_t  (__efiapi *set_position)(efi_file_protocol_t *, u64);
  efi_status_t  (__efiapi *get_info)  (efi_file_protocol_t *,
             efi_guid_t *, unsigned long *,
             void *);
  efi_status_t  (__efiapi *set_info)  (efi_file_protocol_t *,
             efi_guid_t *, unsigned long,
             void *);
  efi_status_t  (__efiapi *flush) (efi_file_protocol_t *);
};

typedef struct efi_simple_file_system_protocol
efi_simple_file_system_protocol_t;

struct efi_simple_file_system_protocol {
  u64 revision;
  int (__efiapi *open_volume)(efi_simple_file_system_protocol_t *,
          efi_file_protocol_t **);
};

#define EFI_FILE_MODE_READ  0x0000000000000001
#define EFI_FILE_MODE_WRITE 0x0000000000000002
#define EFI_FILE_MODE_CREATE  0x8000000000000000
"""

versus

"""
union efi_load_file_protocol {
struct {
  efi_status_t (__efiapi *load_file)(efi_load_file_protocol_t *,
       efi_device_path_protocol_t *,
       bool, unsigned long *, void *);
  };
};
"""

2) mixed mode is problematic due to the use of u64 arguments in the prototypes
3) you have to trust that the device path that was specified was
intended to be loaded as an initrd
4) if you want to authenticate or measure the initrd from the firmware
as it is read, you have to disambiguate EFI_FILE_PROTOCOL calls
involving the initrd from arbitrary other calls.


> IIRC you mentioned that the fwcfg -kernel and -initrd parameters are
> already exposed as pseudo filesystem inside AAVMF, so that one is solved.
>

Yes, but this only works if you use the PE entry point, which rules
out mixed mode.

> I think that only leaves the UEFI shell case? But if you have a UEFI
> shell, then you can only load from an existing file system as well, no?
>

Yes.

> What I can't quite grasp yet is how you would handle multiple initrds
> with a single device path. How would that work?
>

The core kernel does not care about multiple initrds, they have to be
concatenated and passed as one anyway. In this case, that means the
implementation of LoadFile2 is in charge of this.

>
> >
> >> c) I proposed that the kernel does the verification. So there would be
> >> equally nothing in between loading the file and its verification. Yet
> >> responsibilities would be changed.
> >>
> >> But possibly I missed some requirements you have in mind that I should
> >> consider.
> >>
> > 1) The assumption that the initrd can always be loaded from a EFI
> > device path directly does not hold.
>
>
> Can you think of good reasons why this is true? I understand the grub
> one, but that's solvable. What other cases are there?
>

Thin EFI implementations in enclaves, LinuxBoot on arm64, ...

>
> > 2) Loading the initrd into memory and passing the address and size is
> > not acceptable.
>
>
> This would basically be the option to pass the initrd as configuration
> table, right? The only reason that definitely goes against that one that
> I can think of right now is to avoid double copying?
>

That basically forces the stub to reallocate it, unless we put the
[arch specific] allocation policy in the loader. initrds can be large,
and so having to copy them can be a problem.

>
> > 3) Having a special device path that designates the initrd
> > specifically (which will be treated in a special way by the kernel)
> > makes it very easy for the boot firmware to attach policy to the
> > loading of the initrd that can be enforced right when the file is
> > being passed into the kernel.
>
>
> I don't fully understand this one. Can you give examples for such
> policies? :)
>

If you want to measure the initrd into PCR x, it gives you a natural
place to do it, since the data is measured right at the point where
the stub consumes it, and the protocol will never be invoked if the
initrd is not requested (e.g., if the boot environment exposes a
initrd but 'noinitrd' is passed on the command line, in which case we
would prefer not to measure the initrd at all)

>
> > Putting an arbitrary device path in a EFI variable doesn't address 1),
> > and it complicates 3), since you cannot easily distinguish whether the
> > file load that is occurring is the EFI stub loading the initrd at
> > boot.
>
>
> Why do we need to distinguish? I'm missing creativity for the use case
> right now. For 1), we just need to make sure that boot loaders that
> implement their own file systems also expose those file systems as UEFI
> protocols, right?
>

Because an arbitrary file read call cannot be identified as being the
one issued by the stub to load and invoke the initrd.

>
> That said, I don't think the proposed approach of using
> EFI_LOAD_FILE2_PROTOCOL is bad. Whatever we do, we always will need to
> treat initrds special in one way or another; at least by exposing a
> command to specify which one(s) you want to load.
>

Heinrich Schuchardt Feb. 7, 2020, 3:35 p.m. UTC | #20

On 2/7/20 3:18 PM, Alexander Graf wrote:
>
> On 07.02.20 13:58, Ard Biesheuvel wrote:
>> On Fri, 7 Feb 2020 at 13:30, Heinrich Schuchardt <[e-mail]>
>> wrote:
>>>
>>>
>>> On 2/7/20 9:12 AM, Ard Biesheuvel wrote:
>>>> On Fri, 7 Feb 2020 at 00:58, Heinrich Schuchardt
>>>> <[e-mail]> wrote:
>>>>> On 2/7/20 1:21 AM, Ard Biesheuvel wrote:
>>>>>> On Fri, 7 Feb 2020 at 00:01, Heinrich Schuchardt
>>>>>> <[e-mail]> wrote:
>>>>>>> On 2/6/20 11:35 PM, Ard Biesheuvel wrote:
>>>>>>>> On Thu, 6 Feb 2020 at 18:26, Heinrich Schuchardt
>>>>>>>> <[e-mail]> wrote:
>>>> ...
>>>>>>>>> Please, indicate which software you expect to expose the initrd
>>>>>>>>> related
>>>>>>>>> EFI_LOAD_FILE2_PROTOCOL.
>>>>>>>>>
>>>>>>>> The primary use case is GRUB and other intermediate loaders,
>>>>>>>> since it
>>>>>>>> would remove any need for these components to know any such
>>>>>>>> details.
>>>>>>>> My aim is to make the next architecture that gets added to GRUB for
>>>>>>>> EFI boot 100% generic.
>>>>>>>>
>>>>>>>>> Using an UEFI variable for passing the initrd device path would
>>>>>>>>> be a
>>>>>>>>> leaner solution on the bootloader side than requiring an extra
>>>>>>>>> EFI_LOAD_FILE2_PROTOCOL implementation.
>>>>>>>>>
>>>>>>>> This would also require kernel changes, since we don't currently
>>>>>>>> load
>>>>>>>> initrds from arbitrary device paths. The EFI_FILE_PROTOCOL is much
>>>>>>>> more complicated than needed, and it doesn't work well with mixed
>>>>>>>> mode. It also requires GRUB to expose the filesystem it loads the
>>>>>>>> initrd from via EFI protocols, which is currently unnecessary and
>>>>>>>> therefore not implemented.
>>>>>>> This means you move the complexity of EFI_FILE_PROTOCOL from
>>>>>>> Linux to GRUB.
>>>>>>>
>>>>>> No. I am not interested in EFI_FILE_PROTOCOL, only in LoadFile2,
>>>>>> which
>>>>>> is a single method that needs to be implemented.
>>>>> I said you move complexity because GRUB will need to use the
>>>>> EFI_FILE_PROTOCOL to do the job that you do not want to do in Linux.
>>>>>
>>>>>>> I would not have a problem if this would only touch GRUB. But if
>>>>>>> listen
>>>>>>> to Ilias we are replacing one implementation in Linux by one in
>>>>>>> GRUB and
>>>>>>> one in U-Boot and one in EDK2 and one in any other firmware.
>>>>>>>
>>>>>> If u-boot will be used to boot RISC-V in EFI mode without GRUB,
>>>>>> then I
>>>>>> expect that we will need an implementation of this in u-boot.
>>>>> What sets RISC-V apart? GRUB for RISC-V is available.
>>>    >>
>>>> RISC-V EFI boot is not supported yet in upstream Linux.
>>> It is currently prepared Atish Patra of WDC.
>>>
>> Exactly. So it is not in the upstream yet, and I want to converge on a
>> sane generic interface before it gets merged.
>>
>>>>>>>> Also, using an EFI variable defeats the purpose. The whole point of
>>>>>>>> this is making it more likely that the kernel loaded the initrd
>>>>>>>> that
>>>>>>>> the bootloader or firmware intended it to load, and having a
>>>>>>>> piece of
>>>>>>>> simple [signed] code that implements this is the easiest way to
>>>>>>>> achieve that.
>>>>>>> At least on my Debian system it is the operating system creating
>>>>>>> initrd
>>>>>>> and defining which initrd matches which kernel. GRUB simply
>>>>>>> assumes that
>>>>>>> files ending on the same version number match. Therefore I would say
>>>>>>> Linux hopes that GRUB loads what Linux intended.
>>>>>>>
>>>>>>> The chain of trust would not be broken if the kernel were
>>>>>>> responsible
>>>>>>> for loading the initrd and for checking if it matches the kernel.
>>>>>>> Linux
>>>>>>> already does this for the kernel modules in initrd.
>>>>>>>
>>>>>> We can still sign the initrd and Linux can verify the signature. What
>>>>>> I am after is an interface that does not require the initrd to
>>>>>> originate from a EFI file system protocol, and which doesn't require
>>>>>> the loaded initrd to sit in memory for an unspecified amount of time
>>>>>> and its information passed via DT properties or bootparams structs.
>>>>>>
>>>>>> So invoking EFI_FILE_PROTOCOL directly is not going to work,
>>>>>> regardless of whether we get the devicepath from the command line or
>>>>>> from a EFI variable.
>>>>> What do you mean by "is not going to work"?
>>>>>
>>>>> With the device path you can find the handle implementing the
>>>>> EFI_SIMPLE_FIL_SYSTEM_PROTOCOL.
>>>>>
>>>>>>>> For u-boot, it should be trivial to implement a simple LoadFile2
>>>>>>>> protocol wrapper around EFI_FILE_PROTOCOL that can be installed
>>>>>>>> on a
>>>>>>>> handle that also carries EFI_FILE_PROTOCOL.
>>>>>>>>
>>>>>>> A U-Boot implementation of the EFI_LOAD_FILE2_PROTOCOL would need a
>>>>>>> device path variable to find the block device and to open the
>>>>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL before accessing the file.
>>>>>>>
>>>>>>> Linux would not be needing more lines and we would not repeat the
>>>>>>> same
>>>>>>> code in GRUB, U-Boot, EDK2, etc.
>>>>>>>
>>>>>>> As said Linux updates the initrd often. If that file is not
>>>>>>> signed by
>>>>>>> Linux in a well defined way, do not expect any security at all.
>>>>>>>
>>>>>> It is not only about security. The primary goal is to remove the need
>>>>>> for arch specific knowledge in the firmware about DT, bootparams and
>>>>>> initrd allocation policies without being forced to load the initrd
>>>>>> from a filesystem that is exposed via a EFI protocol.
>>>>> Where are device-trees touched by this patch?
>>>>>
>>>>> When booting via UEFI there is no need for knowledge of initrd
>>>>> allocation policies in U-Boot because up to now Linux or GRUB or iPXE
>>>>> load initrd.
>>>>>
>>>>> Furthermore I need no knowledge of bootparams in U-Boot once we
>>>>> properly
>>>>> we support UEFI variables at runtime because grub-update will pass the
>>>>> command line in one of the Bootxxxx UEFI variables.
>>>>>
>>>>> But most importantly I do not have to implement anything Linux
>>>>> specific
>>>>> in U-Boot for booting via UEFI up to now.
>>>>>
>>>> Adding Linux specific stuff to u-boot is arguably more appropriate
>>>> than adding architecture specific stuff to EFI loaders that could
>>>> otherwise be entirely generic.
>>>>
>>>> ...
>>>>> Your patch claims to fend off a specific threat scenario: A user
>>>>> puts an
>>>>> untrusted initrd on the disk and references it in the Linux command
>>>>> line.
>>>>>
>>>>> If he is able to do so with your current bootloader (signed or not
>>>>> signed), he most probably will also be able to delete a good initrd
>>>>> from
>>>>> the filesystem and thus force your code into the unsafe path.
>>>>>
>>>>> That is why I say that with the current fallback logic this patch
>>>>> achieves no increase in security. Of cause you could remove the
>>>>> fallback
>>>>> logic. But in this case your Linux will not boot with any legacy
>>>>> bootloader or firmware.
>>>>>
>>>> If there is a better way to expose the initrd that
>>>> a) does not require the initrd to reside on a file system that is
>>>> accessible via EFI protocols, and
>>>> b) does not require the loader to know about arch specific policies
>>>> regarding the placement of the initrd in memory, and
>>>> c) does not leave a time window between the time that the initrd is
>>>> loaded/verified/measured by the firmware and the time that the kernel
>>>> gets handed the buffer
>>>>
>>>> then I am happy to discuss it. This proposal is the best I could come
>>>> up with to achieve the above.
>>>>
>>> Hello Ard,
>>>
>>> I think part of our different views is that we are thinking about two
>>> different use cases which both have their relevance:
>>>
>>> If I understand you correctly, you are thinking about an embedded device
>>> where the kernel and the initrd is essentially part of the firmware
>>> provided by the device.
>>>
>>> I am thinking of a system running a standard Linux distribution like
>>> Debian where the initrd is generated by the operating system
>>>
>>> In both use cases verifying the initrd is of importance.
>>>
>>> Now concerning the requirements:
>>>
>>> a) In U-Boot all file systems on block devices can be made accessible
>>> via EFI protocols. Are you thinking about initrds that are not in a file
>>> system?
>>>
>> The typical GRUB deployment keeps the core GRUB itself (or the entire
>> thing if it is built as standalone) in the ESP, and the GRUB modules,
>> kernel images and initrds are in /boot, which is typically not a file
>> system that EFI understands. So in that case, initrd= does not work,
>> which is why GRUB loads the initrd into memory directly and passes the
>> base address and size via DT or bootparams structure.
>>
>>> b) My suggestion to use a UEFI variable for communicating the device
>>> path would not require any arch specific policies either.
>>>
>> Passing the EFI device path is not going to help us since the initrd
>> may not be representable as a device path. That is the whole point,
>> actually - this series makes the initrd representable as a device
>> path, but in a simple way that doesn't rely on EFI_FILE_PROTOCOL but
>> only on EFI_LOAD_FILE2_PROTOCOL, which is *much* simpler.
>
>
> So if we had support in grub to just export its own file systems as UEFI
> protocols, that problem would disappear, right? What other reasons are
> left to not just use normal file load operations from the Linux EFI stub?
>
> IIRC you mentioned that the fwcfg -kernel and -initrd parameters are
> already exposed as pseudo filesystem inside AAVMF, so that one is solved.
>
> I think that only leaves the UEFI shell case? But if you have a UEFI
> shell, then you can only load from an existing file system as well, no?
>
> What I can't quite grasp yet is how you would handle multiple initrds
> with a single device path. How would that work?

I guess you refer to the feature described in
https://www.phoronix.com/scan.php?page=news_item&px=GRUB-Multiple-Early-Initrd

In the binary form of an EFI_DEVICE_PATH_PROTOCOL multiple device paths
can be concatenated using a separator node of type "End of Hardware
Device Path node" (0x7f) with sub-type "End This Instance of a Device
Path" (0x01).

Regards

Heinrich

>
>
>>
>>> c) I proposed that the kernel does the verification. So there would be
>>> equally nothing in between loading the file and its verification. Yet
>>> responsibilities would be changed.
>>>
>>> But possibly I missed some requirements you have in mind that I should
>>> consider.
>>>
>> 1) The assumption that the initrd can always be loaded from a EFI
>> device path directly does not hold.
>
>
> Can you think of good reasons why this is true? I understand the grub
> one, but that's solvable. What other cases are there?
>
>
>> 2) Loading the initrd into memory and passing the address and size is
>> not acceptable.
>
>
> This would basically be the option to pass the initrd as configuration
> table, right? The only reason that definitely goes against that one that
> I can think of right now is to avoid double copying?
>
>
>> 3) Having a special device path that designates the initrd
>> specifically (which will be treated in a special way by the kernel)
>> makes it very easy for the boot firmware to attach policy to the
>> loading of the initrd that can be enforced right when the file is
>> being passed into the kernel.
>
>
> I don't fully understand this one. Can you give examples for such
> policies? :)
>
>
>> Putting an arbitrary device path in a EFI variable doesn't address 1),
>> and it complicates 3), since you cannot easily distinguish whether the
>> file load that is occurring is the EFI stub loading the initrd at
>> boot.
>
>
> Why do we need to distinguish? I'm missing creativity for the use case
> right now. For 1), we just need to make sure that boot loaders that
> implement their own file systems also expose those file systems as UEFI
> protocols, right?
>
>
> That said, I don't think the proposed approach of using
> EFI_LOAD_FILE2_PROTOCOL is bad. Whatever we do, we always will need to
> treat initrds special in one way or another; at least by exposing a
> command to specify which one(s) you want to load.
>
>
> Alex
>
>

Ard Biesheuvel Feb. 9, 2020, 11:35 a.m. UTC | #21

On Sun, 9 Feb 2020 at 07:50, Lukas Wunner <[e-mail]> wrote:
>
> On Thu, Feb 06, 2020 at 02:03:51PM +0000, Ard Biesheuvel wrote:
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -566,6 +566,14 @@ union efi_load_file_protocol {
> >       } mixed_mode;
> >  };
> >
> > +struct efi_vendor_dev_path {
> > +     u8              type;
> > +     u8              sub_type;
> > +     u16             length;
> > +     efi_guid_t      vendorguid;
> > +     u8              vendordata[];
> > +} __packed;
>
> In commit 46cd4b75cd0e ("efi: Add device path parser") I introduced a
> struct efi_dev_path with a union for the type-specific data.  Maybe
> you can make use of it instead of introducing a new type?
>

That would make sense, but how do you take the size then? Perhaps it
would be better to have a generic struct definition for the shared
fields, struct definitions for the different types that use the
generic type as the first fields, and a separate union that ties them
altogether for the parser.

>
> > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > @@ -323,3 +323,68 @@ void efi_char16_printk(efi_char16_t *str)
> >       efi_call_proto(efi_table_attr(efi_system_table(), con_out),
> >                      output_string, str);
> >  }
> > +
> > +static const struct {
> > +     struct efi_vendor_dev_path      vendor;
> > +     struct efi_generic_dev_path     end;
> > +} __packed initrd_devpath = {
>
> Nit:  Can we consistently stick to "dev_path" in lieu of "devpath"?
>

Sure.

Laszlo Ersek Feb. 10, 2020, 2:26 p.m. UTC | #22

On 02/07/20 13:36, Ard Biesheuvel wrote:
> On Fri, 7 Feb 2020 at 09:48, Laszlo Ersek <[e-mail]> wrote:
>> On 02/06/20 15:03, Ard Biesheuvel wrote:

>>> +efi_status_t efi_load_initrd_devpath(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_devpath;
>>> +     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;
>>> +
>>> +     initrd_size = 0;
>>> +     status = efi_call_proto(lf2, load_file,
>>> +                             (efi_device_path_protocol_t *)&initrd_devpath,
>>> +                             false, &initrd_size, NULL);
>>
>> The second argument to EFI_LOAD_FILE2_PROTOCOL.LoadFile() is "FilePath",
>> specified as "The device specific path of the file to load". This means
>> it is supposed to be a (possibly empty) sequence of FILEPATH_DEVICE_PATH
>> nodes, terminated by and "End Entire Device Path" node. See
>>
>> - 10.3.1 Generic Device Path Structures
>> - 10.3.5.4 File Path Media Device Path
>>
>> in UEFI-2.8.
>>
>> And "initrd_devpath" is not a device path like that; instead it's the
>> VenMedia device path that's installed on the handle that also carries
>> our LoadFile2 instance.
>>
> 
> OK, so you are saying this could be used to disambiguate which of
> several files you may want to load from the initrd GUIDed device path?

Yes, exactly.

>> Now, I do see that this all theoretical here, as we don't expect the
>> LoadFile2 instance that we've found via our special
>> LINUX_EFI_INITRD_MEDIA_GUID VenMedia devpath to do *any* device-specific
>> filename / pathname parsing.
>>
>> But in that case (i.e., given that the FilePath argument is totally
>> irrelevant), I think it's much clearer if we simply pass an empty device
>> path -- one that consists of a single "End Entire Device Path" node.
>>
>> I've checked, and your ArmVirtQemu patch ignores the FilePath argument
>> too -- justifiedly so. I just think it's better to pass in a well-formed
>> device path, rather than NULL. Because, the FilePath parameter is not
>> marked OPTIONAL in the spec.
>>
> 
> One thing that occurred to me is that we have to decide whether we
> want to support the '10.3.5.8 Relative Offset Range' device path node
> for this file, so that you could potentially load subranges of the
> file. I don't see a use case for it right now, though.

Agreed, it doesn't seem necessary / justified.

> But for my understanding, would the FilePath passed to LoadFile2 be
> 'Offset(...)+EndEntire' in that case? Or should it include the GUID
> device path node as well?

I see the only specified, concrete use case for the Offset() devpath
node in "14.4.2.1 PCI Bus Driver Responsibilities". I think it doesn't
apply at all to our use case.

Also, according to "10.3.5.8 Relative Offset Range",

    This device path node specifies a range of offsets relative to the
    first byte available on the device.

In that sense, it seems like a (mutually exclusive) alternative to
FilePath. Given a device, one would specify *either* an offset range
(which is relative to the start of the device, when the device is viewed
as a range of bytes), *or* a FilePath (which is "relative" to the device
when viewed as a store of named files, but still not as a full-blown
random-access filesystem).

In brief, Offset() doesn't seem to apply in connection with LoadFile2,
at all. Certainly not in our particular use case, I'd suggest.

[...]

Thanks!
Laszlo

Patch

diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index c7b091f50e55..1db943c1ba2b 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -157,6 +157,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;
 
@@ -255,11 +256,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, ULONG_MAX,
-				 efi_get_max_initrd_addr(dram_base, *image_addr),
-				 &initrd_addr, &initrd_size);
+	max_addr = efi_get_max_initrd_addr(dram_base, *image_addr);
+	status = efi_load_initrd_devpath(&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, ULONG_MAX, max_addr,
+					 &initrd_addr, &initrd_size);
+		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 8e60a39df3c5..eaf45ea749b3 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -323,3 +323,68 @@  void efi_char16_printk(efi_char16_t *str)
 	efi_call_proto(efi_table_attr(efi_system_table(), con_out),
 		       output_string, str);
 }
+
+static const struct {
+	struct efi_vendor_dev_path	vendor;
+	struct efi_generic_dev_path	end;
+} __packed initrd_devpath = {
+	{
+		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_status_t efi_load_initrd_devpath(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_devpath;
+	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;
+
+	initrd_size = 0;
+	status = efi_call_proto(lf2, load_file,
+				(efi_device_path_protocol_t *)&initrd_devpath,
+				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,
+				(efi_device_path_protocol_t *)&initrd_devpath,
+				false, &initrd_size, (void *)initrd_addr);
+	if (status != EFI_SUCCESS) {
+		efi_free(initrd_size, initrd_addr);
+		return status;
+	}
+
+	*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 99e93fd76ec5..fbf9f9442eed 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -566,6 +566,14 @@  union efi_load_file_protocol {
 	} mixed_mode;
 };
 
+struct efi_vendor_dev_path {
+	u8		type;
+	u8		sub_type;
+	u16		length;
+	efi_guid_t	vendorguid;
+	u8		vendordata[];
+} __packed;
+
 void efi_pci_disable_bridge_busmaster(void);
 
 typedef efi_status_t (*efi_exit_boot_map_processing)(
@@ -651,4 +659,8 @@  efi_status_t efi_load_initrd(efi_loaded_image_t *image,
 			     unsigned long *load_addr,
 			     unsigned long *load_size);
 
+efi_status_t efi_load_initrd_devpath(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 f3e2ff31b624..7f38f95676dd 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -419,9 +419,20 @@  efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 	if (status != EFI_SUCCESS)
 		goto fail2;
 
-	status = efi_load_initrd(image, hdr->initrd_addr_max,
-				 above4g ? ULONG_MAX : hdr->initrd_addr_max,
-				 &ramdisk_addr, &ramdisk_size);
+	/*
+	 * The initrd loaded from the Linux initrd vendor device
+	 * path should take precedence, as we don't want the
+	 * [unverified] command line to override the initrd
+	 * supplied by the [potentially verified] firmware.
+	 */
+	status = efi_load_initrd_devpath(&ramdisk_addr, &ramdisk_size,
+					 above4g ? ULONG_MAX
+						 : hdr->initrd_addr_max);
+	if (status == EFI_NOT_FOUND)
+		status = efi_load_initrd(image, hdr->initrd_addr_max,
+					 above4g ? ULONG_MAX
+						 : hdr->initrd_addr_max,
+					 &ramdisk_addr, &ramdisk_size);
 	if (status != EFI_SUCCESS)
 		goto fail2;
 	hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
@@ -732,6 +743,25 @@  struct boot_params *efi_main(efi_handle_t handle,
 			 ((u64)boot_params->ext_cmd_line_ptr << 32));
 	efi_parse_options((char *)cmdline_paddr);
 
+	if (!hdr->ramdisk_size && !boot_params->ext_ramdisk_size) {
+		unsigned long max = hdr->initrd_addr_max;
+		unsigned long addr, size;
+
+		if (hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G)
+			max = ULONG_MAX;
+
+		status = efi_load_initrd_devpath(&addr, &size, max);
+		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_devpath() 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 9ccf313fe9de..75c83c322c40 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)