[RFC PATCH 0/3] efi: MMC proxy support for the UEFI varstore

[RFC PATCH 0/3] efi: MMC proxy support for the UEFI varstore

[Ard Biesheuvel]

================================================================================
NOTE: this is a work in progress, and not fully functional yet. In particular,
the actual MMC host protocol methods are stubbed out at the moment, and need to
be wired up to the Linux device drivers.
================================================================================

On mobile and embedded systems, there is usually only a single MMC device for
non-volatile storage, which sits behind a controller that is owned by the OS at
runtime. This makes it difficult to host the UEFI variable store on MMC as well,
since the UEFI runtime services routines expect ownership of the underlying
device as well.

This series proposes an approach to work around this. It implements the UEFI
MMC host protocol in the kernel, in a way that makes it possible to expose it
to the firmware. At the same time, the firmware needs be set up for this, i.e.,
it needs to expose its MMC host protocol pointer via a UEFI configuration table,
so that the kernel can override it if it decides to expose this functionality
to the firmware.

Note that these patches are based on patches in the EFI tree that are queued
for v4.9, which replace the runtime wrappers spinlock with a semaphore. This
allows us to sleep in the firmware callbacks.

Prerequisites for using these patches:

* qemu-system-aarch64 built from this branch:
  https://git.linaro.org/people/ard.biesheuvel/qemu.git/shortlog/refs/heads/mach-virt-pl181
  which adds a PL181 SD/MMC controller to the mach-virt model, and exposes it
  via the device tree. It also sets the 'linux,uefi-varstore' property on this
  node.

* UEFI firmware built from this branch:
  https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/mmc-proxy
  
  Build using the following command
  build -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemuMmcVars.dsc

  Run using 
  qemu-system-aarch64 \
    -M virt -cpu cortex-a57 -m 2048 \
    -device virtio-blk-device,drive=boot \
    -drive if=none,id=boot,file=fat:<path-of-Image>,format=raw \
    -kernel <edk2-dir>/Build/ArmVirtQemuMmcVars-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd \
    -sd <edk2-dir>/build/edk2/Build/ArmVirtQemuMmcVars-AARCH64/DEBUG_GCC5/FV/QEMU_VARS.fd \
    -nographic $@

  This will give you an UEFI environment which keeps its UEFI variables in the
  emulated MMC volume, and exposes its MMC host protocol in a way that allows
  these patches to hook into it.

Patches #1 and #2 implement the arch specific hooks to preserve/restore the NEON
registers that the firmware may expect to be preserved across function calls.

Patch #3 implements the plumbing to call back into the kernel from the firmware.

Please comment on whether this approach seems feasible, and in particular, how
on earth I should wire this up to the actual MMC code.

Thanks,
Ard.

Ard Biesheuvel (3):
  efi/arm64: add SIMD stash/unstash operations
  efi/arm: add SIMD stash/unstash operations
  efi: implement MMC proxy support for the UEFI variable store

 arch/arm/include/asm/efi.h       |  11 +
 arch/arm64/include/asm/efi.h     |  33 +++
 drivers/firmware/efi/Kconfig     |   9 +
 drivers/firmware/efi/Makefile    |   1 +
 drivers/firmware/efi/arm-init.c  |   2 +
 drivers/firmware/efi/mmc-proxy.c | 222 ++++++++++++++++++++
 include/linux/efi.h              |   1 +
 7 files changed, 279 insertions(+)
 create mode 100644 drivers/firmware/efi/mmc-proxy.c

-- 
2.7.4

[RFC PATCH 1/3] efi/arm64: add SIMD stash/unstash operations

[Ard Biesheuvel]

This adds the SIMD stash/unstash operations that we need to allow UEFI
runtime services calls to call back into the kernel.

The UEFI bindings for arm64 stipulate that the ordinary AAPCS rules apply,
which means that code may use FP/NEON registers, and may also expect that
the values of the callee saved registers q8 - q15 are preserved across
function calls.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/efi.h | 33 ++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index a9e54aad15ef..d6bbbcba9820 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -81,4 +81,37 @@ static inline void efi_set_pgd(struct mm_struct *mm)
 void efi_virtmap_load(void);
 void efi_virtmap_unload(void);
 
