| Message ID | 20210802014621.2280899-1-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | Revert "efi_capsule: Move signature from DTB to .rodata" | expand |
Hi Simon, On Sun, Aug 01, 2021 at 07:46:21PM -0600, Simon Glass wrote: > This was unfortunately applied despite much discussion about it being > the wrong way to implement this feature. No this was applied *before* the discussion, not despite. > > Revert it before too many other things are built on top of it. I don't really mind if this gets reverted but there's things that haven't been answered on that discussion [1] and my concern is what happens if CONFIG_OF_EMBED is not selected. Also you need to revert the entire series, not just one of the patches, as it changes the QEMU documentation for enabling authenticated capsule updates, as well as the mkeficapsule app. [1] https://lore.kernel.org/u-boot/YPna8Aiaoov6h50K@enceladus/ Regards /Ilias > > This reverts commit ddf67daac39de76d2697d587148f4c2cb768f492. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > board/emulation/common/Makefile | 1 + > board/emulation/common/qemu_capsule.c | 43 +++++++++++++++++++++++++++ > include/asm-generic/sections.h | 2 -- > lib/efi_loader/Kconfig | 7 ----- > lib/efi_loader/Makefile | 8 ----- > lib/efi_loader/efi_capsule.c | 18 ++--------- > lib/efi_loader/efi_capsule_key.S | 17 ----------- > 7 files changed, 47 insertions(+), 49 deletions(-) > create mode 100644 board/emulation/common/qemu_capsule.c > delete mode 100644 lib/efi_loader/efi_capsule_key.S > > diff --git a/board/emulation/common/Makefile b/board/emulation/common/Makefile > index c5b452e7e34..7ed447a69dc 100644 > --- a/board/emulation/common/Makefile > +++ b/board/emulation/common/Makefile > @@ -2,3 +2,4 @@ > > obj-$(CONFIG_SYS_MTDPARTS_RUNTIME) += qemu_mtdparts.o > obj-$(CONFIG_SET_DFU_ALT_INFO) += qemu_dfu.o > +obj-$(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) += qemu_capsule.o > diff --git a/board/emulation/common/qemu_capsule.c b/board/emulation/common/qemu_capsule.c > new file mode 100644 > index 00000000000..6b8a87022a4 > --- /dev/null > +++ b/board/emulation/common/qemu_capsule.c > @@ -0,0 +1,43 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2020 Linaro Limited > + */ > + > +#include <common.h> > +#include <efi_api.h> > +#include <efi_loader.h> > +#include <env.h> > +#include <fdtdec.h> > +#include <asm/global_data.h> > + > +DECLARE_GLOBAL_DATA_PTR; > + > +int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) > +{ > + const void *fdt_blob = gd->fdt_blob; > + const void *blob; > + const char *cnode_name = "capsule-key"; > + const char *snode_name = "signature"; > + int sig_node; > + int len; > + > + sig_node = fdt_subnode_offset(fdt_blob, 0, snode_name); > + if (sig_node < 0) { > + EFI_PRINT("Unable to get signature node offset\n"); > + return -FDT_ERR_NOTFOUND; > + } > + > + blob = fdt_getprop(fdt_blob, sig_node, cnode_name, &len); > + > + if (!blob || len < 0) { > + EFI_PRINT("Unable to get capsule-key value\n"); > + *pkey = NULL; > + *pkey_len = 0; > + return -FDT_ERR_NOTFOUND; > + } > + > + *pkey = (void *)blob; > + *pkey_len = len; > + > + return 0; > +} > diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h > index ec992b0c2e3..267f1db73f2 100644 > --- a/include/asm-generic/sections.h > +++ b/include/asm-generic/sections.h > @@ -27,8 +27,6 @@ extern char __efi_helloworld_begin[]; > extern char __efi_helloworld_end[]; > extern char __efi_var_file_begin[]; > extern char __efi_var_file_end[]; > -extern char __efi_capsule_sig_begin[]; > -extern char __efi_capsule_sig_end[]; > > /* Private data used by of-platdata devices/uclasses */ > extern char __priv_data_start[], __priv_data_end[]; > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > index dacc3b58810..7a469f22721 100644 > --- a/lib/efi_loader/Kconfig > +++ b/lib/efi_loader/Kconfig > @@ -214,13 +214,6 @@ config EFI_CAPSULE_AUTHENTICATE > Select this option if you want to enable capsule > authentication > > -config EFI_CAPSULE_KEY_PATH > - string "Path to .esl cert for capsule authentication" > - depends on EFI_CAPSULE_AUTHENTICATE > - help > - Provide the EFI signature list (esl) certificate used for capsule > - authentication > - > config EFI_DEVICE_PATH_TO_TEXT > bool "Device path to text protocol" > default y > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile > index 9b369430e25..fd344cea29b 100644 > --- a/lib/efi_loader/Makefile > +++ b/lib/efi_loader/Makefile > @@ -20,19 +20,11 @@ always += helloworld.efi > targets += helloworld.o > endif > > -ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE),y) > -EFI_CAPSULE_KEY_PATH := $(subst $\",,$(CONFIG_EFI_CAPSULE_KEY_PATH)) > -ifeq ("$(wildcard $(EFI_CAPSULE_KEY_PATH))","") > -$(error .esl cerificate not found. Configure your CONFIG_EFI_CAPSULE_KEY_PATH) > -endif > -endif > - > obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o > obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o > obj-y += efi_boottime.o > obj-y += efi_helper.o > obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o > -obj-$(CONFIG_EFI_CAPSULE_AUTHENTICATE) += efi_capsule_key.o > obj-$(CONFIG_EFI_CAPSULE_FIRMWARE) += efi_firmware.o > obj-y += efi_console.o > obj-y += efi_device_path.o > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > index 26990bc2df4..b75e4bcba1a 100644 > --- a/lib/efi_loader/efi_capsule.c > +++ b/lib/efi_loader/efi_capsule.c > @@ -16,7 +16,6 @@ > #include <mapmem.h> > #include <sort.h> > > -#include <asm/sections.h> > #include <crypto/pkcs7.h> > #include <crypto/pkcs7_parser.h> > #include <linux/err.h> > @@ -253,23 +252,12 @@ out: > > #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) > > -static int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) > -{ > - const void *blob = __efi_capsule_sig_begin; > - const int len = __efi_capsule_sig_end - __efi_capsule_sig_begin; > - > - *pkey = (void *)blob; > - *pkey_len = len; > - > - return 0; > -} > - > efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_size, > void **image, efi_uintn_t *image_size) > { > u8 *buf; > int ret; > - void *stored_pkey, *pkey; > + void *fdt_pkey, *pkey; > efi_uintn_t pkey_len; > uint64_t monotonic_count; > struct efi_signature_store *truststore; > @@ -322,7 +310,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s > goto out; > } > > - ret = efi_get_public_key_data(&stored_pkey, &pkey_len); > + ret = efi_get_public_key_data(&fdt_pkey, &pkey_len); > if (ret < 0) > goto out; > > @@ -330,7 +318,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s > if (!pkey) > goto out; > > - memcpy(pkey, stored_pkey, pkey_len); > + memcpy(pkey, fdt_pkey, pkey_len); > truststore = efi_build_signature_store(pkey, pkey_len); > if (!truststore) > goto out; > diff --git a/lib/efi_loader/efi_capsule_key.S b/lib/efi_loader/efi_capsule_key.S > deleted file mode 100644 > index 58f00b8e4bc..00000000000 > --- a/lib/efi_loader/efi_capsule_key.S > +++ /dev/null > @@ -1,17 +0,0 @@ > -/* SPDX-License-Identifier: GPL-2.0+ */ > -/* > - * .esl cert for capsule authentication > - * > - * Copyright (c) 2021, Ilias Apalodimas <ilias.apalodimas@linaro.org> > - */ > - > -#include <config.h> > - > -.section .rodata.capsule_key.init,"a" > -.balign 16 > -.global __efi_capsule_sig_begin > -__efi_capsule_sig_begin: > -.incbin CONFIG_EFI_CAPSULE_KEY_PATH > -__efi_capsule_sig_end: > -.global __efi_capsule_sig_end > -.balign 16 > -- > 2.32.0.554.ge1b32706d8-goog >
Hi Ilias, On Sun, 1 Aug 2021 at 20:28, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Sun, Aug 01, 2021 at 07:46:21PM -0600, Simon Glass wrote: > > This was unfortunately applied despite much discussion about it being > > the wrong way to implement this feature. > > No this was applied *before* the discussion, not despite. Oh sorry...I didn't notice either way. Normally there is an email on the patch saying it was applied. Perhaps I missed it. > > > > > Revert it before too many other things are built on top of it. > > I don't really mind if this gets reverted but there's things that haven't > been answered on that discussion [1] and my concern is what happens if > CONFIG_OF_EMBED is not selected. Can we start a new discussion perhaps? Or use one of the contributor calls to talk about it? We should not be using OF_EMBED except for testing. > > Also you need to revert the entire series, not just one of the patches, > as it changes the QEMU documentation for enabling authenticated capsule > updates, as well as the mkeficapsule app. Heinrich, do you have any thoughts on this? Regards, Simon > > [1] https://lore.kernel.org/u-boot/YPna8Aiaoov6h50K@enceladus/ > > Regards > /Ilias > > > > This reverts commit ddf67daac39de76d2697d587148f4c2cb768f492. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > board/emulation/common/Makefile | 1 + > > board/emulation/common/qemu_capsule.c | 43 +++++++++++++++++++++++++++ > > include/asm-generic/sections.h | 2 -- > > lib/efi_loader/Kconfig | 7 ----- > > lib/efi_loader/Makefile | 8 ----- > > lib/efi_loader/efi_capsule.c | 18 ++--------- > > lib/efi_loader/efi_capsule_key.S | 17 ----------- > > 7 files changed, 47 insertions(+), 49 deletions(-) > > create mode 100644 board/emulation/common/qemu_capsule.c > > delete mode 100644 lib/efi_loader/efi_capsule_key.S > > > > diff --git a/board/emulation/common/Makefile b/board/emulation/common/Makefile > > index c5b452e7e34..7ed447a69dc 100644 > > --- a/board/emulation/common/Makefile > > +++ b/board/emulation/common/Makefile > > @@ -2,3 +2,4 @@ > > > > obj-$(CONFIG_SYS_MTDPARTS_RUNTIME) += qemu_mtdparts.o > > obj-$(CONFIG_SET_DFU_ALT_INFO) += qemu_dfu.o > > +obj-$(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) += qemu_capsule.o > > diff --git a/board/emulation/common/qemu_capsule.c b/board/emulation/common/qemu_capsule.c > > new file mode 100644 > > index 00000000000..6b8a87022a4 > > --- /dev/null > > +++ b/board/emulation/common/qemu_capsule.c > > @@ -0,0 +1,43 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (c) 2020 Linaro Limited > > + */ > > + > > +#include <common.h> > > +#include <efi_api.h> > > +#include <efi_loader.h> > > +#include <env.h> > > +#include <fdtdec.h> > > +#include <asm/global_data.h> > > + > > +DECLARE_GLOBAL_DATA_PTR; > > + > > +int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) > > +{ > > + const void *fdt_blob = gd->fdt_blob; > > + const void *blob; > > + const char *cnode_name = "capsule-key"; > > + const char *snode_name = "signature"; > > + int sig_node; > > + int len; > > + > > + sig_node = fdt_subnode_offset(fdt_blob, 0, snode_name); > > + if (sig_node < 0) { > > + EFI_PRINT("Unable to get signature node offset\n"); > > + return -FDT_ERR_NOTFOUND; > > + } > > + > > + blob = fdt_getprop(fdt_blob, sig_node, cnode_name, &len); > > + > > + if (!blob || len < 0) { > > + EFI_PRINT("Unable to get capsule-key value\n"); > > + *pkey = NULL; > > + *pkey_len = 0; > > + return -FDT_ERR_NOTFOUND; > > + } > > + > > + *pkey = (void *)blob; > > + *pkey_len = len; > > + > > + return 0; > > +} > > diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h > > index ec992b0c2e3..267f1db73f2 100644 > > --- a/include/asm-generic/sections.h > > +++ b/include/asm-generic/sections.h > > @@ -27,8 +27,6 @@ extern char __efi_helloworld_begin[]; > > extern char __efi_helloworld_end[]; > > extern char __efi_var_file_begin[]; > > extern char __efi_var_file_end[]; > > -extern char __efi_capsule_sig_begin[]; > > -extern char __efi_capsule_sig_end[]; > > > > /* Private data used by of-platdata devices/uclasses */ > > extern char __priv_data_start[], __priv_data_end[]; > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > index dacc3b58810..7a469f22721 100644 > > --- a/lib/efi_loader/Kconfig > > +++ b/lib/efi_loader/Kconfig > > @@ -214,13 +214,6 @@ config EFI_CAPSULE_AUTHENTICATE > > Select this option if you want to enable capsule > > authentication > > > > -config EFI_CAPSULE_KEY_PATH > > - string "Path to .esl cert for capsule authentication" > > - depends on EFI_CAPSULE_AUTHENTICATE > > - help > > - Provide the EFI signature list (esl) certificate used for capsule > > - authentication > > - > > config EFI_DEVICE_PATH_TO_TEXT > > bool "Device path to text protocol" > > default y > > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile > > index 9b369430e25..fd344cea29b 100644 > > --- a/lib/efi_loader/Makefile > > +++ b/lib/efi_loader/Makefile > > @@ -20,19 +20,11 @@ always += helloworld.efi > > targets += helloworld.o > > endif > > > > -ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE),y) > > -EFI_CAPSULE_KEY_PATH := $(subst $\",,$(CONFIG_EFI_CAPSULE_KEY_PATH)) > > -ifeq ("$(wildcard $(EFI_CAPSULE_KEY_PATH))","") > > -$(error .esl cerificate not found. Configure your CONFIG_EFI_CAPSULE_KEY_PATH) > > -endif > > -endif > > - > > obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o > > obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o > > obj-y += efi_boottime.o > > obj-y += efi_helper.o > > obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o > > -obj-$(CONFIG_EFI_CAPSULE_AUTHENTICATE) += efi_capsule_key.o > > obj-$(CONFIG_EFI_CAPSULE_FIRMWARE) += efi_firmware.o > > obj-y += efi_console.o > > obj-y += efi_device_path.o > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > index 26990bc2df4..b75e4bcba1a 100644 > > --- a/lib/efi_loader/efi_capsule.c > > +++ b/lib/efi_loader/efi_capsule.c > > @@ -16,7 +16,6 @@ > > #include <mapmem.h> > > #include <sort.h> > > > > -#include <asm/sections.h> > > #include <crypto/pkcs7.h> > > #include <crypto/pkcs7_parser.h> > > #include <linux/err.h> > > @@ -253,23 +252,12 @@ out: > > > > #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) > > > > -static int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) > > -{ > > - const void *blob = __efi_capsule_sig_begin; > > - const int len = __efi_capsule_sig_end - __efi_capsule_sig_begin; > > - > > - *pkey = (void *)blob; > > - *pkey_len = len; > > - > > - return 0; > > -} > > - > > efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_size, > > void **image, efi_uintn_t *image_size) > > { > > u8 *buf; > > int ret; > > - void *stored_pkey, *pkey; > > + void *fdt_pkey, *pkey; > > efi_uintn_t pkey_len; > > uint64_t monotonic_count; > > struct efi_signature_store *truststore; > > @@ -322,7 +310,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s > > goto out; > > } > > > > - ret = efi_get_public_key_data(&stored_pkey, &pkey_len); > > + ret = efi_get_public_key_data(&fdt_pkey, &pkey_len); > > if (ret < 0) > > goto out; > > > > @@ -330,7 +318,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s > > if (!pkey) > > goto out; > > > > - memcpy(pkey, stored_pkey, pkey_len); > > + memcpy(pkey, fdt_pkey, pkey_len); > > truststore = efi_build_signature_store(pkey, pkey_len); > > if (!truststore) > > goto out; > > diff --git a/lib/efi_loader/efi_capsule_key.S b/lib/efi_loader/efi_capsule_key.S > > deleted file mode 100644 > > index 58f00b8e4bc..00000000000 > > --- a/lib/efi_loader/efi_capsule_key.S > > +++ /dev/null > > @@ -1,17 +0,0 @@ > > -/* SPDX-License-Identifier: GPL-2.0+ */ > > -/* > > - * .esl cert for capsule authentication > > - * > > - * Copyright (c) 2021, Ilias Apalodimas <ilias.apalodimas@linaro.org> > > - */ > > - > > -#include <config.h> > > - > > -.section .rodata.capsule_key.init,"a" > > -.balign 16 > > -.global __efi_capsule_sig_begin > > -__efi_capsule_sig_begin: > > -.incbin CONFIG_EFI_CAPSULE_KEY_PATH > > -__efi_capsule_sig_end: > > -.global __efi_capsule_sig_end > > -.balign 16 > > -- > > 2.32.0.554.ge1b32706d8-goog > >
On Sun, Aug 01, 2021 at 08:47:15PM -0600, Simon Glass wrote: > Hi Ilias, > > On Sun, 1 Aug 2021 at 20:28, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Simon, > > > > On Sun, Aug 01, 2021 at 07:46:21PM -0600, Simon Glass wrote: > > > This was unfortunately applied despite much discussion about it being > > > the wrong way to implement this feature. > > > > No this was applied *before* the discussion, not despite. > > Oh sorry...I didn't notice either way. Normally there is an email on > the patch saying it was applied. Perhaps I missed it. > > > > > > > > > Revert it before too many other things are built on top of it. > > > > I don't really mind if this gets reverted but there's things that haven't > > been answered on that discussion [1] and my concern is what happens if > > CONFIG_OF_EMBED is not selected. > > Can we start a new discussion perhaps? Or use one of the contributor > calls to talk about it? > > We should not be using OF_EMBED except for testing. > > > > > Also you need to revert the entire series, not just one of the patches, > > as it changes the QEMU documentation for enabling authenticated capsule > > updates, as well as the mkeficapsule app. > > Heinrich, do you have any thoughts on this? # I'm not Heinrich :) As far as the authentication logic itself is concerned, it is utterly generic except how and from where a public key is retrieved. (It can potentially be platform-specific.) Moreover, mkeficapsule really doesn't care where the key is. So I don't think we need revert all those changes. For testing, we can run a test on sandbox by having sandbox-specific efi_get_public_key_data() function, i.e. we may want to contain the key in a file on ESP or just in a specific flash partition. Obviously, it's not safe, but it's just a test to verify that the logic is sane. If the discussion goes on for an unexpected spell of time, I would like to take this workaround for now. -Takahiro Akashi > Regards, > Simon > > > > > [1] https://lore.kernel.org/u-boot/YPna8Aiaoov6h50K@enceladus/ > > > > Regards > > /Ilias > > > > > > This reverts commit ddf67daac39de76d2697d587148f4c2cb768f492. > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > --- > > > > > > board/emulation/common/Makefile | 1 + > > > board/emulation/common/qemu_capsule.c | 43 +++++++++++++++++++++++++++ > > > include/asm-generic/sections.h | 2 -- > > > lib/efi_loader/Kconfig | 7 ----- > > > lib/efi_loader/Makefile | 8 ----- > > > lib/efi_loader/efi_capsule.c | 18 ++--------- > > > lib/efi_loader/efi_capsule_key.S | 17 ----------- > > > 7 files changed, 47 insertions(+), 49 deletions(-) > > > create mode 100644 board/emulation/common/qemu_capsule.c > > > delete mode 100644 lib/efi_loader/efi_capsule_key.S > > > > > > diff --git a/board/emulation/common/Makefile b/board/emulation/common/Makefile > > > index c5b452e7e34..7ed447a69dc 100644 > > > --- a/board/emulation/common/Makefile > > > +++ b/board/emulation/common/Makefile > > > @@ -2,3 +2,4 @@ > > > > > > obj-$(CONFIG_SYS_MTDPARTS_RUNTIME) += qemu_mtdparts.o > > > obj-$(CONFIG_SET_DFU_ALT_INFO) += qemu_dfu.o > > > +obj-$(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) += qemu_capsule.o > > > diff --git a/board/emulation/common/qemu_capsule.c b/board/emulation/common/qemu_capsule.c > > > new file mode 100644 > > > index 00000000000..6b8a87022a4 > > > --- /dev/null > > > +++ b/board/emulation/common/qemu_capsule.c > > > @@ -0,0 +1,43 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* > > > + * Copyright (c) 2020 Linaro Limited > > > + */ > > > + > > > +#include <common.h> > > > +#include <efi_api.h> > > > +#include <efi_loader.h> > > > +#include <env.h> > > > +#include <fdtdec.h> > > > +#include <asm/global_data.h> > > > + > > > +DECLARE_GLOBAL_DATA_PTR; > > > + > > > +int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) > > > +{ > > > + const void *fdt_blob = gd->fdt_blob; > > > + const void *blob; > > > + const char *cnode_name = "capsule-key"; > > > + const char *snode_name = "signature"; > > > + int sig_node; > > > + int len; > > > + > > > + sig_node = fdt_subnode_offset(fdt_blob, 0, snode_name); > > > + if (sig_node < 0) { > > > + EFI_PRINT("Unable to get signature node offset\n"); > > > + return -FDT_ERR_NOTFOUND; > > > + } > > > + > > > + blob = fdt_getprop(fdt_blob, sig_node, cnode_name, &len); > > > + > > > + if (!blob || len < 0) { > > > + EFI_PRINT("Unable to get capsule-key value\n"); > > > + *pkey = NULL; > > > + *pkey_len = 0; > > > + return -FDT_ERR_NOTFOUND; > > > + } > > > + > > > + *pkey = (void *)blob; > > > + *pkey_len = len; > > > + > > > + return 0; > > > +} > > > diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h > > > index ec992b0c2e3..267f1db73f2 100644 > > > --- a/include/asm-generic/sections.h > > > +++ b/include/asm-generic/sections.h > > > @@ -27,8 +27,6 @@ extern char __efi_helloworld_begin[]; > > > extern char __efi_helloworld_end[]; > > > extern char __efi_var_file_begin[]; > > > extern char __efi_var_file_end[]; > > > -extern char __efi_capsule_sig_begin[]; > > > -extern char __efi_capsule_sig_end[]; > > > > > > /* Private data used by of-platdata devices/uclasses */ > > > extern char __priv_data_start[], __priv_data_end[]; > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > index dacc3b58810..7a469f22721 100644 > > > --- a/lib/efi_loader/Kconfig > > > +++ b/lib/efi_loader/Kconfig > > > @@ -214,13 +214,6 @@ config EFI_CAPSULE_AUTHENTICATE > > > Select this option if you want to enable capsule > > > authentication > > > > > > -config EFI_CAPSULE_KEY_PATH > > > - string "Path to .esl cert for capsule authentication" > > > - depends on EFI_CAPSULE_AUTHENTICATE > > > - help > > > - Provide the EFI signature list (esl) certificate used for capsule > > > - authentication > > > - > > > config EFI_DEVICE_PATH_TO_TEXT > > > bool "Device path to text protocol" > > > default y > > > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile > > > index 9b369430e25..fd344cea29b 100644 > > > --- a/lib/efi_loader/Makefile > > > +++ b/lib/efi_loader/Makefile > > > @@ -20,19 +20,11 @@ always += helloworld.efi > > > targets += helloworld.o > > > endif > > > > > > -ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE),y) > > > -EFI_CAPSULE_KEY_PATH := $(subst $\",,$(CONFIG_EFI_CAPSULE_KEY_PATH)) > > > -ifeq ("$(wildcard $(EFI_CAPSULE_KEY_PATH))","") > > > -$(error .esl cerificate not found. Configure your CONFIG_EFI_CAPSULE_KEY_PATH) > > > -endif > > > -endif > > > - > > > obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o > > > obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o > > > obj-y += efi_boottime.o > > > obj-y += efi_helper.o > > > obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o > > > -obj-$(CONFIG_EFI_CAPSULE_AUTHENTICATE) += efi_capsule_key.o > > > obj-$(CONFIG_EFI_CAPSULE_FIRMWARE) += efi_firmware.o > > > obj-y += efi_console.o > > > obj-y += efi_device_path.o > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > index 26990bc2df4..b75e4bcba1a 100644 > > > --- a/lib/efi_loader/efi_capsule.c > > > +++ b/lib/efi_loader/efi_capsule.c > > > @@ -16,7 +16,6 @@ > > > #include <mapmem.h> > > > #include <sort.h> > > > > > > -#include <asm/sections.h> > > > #include <crypto/pkcs7.h> > > > #include <crypto/pkcs7_parser.h> > > > #include <linux/err.h> > > > @@ -253,23 +252,12 @@ out: > > > > > > #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) > > > > > > -static int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) > > > -{ > > > - const void *blob = __efi_capsule_sig_begin; > > > - const int len = __efi_capsule_sig_end - __efi_capsule_sig_begin; > > > - > > > - *pkey = (void *)blob; > > > - *pkey_len = len; > > > - > > > - return 0; > > > -} > > > - > > > efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_size, > > > void **image, efi_uintn_t *image_size) > > > { > > > u8 *buf; > > > int ret; > > > - void *stored_pkey, *pkey; > > > + void *fdt_pkey, *pkey; > > > efi_uintn_t pkey_len; > > > uint64_t monotonic_count; > > > struct efi_signature_store *truststore; > > > @@ -322,7 +310,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s > > > goto out; > > > } > > > > > > - ret = efi_get_public_key_data(&stored_pkey, &pkey_len); > > > + ret = efi_get_public_key_data(&fdt_pkey, &pkey_len); > > > if (ret < 0) > > > goto out; > > > > > > @@ -330,7 +318,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s > > > if (!pkey) > > > goto out; > > > > > > - memcpy(pkey, stored_pkey, pkey_len); > > > + memcpy(pkey, fdt_pkey, pkey_len); > > > truststore = efi_build_signature_store(pkey, pkey_len); > > > if (!truststore) > > > goto out; > > > diff --git a/lib/efi_loader/efi_capsule_key.S b/lib/efi_loader/efi_capsule_key.S > > > deleted file mode 100644 > > > index 58f00b8e4bc..00000000000 > > > --- a/lib/efi_loader/efi_capsule_key.S > > > +++ /dev/null > > > @@ -1,17 +0,0 @@ > > > -/* SPDX-License-Identifier: GPL-2.0+ */ > > > -/* > > > - * .esl cert for capsule authentication > > > - * > > > - * Copyright (c) 2021, Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > - */ > > > - > > > -#include <config.h> > > > - > > > -.section .rodata.capsule_key.init,"a" > > > -.balign 16 > > > -.global __efi_capsule_sig_begin > > > -__efi_capsule_sig_begin: > > > -.incbin CONFIG_EFI_CAPSULE_KEY_PATH > > > -__efi_capsule_sig_end: > > > -.global __efi_capsule_sig_end > > > -.balign 16 > > > -- > > > 2.32.0.554.ge1b32706d8-goog > > >
Hi, On Mon, 2 Aug 2021 at 01:15, KASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > On Sun, Aug 01, 2021 at 08:47:15PM -0600, Simon Glass wrote: > > Hi Ilias, > > > > On Sun, 1 Aug 2021 at 20:28, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > On Sun, Aug 01, 2021 at 07:46:21PM -0600, Simon Glass wrote: > > > > This was unfortunately applied despite much discussion about it being > > > > the wrong way to implement this feature. > > > > > > No this was applied *before* the discussion, not despite. > > > > Oh sorry...I didn't notice either way. Normally there is an email on > > the patch saying it was applied. Perhaps I missed it. > > > > > > > > > > > > > Revert it before too many other things are built on top of it. > > > > > > I don't really mind if this gets reverted but there's things that haven't > > > been answered on that discussion [1] and my concern is what happens if > > > CONFIG_OF_EMBED is not selected. > > > > Can we start a new discussion perhaps? Or use one of the contributor > > calls to talk about it? > > > > We should not be using OF_EMBED except for testing. > > > > > > > > Also you need to revert the entire series, not just one of the patches, > > > as it changes the QEMU documentation for enabling authenticated capsule > > > updates, as well as the mkeficapsule app. > > > > Heinrich, do you have any thoughts on this? > > # I'm not Heinrich :) Perhaps you could impersonate him :-) I ask because he had been doing a lot of EFI work. > > As far as the authentication logic itself is concerned, > it is utterly generic except how and from where a public key is > retrieved. (It can potentially be platform-specific.) > Moreover, mkeficapsule really doesn't care where the key is. > > So I don't think we need revert all those changes. I agree. Having another look, I think perhaps three patches is enough. I will try again. > > For testing, we can run a test on sandbox by having sandbox-specific > efi_get_public_key_data() function, i.e. we may want to contain > the key in a file on ESP or just in a specific flash partition. > > Obviously, it's not safe, but it's just a test to verify that the logic > is sane. > > If the discussion goes on for an unexpected spell of time, > I would like to take this workaround for now. I think this effort should go back to before the change to putting things in rodata. That was when things went really off the rails. With things back in the DT, you should be able to write a test with the existing sandbox build without any special-case code. [..] Regards, Simon
On Mon, Aug 02, 2021 at 08:44:41AM -0600, Simon Glass wrote: > Hi, > > On Mon, 2 Aug 2021 at 01:15, KASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > On Sun, Aug 01, 2021 at 08:47:15PM -0600, Simon Glass wrote: > > > Hi Ilias, > > > > > > On Sun, 1 Aug 2021 at 20:28, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Hi Simon, > > > > > > > > On Sun, Aug 01, 2021 at 07:46:21PM -0600, Simon Glass wrote: > > > > > This was unfortunately applied despite much discussion about it being > > > > > the wrong way to implement this feature. > > > > > > > > No this was applied *before* the discussion, not despite. > > > > > > Oh sorry...I didn't notice either way. Normally there is an email on > > > the patch saying it was applied. Perhaps I missed it. > > > > > > > > > > > > > > > > > Revert it before too many other things are built on top of it. > > > > > > > > I don't really mind if this gets reverted but there's things that haven't > > > > been answered on that discussion [1] and my concern is what happens if > > > > CONFIG_OF_EMBED is not selected. > > > > > > Can we start a new discussion perhaps? Or use one of the contributor > > > calls to talk about it? > > > > > > We should not be using OF_EMBED except for testing. > > > > > > > > > > > Also you need to revert the entire series, not just one of the patches, > > > > as it changes the QEMU documentation for enabling authenticated capsule > > > > updates, as well as the mkeficapsule app. > > > > > > Heinrich, do you have any thoughts on this? > > > > # I'm not Heinrich :) > > Perhaps you could impersonate him :-) I ask because he had been doing > a lot of EFI work. I know. I was just kidding :) > > > > As far as the authentication logic itself is concerned, > > it is utterly generic except how and from where a public key is > > retrieved. (It can potentially be platform-specific.) > > Moreover, mkeficapsule really doesn't care where the key is. > > > > So I don't think we need revert all those changes. > > I agree. Having another look, I think perhaps three patches is enough. > I will try again. > > > > > For testing, we can run a test on sandbox by having sandbox-specific > > efi_get_public_key_data() function, i.e. we may want to contain > > the key in a file on ESP or just in a specific flash partition. > > > > Obviously, it's not safe, but it's just a test to verify that the logic > > is sane. > > > > If the discussion goes on for an unexpected spell of time, > > I would like to take this workaround for now. > > I think this effort should go back to before the change to putting > things in rodata. That was when things went really off the rails. > > With things back in the DT, you should be able to write a test with > the existing sandbox build without any special-case code. Well, that is the way I have adopted in my v1 patch[1]. I hope that the discussion be settled first. -Takahiro Akashi [1] https://lists.denx.de/pipermail/u-boot/2021-May/449575.html > [..] > > Regards, > Simon
Hi Takahiro, On Mon, 2 Aug 2021 at 18:21, KASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > On Mon, Aug 02, 2021 at 08:44:41AM -0600, Simon Glass wrote: > > Hi, > > > > On Mon, 2 Aug 2021 at 01:15, KASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > On Sun, Aug 01, 2021 at 08:47:15PM -0600, Simon Glass wrote: > > > > Hi Ilias, > > > > > > > > On Sun, 1 Aug 2021 at 20:28, Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > On Sun, Aug 01, 2021 at 07:46:21PM -0600, Simon Glass wrote: > > > > > > This was unfortunately applied despite much discussion about it being > > > > > > the wrong way to implement this feature. > > > > > > > > > > No this was applied *before* the discussion, not despite. > > > > > > > > Oh sorry...I didn't notice either way. Normally there is an email on > > > > the patch saying it was applied. Perhaps I missed it. > > > > > > > > > > > > > > > > > > > > > Revert it before too many other things are built on top of it. > > > > > > > > > > I don't really mind if this gets reverted but there's things that haven't > > > > > been answered on that discussion [1] and my concern is what happens if > > > > > CONFIG_OF_EMBED is not selected. > > > > > > > > Can we start a new discussion perhaps? Or use one of the contributor > > > > calls to talk about it? > > > > > > > > We should not be using OF_EMBED except for testing. > > > > > > > > > > > > > > Also you need to revert the entire series, not just one of the patches, > > > > > as it changes the QEMU documentation for enabling authenticated capsule > > > > > updates, as well as the mkeficapsule app. > > > > > > > > Heinrich, do you have any thoughts on this? > > > > > > # I'm not Heinrich :) > > > > Perhaps you could impersonate him :-) I ask because he had been doing > > a lot of EFI work. > > I know. I was just kidding :) > > > > > > > As far as the authentication logic itself is concerned, > > > it is utterly generic except how and from where a public key is > > > retrieved. (It can potentially be platform-specific.) > > > Moreover, mkeficapsule really doesn't care where the key is. > > > > > > So I don't think we need revert all those changes. > > > > I agree. Having another look, I think perhaps three patches is enough. > > I will try again. > > > > > > > > For testing, we can run a test on sandbox by having sandbox-specific > > > efi_get_public_key_data() function, i.e. we may want to contain > > > the key in a file on ESP or just in a specific flash partition. > > > > > > Obviously, it's not safe, but it's just a test to verify that the logic > > > is sane. > > > > > > If the discussion goes on for an unexpected spell of time, > > > I would like to take this workaround for now. > > > > I think this effort should go back to before the change to putting > > things in rodata. That was when things went really off the rails. > > > > With things back in the DT, you should be able to write a test with > > the existing sandbox build without any special-case code. > > Well, that is the way I have adopted in my v1 patch[1]. > I hope that the discussion be settled first. Yes, that looks a lot better. I cannot see why it was not applied at the time. > > -Takahiro Akashi > > [1] https://lists.denx.de/pipermail/u-boot/2021-May/449575.html > > Regards, SImon
diff --git a/board/emulation/common/Makefile b/board/emulation/common/Makefile index c5b452e7e34..7ed447a69dc 100644 --- a/board/emulation/common/Makefile +++ b/board/emulation/common/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_SYS_MTDPARTS_RUNTIME) += qemu_mtdparts.o obj-$(CONFIG_SET_DFU_ALT_INFO) += qemu_dfu.o +obj-$(CONFIG_EFI_CAPSULE_FIRMWARE_MANAGEMENT) += qemu_capsule.o diff --git a/board/emulation/common/qemu_capsule.c b/board/emulation/common/qemu_capsule.c new file mode 100644 index 00000000000..6b8a87022a4 --- /dev/null +++ b/board/emulation/common/qemu_capsule.c @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2020 Linaro Limited + */ + +#include <common.h> +#include <efi_api.h> +#include <efi_loader.h> +#include <env.h> +#include <fdtdec.h> +#include <asm/global_data.h> + +DECLARE_GLOBAL_DATA_PTR; + +int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) +{ + const void *fdt_blob = gd->fdt_blob; + const void *blob; + const char *cnode_name = "capsule-key"; + const char *snode_name = "signature"; + int sig_node; + int len; + + sig_node = fdt_subnode_offset(fdt_blob, 0, snode_name); + if (sig_node < 0) { + EFI_PRINT("Unable to get signature node offset\n"); + return -FDT_ERR_NOTFOUND; + } + + blob = fdt_getprop(fdt_blob, sig_node, cnode_name, &len); + + if (!blob || len < 0) { + EFI_PRINT("Unable to get capsule-key value\n"); + *pkey = NULL; + *pkey_len = 0; + return -FDT_ERR_NOTFOUND; + } + + *pkey = (void *)blob; + *pkey_len = len; + + return 0; +} diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h index ec992b0c2e3..267f1db73f2 100644 --- a/include/asm-generic/sections.h +++ b/include/asm-generic/sections.h @@ -27,8 +27,6 @@ extern char __efi_helloworld_begin[]; extern char __efi_helloworld_end[]; extern char __efi_var_file_begin[]; extern char __efi_var_file_end[]; -extern char __efi_capsule_sig_begin[]; -extern char __efi_capsule_sig_end[]; /* Private data used by of-platdata devices/uclasses */ extern char __priv_data_start[], __priv_data_end[]; diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index dacc3b58810..7a469f22721 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -214,13 +214,6 @@ config EFI_CAPSULE_AUTHENTICATE Select this option if you want to enable capsule authentication -config EFI_CAPSULE_KEY_PATH - string "Path to .esl cert for capsule authentication" - depends on EFI_CAPSULE_AUTHENTICATE - help - Provide the EFI signature list (esl) certificate used for capsule - authentication - config EFI_DEVICE_PATH_TO_TEXT bool "Device path to text protocol" default y diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index 9b369430e25..fd344cea29b 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -20,19 +20,11 @@ always += helloworld.efi targets += helloworld.o endif -ifeq ($(CONFIG_EFI_CAPSULE_AUTHENTICATE),y) -EFI_CAPSULE_KEY_PATH := $(subst $\",,$(CONFIG_EFI_CAPSULE_KEY_PATH)) -ifeq ("$(wildcard $(EFI_CAPSULE_KEY_PATH))","") -$(error .esl cerificate not found. Configure your CONFIG_EFI_CAPSULE_KEY_PATH) -endif -endif - obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o obj-y += efi_boottime.o obj-y += efi_helper.o obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o -obj-$(CONFIG_EFI_CAPSULE_AUTHENTICATE) += efi_capsule_key.o obj-$(CONFIG_EFI_CAPSULE_FIRMWARE) += efi_firmware.o obj-y += efi_console.o obj-y += efi_device_path.o diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 26990bc2df4..b75e4bcba1a 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -16,7 +16,6 @@ #include <mapmem.h> #include <sort.h> -#include <asm/sections.h> #include <crypto/pkcs7.h> #include <crypto/pkcs7_parser.h> #include <linux/err.h> @@ -253,23 +252,12 @@ out: #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) -static int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) -{ - const void *blob = __efi_capsule_sig_begin; - const int len = __efi_capsule_sig_end - __efi_capsule_sig_begin; - - *pkey = (void *)blob; - *pkey_len = len; - - return 0; -} - efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_size, void **image, efi_uintn_t *image_size) { u8 *buf; int ret; - void *stored_pkey, *pkey; + void *fdt_pkey, *pkey; efi_uintn_t pkey_len; uint64_t monotonic_count; struct efi_signature_store *truststore; @@ -322,7 +310,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s goto out; } - ret = efi_get_public_key_data(&stored_pkey, &pkey_len); + ret = efi_get_public_key_data(&fdt_pkey, &pkey_len); if (ret < 0) goto out; @@ -330,7 +318,7 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s if (!pkey) goto out; - memcpy(pkey, stored_pkey, pkey_len); + memcpy(pkey, fdt_pkey, pkey_len); truststore = efi_build_signature_store(pkey, pkey_len); if (!truststore) goto out; diff --git a/lib/efi_loader/efi_capsule_key.S b/lib/efi_loader/efi_capsule_key.S deleted file mode 100644 index 58f00b8e4bc..00000000000 --- a/lib/efi_loader/efi_capsule_key.S +++ /dev/null @@ -1,17 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0+ */ -/* - * .esl cert for capsule authentication - * - * Copyright (c) 2021, Ilias Apalodimas <ilias.apalodimas@linaro.org> - */ - -#include <config.h> - -.section .rodata.capsule_key.init,"a" -.balign 16 -.global __efi_capsule_sig_begin -__efi_capsule_sig_begin: -.incbin CONFIG_EFI_CAPSULE_KEY_PATH -__efi_capsule_sig_end: -.global __efi_capsule_sig_end -.balign 16
This was unfortunately applied despite much discussion about it being the wrong way to implement this feature. Revert it before too many other things are built on top of it. This reverts commit ddf67daac39de76d2697d587148f4c2cb768f492. Signed-off-by: Simon Glass <sjg@chromium.org> --- board/emulation/common/Makefile | 1 + board/emulation/common/qemu_capsule.c | 43 +++++++++++++++++++++++++++ include/asm-generic/sections.h | 2 -- lib/efi_loader/Kconfig | 7 ----- lib/efi_loader/Makefile | 8 ----- lib/efi_loader/efi_capsule.c | 18 ++--------- lib/efi_loader/efi_capsule_key.S | 17 ----------- 7 files changed, 47 insertions(+), 49 deletions(-) create mode 100644 board/emulation/common/qemu_capsule.c delete mode 100644 lib/efi_loader/efi_capsule_key.S