+struct efi_simd_reg_stash {
+	u8	q8[16];
+	u8	q9[16];
+	u8	q10[16];
+	u8	q11[16];
+	u8	q12[16];
+	u8	q13[16];
+	u8	q14[16];
+	u8	q15[16];
+};
+
+static inline void arch_efi_stash_simd_regs(struct efi_simd_reg_stash *stash)
+{
+	asm(	"stp	q8,  q9,  [%1];"
+		"stp	q10, q11, [%1, #32];"
+		"stp	q12, q13, [%1, #64];"
+		"stp	q14, q15, [%1, #96];"
+
+		: "=m"(*stash)
+		: "r"(stash));
+}
+
+static inline void arch_efi_unstash_simd_regs(struct efi_simd_reg_stash *stash)
+{
+	asm(	"ldp	q8,  q9,  [%1];"
+		"ldp	q10, q11, [%1, #32];"
+		"ldp	q12, q13, [%1, #64];"
+		"ldp	q14, q15, [%1, #96];"
+
+		:
+		: "m"(*stash), "r"(stash));
+}
+
 #endif /* _ASM_EFI_H */
-- 
2.7.4

[RFC PATCH 2/3] efi/arm: add SIMD stash/unstash operations

[Ard Biesheuvel]

This adds the SIMD stash/unstash operations that we need to allow UEFI
runtime services calls to call back into the kernel. In the ARM case,
these are actually NOPs, since UEFI on ARM is not allowed to use the
FP/NEON register file.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/include/asm/efi.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index 766bf9b78160..c71ea8c21d0d 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -83,4 +83,15 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
 #define MIN_ZIMAGE_OFFSET	MAX_UNCOMP_KERNEL_SIZE
 #define MAX_FDT_OFFSET		ZIMAGE_OFFSET_LIMIT
 
+struct efi_simd_reg_stash {
+};
+
+static inline void arch_efi_stash_simd_regs(struct efi_simd_reg_stash *stash)
+{
+}
+
+static inline void arch_efi_unstash_simd_regs(struct efi_simd_reg_stash *stash)
+{
+}
+
 #endif /* _ASM_ARM_EFI_H */
-- 
2.7.4

[RFC PATCH 3/3] efi: implement MMC proxy support for the UEFI variable store

[Ard Biesheuvel]

---
 drivers/firmware/efi/Kconfig     |   9 +
 drivers/firmware/efi/Makefile    |   1 +
 drivers/firmware/efi/arm-init.c  |   2 +
 drivers/firmware/efi/mmc-proxy.c | 222 ++++++++++++++++++++
 include/linux/efi.h              |   1 +
 5 files changed, 235 insertions(+)

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index c981be17d3c0..1933e186a9c2 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -129,6 +129,15 @@ config EFI_TEST
 	  Say Y here to enable the runtime services support via /dev/efi_test.
 	  If unsure, say N.
 
+config EFI_MMC_PROXY
+	bool "Expose Embedded MMC host protocol to the firmware"
+	depends on EFI && (ARM || ARM64)
+	help
+	  This driver exposes the MMC host whose DT node is annotated with the
+	  "linux,uefi-varstore" property to the firmware via a UEFI compatible
+	  protocol. This allows the EFI variable store to reside on an MMC
+	  volume that is owned by the OS at runtime.
+
 endmenu
 
 config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index c8a439f6d715..88fdf0e13d6b 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -26,3 +26,4 @@ arm-obj-$(CONFIG_EFI)			:= arm-init.o arm-runtime.o
 obj-$(CONFIG_ARM)			+= $(arm-obj-y)
 obj-$(CONFIG_ARM64)			+= $(arm-obj-y)
 obj-$(CONFIG_EFI_CAPSULE_LOADER)	+= capsule-loader.o
+obj-$(CONFIG_EFI_MMC_PROXY)		+= mmc-proxy.o
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index 8efe13075c92..7a57253975a5 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -56,9 +56,11 @@ static phys_addr_t efi_to_phys(unsigned long addr)
 }
 
 static __initdata unsigned long screen_info_table = EFI_INVALID_TABLE_ADDR;
+unsigned long __initdata mmc_fv_emulation_table = EFI_INVALID_TABLE_ADDR;
 
 static __initdata efi_config_table_type_t arch_tables[] = {
 	{LINUX_EFI_ARM_SCREEN_INFO_TABLE_GUID, NULL, &screen_info_table},
+	{LINUX_MMC_FV_EMULATION_TABLE_GUID, NULL, &mmc_fv_emulation_table},
 	{NULL_GUID, NULL, NULL}
 };
 
diff --git a/drivers/firmware/efi/mmc-proxy.c b/drivers/firmware/efi/mmc-proxy.c
new file mode 100644
index 000000000000..3702bb2a5af0
--- /dev/null
+++ b/drivers/firmware/efi/mmc-proxy.c
@@ -0,0 +1,222 @@
+/*
+ * Copyright (C) 2016 Linaro Ltd. <ard.biesheuvel@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/efi.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+
+#include <asm/efi.h>
+
+extern unsigned long mmc_fv_emulation_table;
+
+enum emmc_state {
+	mmc_invalid_state = 0,
+	mmc_hw_initialization_state,
+	mmc_idle_state,
+	mmc_ready_state,
+	mmc_identification_state,
+	mmc_standby_state,
+	mmc_transfer_state,
+	mmc_sending_data_state,
+	mmc_receive_data_state,
+	mmc_programming_state,
+	mmc_disconnect_state,
+};
+
+static char const emmc_state_string[][28] = {
+	"mmc_invalid_state",
+	"mmc_hw_initialization_state",
+	"mmc_idle_state",
+	"mmc_ready_state",
+	"mmc_identification_state",
+	"mmc_standby_state",
+	"mmc_transfer_state",
+	"mmc_sending_data_state",
+	"mmc_receive_data_state",
+	"mmc_programming_state",
+	"mmc_disconnect_state",
+};
+
+/*
+ * This MMC host protocol version is not in the UEFI spec, and lives under
+ * EmbeddedPkg/ in Tianocore/EDK2.
+ */
+struct emmc_host_protocol
+{
+	u32		revision;
+	bool		(*is_card_present)(struct emmc_host_protocol*);
+	bool		(*is_read_only)(struct emmc_host_protocol*);
+	efi_status_t	(*build_device_path)(struct emmc_host_protocol*,
+					     void*);
+	efi_status_t	(*notify_state)(struct emmc_host_protocol*,
+					enum emmc_state);
+	efi_status_t	(*send_command)(struct emmc_host_protocol*,
+					u32, u32);
+	efi_status_t	(*receive_response)(struct emmc_host_protocol*,
+					    u32, u32*);
+	efi_status_t	(*read_block_data)(struct emmc_host_protocol*,
+					   u64, unsigned long, u32*);
+	efi_status_t	(*write_block_data)(struct emmc_host_protocol*,
+					    u64, unsigned long, u32*);
+};
+
+static bool mmc_is_card_present(struct emmc_host_protocol *this)
+{
+	return true;
+}
+
+static bool mmc_is_read_only(struct emmc_host_protocol *this)
+{
+	return false;
+}
+
+static efi_status_t mmc_build_device_path(struct emmc_host_protocol *this,
+					  void *dpp)
+{
+	return EFI_UNSUPPORTED;
+}
+
+static efi_status_t mmc_notify_state(struct emmc_host_protocol *this,
+				     enum emmc_state state)
+{
+	struct efi_simd_reg_stash simd_stash;
+
+	arch_efi_stash_simd_regs(&simd_stash);
+	arch_efi_call_virt_teardown();
+
+	pr_warn("mmc-proxy: entered %s state\n", emmc_state_string[state]);
+	might_sleep();
+
+	arch_efi_call_virt_setup();
+	arch_efi_unstash_simd_regs(&simd_stash);
+
+	return EFI_SUCCESS;
+}
+
+static efi_status_t mmc_send_command(struct emmc_host_protocol *this, u32 cmd,
+				     u32 arg)
+{
+	struct efi_simd_reg_stash simd_stash;
+
+	arch_efi_stash_simd_regs(&simd_stash);
+	arch_efi_call_virt_teardown();
+
+	pr_warn("mmc-proxy: send command %d (0x%x)\n", cmd, arg);
+	might_sleep();
+
+	arch_efi_call_virt_setup();
+	arch_efi_unstash_simd_regs(&simd_stash);
+	return EFI_NOT_FOUND;
+}
+
+static efi_status_t mmc_receive_response(struct emmc_host_protocol *this,
+					 u32 mmc_response_type, u32 *buffer)
+{
+	struct efi_simd_reg_stash simd_stash;
+	u32 response[4] = {};
+
+	arch_efi_stash_simd_regs(&simd_stash);
+	arch_efi_call_virt_teardown();
+
+	pr_warn("mmc-proxy: receive response %d\n", mmc_response_type);
+	might_sleep();
+
+	arch_efi_call_virt_setup();
+	arch_efi_unstash_simd_regs(&simd_stash);
+	memcpy(buffer, response, sizeof(response));
+
+	return EFI_NOT_FOUND;
+}
+
+static efi_status_t mmc_read_block_data(struct emmc_host_protocol *this,
+					u64 lba, unsigned long len,
+					u32 *buffer)
+{
+	struct efi_simd_reg_stash simd_stash;
+	void *bounce;
+
+	arch_efi_stash_simd_regs(&simd_stash);
+	arch_efi_call_virt_teardown();
+
+	bounce = kmalloc(len, GFP_KERNEL);
+
+	pr_warn("mmc-proxy: read block data lba==%lld, len==%ld\n", lba, len);
+	might_sleep();
+
+	arch_efi_call_virt_setup();
+	arch_efi_unstash_simd_regs(&simd_stash);
+	memcpy(buffer, bounce, len);
+	kfree(bounce);
+
+	return EFI_NOT_FOUND;
+}
+
+static efi_status_t mmc_write_block_data(struct emmc_host_protocol *this,
+					 u64 lba, unsigned long len,
+					 u32 *buffer)
+{
+	struct efi_simd_reg_stash simd_stash;
+	void *bounce;
+
+	/*
+	 * 'buffer' may point to memory that is mapped via the EFI page tables,
+	 * so copy the data before switching back to the ordinary mappings.
+	 */
+	bounce = kmalloc(len, GFP_ATOMIC);
+	if (!bounce)
+		return EFI_OUT_OF_RESOURCES;
+	memcpy(bounce, buffer, len);
+
+	arch_efi_stash_simd_regs(&simd_stash);
+	arch_efi_call_virt_teardown();
+
+	pr_warn("mmc-proxy: write block data lba==%lld, len==%ld\n", lba, len);
+	might_sleep();
+
+	kfree(bounce);
+	arch_efi_call_virt_setup();
+	arch_efi_unstash_simd_regs(&simd_stash);
+	return EFI_NOT_FOUND;
+}
+
+static const struct emmc_host_protocol emmc_proxy =
+{
+	0x00010001,		// revision 1.1
+	mmc_is_card_present,
+	mmc_is_read_only,
+	mmc_build_device_path,
+	mmc_notify_state,
+	mmc_send_command,
+	mmc_receive_response,
+	mmc_read_block_data,
+	mmc_write_block_data
+};
+
+static int __init emmc_proxy_install(void)
+{
+	unsigned long *p;
+
+	if (mmc_fv_emulation_table == EFI_INVALID_TABLE_ADDR)
+		return 0;
+
+	p = memremap(mmc_fv_emulation_table, sizeof(*p), MEMREMAP_WB);
+	if (!p)
+		return -ENOMEM;
+
+	/*
+	 * We expect a NULL value here, since the firmware should have cleared
+	 * this value in its ExitBootServices() handler.
+	 */
+	if (!WARN_ON(*p != 0))
+		*p = (unsigned long)&emmc_proxy;
+
+	memunmap(p);
+	return 0;
+}
+late_initcall(emmc_proxy_install);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2d089487d2da..25a4ae648b6f 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -599,6 +599,7 @@ void efi_native_runtime_setup(void);
  */
 #define LINUX_EFI_ARM_SCREEN_INFO_TABLE_GUID	EFI_GUID(0xe03fc20a, 0x85dc, 0x406e,  0xb9, 0x0e, 0x4a, 0xb5, 0x02, 0x37, 0x1d, 0x95)
 #define LINUX_EFI_LOADER_ENTRY_GUID		EFI_GUID(0x4a67b082, 0x0a4c, 0x41cf,  0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f)
+#define LINUX_MMC_FV_EMULATION_TABLE_GUID	EFI_GUID(0x44cd9912, 0xfda8, 0x4b7d,  0xa6, 0x13, 0x1b, 0x61, 0x34, 0xfc, 0xdc, 0x4a)
 
 typedef struct {
 	efi_guid_t guid;
-- 
2.7.4

[RFC PATCH 0/3] efi: MMC proxy support for the UEFI varstore

[Mark Rutland]

Hi Ard,

On Thu, Sep 22, 2016 at 12:30:03PM +0100, Ard Biesheuvel wrote:
> ================================================================================
> NOTE: this is a work in progress, and not fully functional yet. In particular,
> the actual MMC host protocol methods are stubbed out at the moment, and need to
> be wired up to the Linux device drivers.
> ================================================================================
> 
> On mobile and embedded systems, there is usually only a single MMC device for
> non-volatile storage, which sits behind a controller that is owned by the OS at
> runtime. This makes it difficult to host the UEFI variable store on MMC as well,
> since the UEFI runtime services routines expect ownership of the underlying
> device as well.
> 
> This series proposes an approach to work around this. It implements the UEFI
> MMC host protocol in the kernel, in a way that makes it possible to expose it
> to the firmware. At the same time, the firmware needs be set up for this, i.e.,
> it needs to expose its MMC host protocol pointer via a UEFI configuration table,
> so that the kernel can override it if it decides to expose this functionality
> to the firmware.

At a high level, and assuming a number of details from previous
discussions, I think the general approach of having the kernel mediate
access to the MMC makes sense.

However, I don't think that this series has enough detail for critical
review, even at the interface level. e.g. there is no mention of how
this caters for replay attacks and so on (which the current spec
sidesteps). I'm under the impression that there are mechanisms which
have been discussed for this, and I hope this is simply an oversight.

I also think that this needs to go via the USWG (and to the UEFI spec),
before we can consider using it. I say this because:

* This is critical to correct operation of variable storage as required
  for the standard boot flow. This is a major change to the way variable
  storage works today, with a number of (security) implications.

* There are others in this space trying to use the same class of
  hardware, e.g. FreeBSD. We don't want a Linux-specific interface, nor
  do we want a proliferation of interfaces for this purpose.

I have a few other general concerns:

* Identification of the relevant MMC device(s).
 
  Patch 3 suggests a devicetree property. I don't think that the
  'linux,' prefix makes sense, and it's not clear what we would do with
  ACPI.
 
  Does the MMC device not have some identifier we can query, so that we
  can match this up without requiring additional info in ACPI/DT?

* Lifetime guarantees
 
  When is it valid for EFI to call the MMC proxy? Can other services
  (e.g. ACPI) call this?

  How do we handle kexec/kdump? e.g. how do we teardown the interface
  before branching to a new kernel, how do we safely tear down a crashed
  kernel's interface, what can we call before doing so?

  How do we handle suspend/resume? e.g. is it necessary to re-register
  upon resume?

Thanks,
Mark.

> Note that these patches are based on patches in the EFI tree that are queued
> for v4.9, which replace the runtime wrappers spinlock with a semaphore. This
> allows us to sleep in the firmware callbacks.
> 
> Prerequisites for using these patches:
> 
> * qemu-system-aarch64 built from this branch:
>   https://git.linaro.org/people/ard.biesheuvel/qemu.git/shortlog/refs/heads/mach-virt-pl181
>   which adds a PL181 SD/MMC controller to the mach-virt model, and exposes it
>   via the device tree. It also sets the 'linux,uefi-varstore' property on this
>   node.
> 
> * UEFI firmware built from this branch:
>   https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/mmc-proxy
>   
>   Build using the following command
>   build -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemuMmcVars.dsc
> 
>   Run using 
>   qemu-system-aarch64 \
>     -M virt -cpu cortex-a57 -m 2048 \
>     -device virtio-blk-device,drive=boot \
>     -drive if=none,id=boot,file=fat:<path-of-Image>,format=raw \
>     -kernel <edk2-dir>/Build/ArmVirtQemuMmcVars-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd \
>     -sd <edk2-dir>/build/edk2/Build/ArmVirtQemuMmcVars-AARCH64/DEBUG_GCC5/FV/QEMU_VARS.fd \
>     -nographic $@
> 
>   This will give you an UEFI environment which keeps its UEFI variables in the
>   emulated MMC volume, and exposes its MMC host protocol in a way that allows
>   these patches to hook into it.
> 
> Patches #1 and #2 implement the arch specific hooks to preserve/restore the NEON
> registers that the firmware may expect to be preserved across function calls.
> 
> Patch #3 implements the plumbing to call back into the kernel from the firmware.
> 
> Please comment on whether this approach seems feasible, and in particular, how
> on earth I should wire this up to the actual MMC code.
> 
> Thanks,
> Ard.
> 
> Ard Biesheuvel (3):
>   efi/arm64: add SIMD stash/unstash operations
>   efi/arm: add SIMD stash/unstash operations
>   efi: implement MMC proxy support for the UEFI variable store
> 
>  arch/arm/include/asm/efi.h       |  11 +
>  arch/arm64/include/asm/efi.h     |  33 +++
>  drivers/firmware/efi/Kconfig     |   9 +
>  drivers/firmware/efi/Makefile    |   1 +
>  drivers/firmware/efi/arm-init.c  |   2 +
>  drivers/firmware/efi/mmc-proxy.c | 222 ++++++++++++++++++++
>  include/linux/efi.h              |   1 +
>  7 files changed, 279 insertions(+)
>  create mode 100644 drivers/firmware/efi/mmc-proxy.c
> 
> -- 
> 2.7.4
> 

[RFC PATCH 0/3] efi: MMC proxy support for the UEFI varstore

[Ard Biesheuvel]

(adding Charles whom I failed to cc)

On 22 September 2016 at 13:58, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Ard,
>
> On Thu, Sep 22, 2016 at 12:30:03PM +0100, Ard Biesheuvel wrote:
>> ================================================================================
>> NOTE: this is a work in progress, and not fully functional yet. In particular,
>> the actual MMC host protocol methods are stubbed out at the moment, and need to
>> be wired up to the Linux device drivers.
>> ================================================================================
>>
>> On mobile and embedded systems, there is usually only a single MMC device for
>> non-volatile storage, which sits behind a controller that is owned by the OS at
>> runtime. This makes it difficult to host the UEFI variable store on MMC as well,
>> since the UEFI runtime services routines expect ownership of the underlying
>> device as well.
>>
>> This series proposes an approach to work around this. It implements the UEFI
>> MMC host protocol in the kernel, in a way that makes it possible to expose it
>> to the firmware. At the same time, the firmware needs be set up for this, i.e.,
>> it needs to expose its MMC host protocol pointer via a UEFI configuration table,
>> so that the kernel can override it if it decides to expose this functionality
>> to the firmware.
>
> At a high level, and assuming a number of details from previous
> discussions, I think the general approach of having the kernel mediate
> access to the MMC makes sense.
>

OK.

> However, I don't think that this series has enough detail for critical
> review, even at the interface level. e.g. there is no mention of how
> this caters for replay attacks and so on (which the current spec
> sidesteps). I'm under the impression that there are mechanisms which
> have been discussed for this, and I hope this is simply an oversight.
>

This series does not take UEFI Secure Boot into account at all, and so
replay protection is not currently being addressed explicitly.
However, the reason for exposing an MMC host protocol rather than some
more abstract protocol (i.e., block I/O) is to allow firmware that
uses RPMB over MMC (e.g, with an authentication component residing in
the secure world) to reuse the same interface while maintaining the
same level of security it would have if it owned the MMC fully.

> I also think that this needs to go via the USWG (and to the UEFI spec),
> before we can consider using it. I say this because:
>
> * This is critical to correct operation of variable storage as required
>   for the standard boot flow. This is a major change to the way variable
>   storage works today, with a number of (security) implications.
>

The primary issue there is that the variable protocols are
architectural DXE protocols defined in the PI spec, not the UEFI spec
(and the MMC host protocol I am using here is not in any spec afaik),
and so implementing this in a UEFI compliant way is not currently
possible. Exposing a UEFI protocol like block I/O is not sufficient,
since the variable protocols are depended upon earlier in the boot
sequence (in UEFI implementations based on PI).

So yes, this needs to be discussed (and it will be, but not out in the
open, unfortunately.) However, it is unclear whether it is feasible to
address this at the UEFI level, or whether we will need to go beyond
and define something based on the PI spec directly. (/me looks at
Charles)

> * There are others in this space trying to use the same class of
>   hardware, e.g. FreeBSD. We don't want a Linux-specific interface, nor
>   do we want a proliferation of interfaces for this purpose.
>

No, we don't.

> I have a few other general concerns:
>
> * Identification of the relevant MMC device(s).
>
>   Patch 3 suggests a devicetree property. I don't think that the
>   'linux,' prefix makes sense, and it's not clear what we would do with
>   ACPI.
>
>   Does the MMC device not have some identifier we can query, so that we
>   can match this up without requiring additional info in ACPI/DT?
>

Good question. IIRC (but I'm a little rusty here), that is the case
for SD but not for MMC? (but it probably depends on the version) In
any case, there are other concerns here, regarding how the OS knows
which slice of the MMC is in use by the firmware. So the firmware
should expose sufficient information to the OS for it to figure out
which MMC volume it should expose to the firmware. UEFI generally uses
device paths for this, but I'm not sure if that is appropriate here.

> * Lifetime guarantees
>
>   When is it valid for EFI to call the MMC proxy? Can other services
>   (e.g. ACPI) call this?
>
>   How do we handle kexec/kdump? e.g. how do we teardown the interface
>   before branching to a new kernel, how do we safely tear down a crashed
>   kernel's interface, what can we call before doing so?
>
>   How do we handle suspend/resume? e.g. is it necessary to re-register
>   upon resume?
>

All good questions, and exactly the kind of feedback I am seeking.

Tearing down the interface could be as simple as clearing the pointer,
but some synchronization is probably in order to make sure that no
calls are in progress.

But to clarify the purpose of this series: are there any concerns
regarding exposing callbacks to the firmware in general, and for MMC
access in particular, from the Linux side? This code seems to work as
expected, but I may have missed something important (and I still need
help to actually wire it up to the MMC code)

-- 
Ard.

[RFC PATCH 0/3] efi: MMC proxy support for the UEFI varstore

[Mark Rutland]

On Thu, Sep 22, 2016 at 02:37:00PM +0100, Ard Biesheuvel wrote:
> On 22 September 2016 at 13:58, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Sep 22, 2016 at 12:30:03PM +0100, Ard Biesheuvel wrote:

> > I also think that this needs to go via the USWG (and to the UEFI spec),
> > before we can consider using it.

> The primary issue there is that the variable protocols are
> architectural DXE protocols defined in the PI spec, not the UEFI spec
> (and the MMC host protocol I am using here is not in any spec afaik),
> and so implementing this in a UEFI compliant way is not currently
> possible. Exposing a UEFI protocol like block I/O is not sufficient,
> since the variable protocols are depended upon earlier in the boot
> sequence (in UEFI implementations based on PI).
> 
> So yes, this needs to be discussed (and it will be, but not out in the
> open, unfortunately.) However, it is unclear whether it is feasible to
> address this at the UEFI level, or whether we will need to go beyond
> and define something based on the PI spec directly. (/me looks at
> Charles)

That is... less than optimal. :(

> > I have a few other general concerns:

> > * Lifetime guarantees
> >
> >   When is it valid for EFI to call the MMC proxy? Can other services
> >   (e.g. ACPI) call this?
> >
> >   How do we handle kexec/kdump? e.g. how do we teardown the interface
> >   before branching to a new kernel, how do we safely tear down a crashed
> >   kernel's interface, what can we call before doing so?
> >
> >   How do we handle suspend/resume? e.g. is it necessary to re-register
> >   upon resume?
> 
> All good questions, and exactly the kind of feedback I am seeking.
> 
> Tearing down the interface could be as simple as clearing the pointer,
> but some synchronization is probably in order to make sure that no
> calls are in progress.
> 
> But to clarify the purpose of this series: are there any concerns
> regarding exposing callbacks to the firmware in general, and for MMC
> access in particular, from the Linux side?

I have a general uneasiness about having UEFI call kernel function
pointers, inverting the usual relationship, but I'll need to think about
that a little further -- the lifetime stuff was the most obvious problem
in that area, but I'm sure there are more. ;)

It would really help to know *when* UEFI is expected/allowed to call
this. For synchronous calls, the FW could return a new
requires-os-support error code, and the OS could then call some API,
figure out what to do, and later send any acknowledgement required back
to UEFI. That might not cover all cases, though.

Thanks,
Mark.

[RFC PATCH 0/3] efi: MMC proxy support for the UEFI varstore

[Mark Rutland]

On Thu, Sep 22, 2016 at 01:58:54PM +0100, Mark Rutland wrote:
> On Thu, Sep 22, 2016 at 12:30:03PM +0100, Ard Biesheuvel wrote:

> > This series proposes an approach to work around this. It implements the UEFI
> > MMC host protocol in the kernel, in a way that makes it possible to expose it
> > to the firmware. At the same time, the firmware needs be set up for this, i.e.,
> > it needs to expose its MMC host protocol pointer via a UEFI configuration table,
> > so that the kernel can override it if it decides to expose this functionality
> > to the firmware.
> 
> At a high level, and assuming a number of details from previous
> discussions, I think the general approach of having the kernel mediate
> access to the MMC makes sense.

> I have a few other general concerns:

Another few thoughts I had:

* The UEFI spec mandates that a certain amount of stack space is available to
  runtime services which we call. If UEFI can call back into the kernel, it's
  not clear how much of the stack is available to either UEFI or the kernel.

  This is a rather fragile area -- kernel stack usage can vary wildly depending
  on configuration options.

* CPU state management. Runtime services can temporarily mask interrupts, and
  may require interrupts to remain masked over calls back into the kernel. That
  and other concerns mean that deadlock is going to be very difficult to avoid.

* It gets in the way of (though doesn't strictly prevent) sandboxing. One thing
  I'd like to do is run UEFI services in a more restricted environment (e.g. a
  VM if we have EL2 available), to aid robustness and debugging. Having a single
  entry/exit point, and not having to proxy calls would make this far simpler.

Overall, I'd far prefer that we have a strict one-way call policy (as is the
case today with UEFI), even if that means we have to make a number of calls to
achieve some functionality. In addition to the above, that also sidesteps the
lifetime issues I mentioned previously (modulo some FW-side state retained
across kexec and so on).

Thanks,
Mark.

[RFC PATCH 0/3] efi: MMC proxy support for the UEFI varstore

[Arnd Bergmann]

On Thursday 22 September 2016, Ard Biesheuvel wrote:
> ================================================================================
> NOTE: this is a work in progress, and not fully functional yet. In particular,
> the actual MMC host protocol methods are stubbed out at the moment, and need to
> be wired up to the Linux device drivers.
> ================================================================================
> 
> On mobile and embedded systems, there is usually only a single MMC device for
> non-volatile storage, which sits behind a controller that is owned by the OS at
> runtime. This makes it difficult to host the UEFI variable store on MMC as well,
> since the UEFI runtime services routines expect ownership of the underlying
> device as well.
> 
> This series proposes an approach to work around this. It implements the UEFI
> MMC host protocol in the kernel, in a way that makes it possible to expose it
> to the firmware. At the same time, the firmware needs be set up for this, i.e.,
> it needs to expose its MMC host protocol pointer via a UEFI configuration table,
> so that the kernel can override it if it decides to expose this functionality
> to the firmware.
> 
> Note that these patches are based on patches in the EFI tree that are queued
> for v4.9, which replace the runtime wrappers spinlock with a semaphore. This
> allows us to sleep in the firmware callbacks.

Would it be possible to implement the UEFI varstore more generally on top
of the Linux pstore interface and then have a pstore backend on top of
MMC? That could give us very similar behavior, but also a bit more flexibility.
It would be a bit confusing to have the 'dmesg' pstore target on top of
UEFI varstore which in turn is on top of pstore on MMC, but I think that's
ok as long as we prevent recursion.

	Arnd

[RFC PATCH 0/3] efi: MMC proxy support for the UEFI varstore

[Ard Biesheuvel]

On 28 September 2016 at 16:54, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 22 September 2016, Ard Biesheuvel wrote:
>> ================================================================================
>> NOTE: this is a work in progress, and not fully functional yet. In particular,
>> the actual MMC host protocol methods are stubbed out at the moment, and need to
>> be wired up to the Linux device drivers.
>> ================================================================================
>>
>> On mobile and embedded systems, there is usually only a single MMC device for
>> non-volatile storage, which sits behind a controller that is owned by the OS at
>> runtime. This makes it difficult to host the UEFI variable store on MMC as well,
>> since the UEFI runtime services routines expect ownership of the underlying
>> device as well.
>>
>> This series proposes an approach to work around this. It implements the UEFI
>> MMC host protocol in the kernel, in a way that makes it possible to expose it
>> to the firmware. At the same time, the firmware needs be set up for this, i.e.,
>> it needs to expose its MMC host protocol pointer via a UEFI configuration table,
>> so that the kernel can override it if it decides to expose this functionality
>> to the firmware.
>>
>> Note that these patches are based on patches in the EFI tree that are queued
>> for v4.9, which replace the runtime wrappers spinlock with a semaphore. This
>> allows us to sleep in the firmware callbacks.
>
> Would it be possible to implement the UEFI varstore more generally on top
> of the Linux pstore interface and then have a pstore backend on top of
> MMC? That could give us very similar behavior, but also a bit more flexibility.
> It would be a bit confusing to have the 'dmesg' pstore target on top of
> UEFI varstore which in turn is on top of pstore on MMC, but I think that's
> ok as long as we prevent recursion.
>

The reason for choosing MMC over block I/O or other more generic
interfaces is that it should also allow firmware implementations that
support UEFI secure boot using RPMB/MMC, where the authentication and
the RPMB related crypto are performed by a secure world component that
talks to the firmware directly.