| Message ID | 20211007062340.72207-4-takahiro.akashi@linaro.org |
|---|---|
| State | Accepted, archived |
| Commit | 7a6fb28c8e4b03bc37b05936ae5fa4c16c278520 |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: capsule: improve capsule authentication support | expand |
Akashi-san, On Thu, Oct 07, 2021 at 03:23:32PM +0900, AKASHI Takahiro wrote: > The commit 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to > .rodata"") failed to revert the removal of efi_get_public_key_data(). > > Add back this function and move it under lib/efi_loader so that other > platforms can utilize it. It is now declared as a weak function so that > it can be replaced with a platform-specific implementation. > > Fixes: 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to > .rodata"") Yep I noticed the same thing a few days ago and told Heinrich, I'd fix that. Thanks! > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > lib/efi_loader/efi_capsule.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > index b75e4bcba1a9..44f5da61a9be 100644 > --- a/lib/efi_loader/efi_capsule.c > +++ b/lib/efi_loader/efi_capsule.c > @@ -11,15 +11,20 @@ > #include <common.h> > #include <efi_loader.h> > #include <efi_variable.h> > +#include <env.h> > +#include <fdtdec.h> > #include <fs.h> > #include <malloc.h> > #include <mapmem.h> > #include <sort.h> > +#include <asm/global_data.h> > > #include <crypto/pkcs7.h> > #include <crypto/pkcs7_parser.h> > #include <linux/err.h> > > +DECLARE_GLOBAL_DATA_PTR; > + > const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID; > static const efi_guid_t efi_guid_firmware_management_capsule_id = > EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > @@ -251,6 +256,37 @@ out: > } > > #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) > +int __weak 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) { > + log_err("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) { > + log_err("Unable to get capsule-key value\n"); > + *pkey = NULL; > + *pkey_len = 0; > + > + return -FDT_ERR_NOTFOUND; > + } > + > + *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) > -- > 2.33.0 > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Hi Takahiro, On Thu, 7 Oct 2021 at 00:25, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > The commit 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to > .rodata"") failed to revert the removal of efi_get_public_key_data(). > > Add back this function and move it under lib/efi_loader so that other > platforms can utilize it. It is now declared as a weak function so that > it can be replaced with a platform-specific implementation. > > Fixes: 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to > .rodata"") > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > lib/efi_loader/efi_capsule.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > index b75e4bcba1a9..44f5da61a9be 100644 > --- a/lib/efi_loader/efi_capsule.c > +++ b/lib/efi_loader/efi_capsule.c > @@ -11,15 +11,20 @@ > #include <common.h> > #include <efi_loader.h> > #include <efi_variable.h> > +#include <env.h> > +#include <fdtdec.h> > #include <fs.h> > #include <malloc.h> > #include <mapmem.h> > #include <sort.h> > +#include <asm/global_data.h> > > #include <crypto/pkcs7.h> > #include <crypto/pkcs7_parser.h> > #include <linux/err.h> > > +DECLARE_GLOBAL_DATA_PTR; > + > const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID; > static const efi_guid_t efi_guid_firmware_management_capsule_id = > EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > @@ -251,6 +256,37 @@ out: > } > > #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) > +int __weak efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) I don't think this should be weak. What other way is there of handling this and why would it be platform-specific? > +{ > + 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) { > + log_err("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) { > + log_err("Unable to get capsule-key value\n"); > + *pkey = NULL; > + *pkey_len = 0; > + > + return -FDT_ERR_NOTFOUND; > + } > + > + *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) > -- > 2.33.0 > Regards, Simon
Hi Simon, 2021年10月15日(金) 9:40 Simon Glass <sjg@chromium.org>: > > Hi Takahiro, > > On Thu, 7 Oct 2021 at 00:25, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > The commit 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to > > .rodata"") failed to revert the removal of efi_get_public_key_data(). > > > > Add back this function and move it under lib/efi_loader so that other > > platforms can utilize it. It is now declared as a weak function so that > > it can be replaced with a platform-specific implementation. > > > > Fixes: 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to > > .rodata"") > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > lib/efi_loader/efi_capsule.c | 36 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > index b75e4bcba1a9..44f5da61a9be 100644 > > --- a/lib/efi_loader/efi_capsule.c > > +++ b/lib/efi_loader/efi_capsule.c > > @@ -11,15 +11,20 @@ > > #include <common.h> > > #include <efi_loader.h> > > #include <efi_variable.h> > > +#include <env.h> > > +#include <fdtdec.h> > > #include <fs.h> > > #include <malloc.h> > > #include <mapmem.h> > > #include <sort.h> > > +#include <asm/global_data.h> > > > > #include <crypto/pkcs7.h> > > #include <crypto/pkcs7_parser.h> > > #include <linux/err.h> > > > > +DECLARE_GLOBAL_DATA_PTR; > > + > > const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID; > > static const efi_guid_t efi_guid_firmware_management_capsule_id = > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > @@ -251,6 +256,37 @@ out: > > } > > > > #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) > > +int __weak efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) > > I don't think this should be weak. What other way is there of handling > this and why would it be platform-specific? I have a question about the current design of the capsule auth key. If the platform has its own key-storage, how can the platform use the platform specific storage? Does such platform load the key from the storage and generate the dtb node in the platform initialization code? (or device driver?) Thank you, > > > +{ > > + 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) { > > + log_err("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) { > > + log_err("Unable to get capsule-key value\n"); > > + *pkey = NULL; > > + *pkey_len = 0; > > + > > + return -FDT_ERR_NOTFOUND; > > + } > > + > > + *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) > > -- > > 2.33.0 > > > > Regards, > Simon -- Masami Hiramatsu
Le mer. 20 oct. 2021 à 10:18, Masami Hiramatsu <masami.hiramatsu@linaro.org> a écrit : > Hi Simon, > > 2021年10月15日(金) 9:40 Simon Glass <sjg@chromium.org>: > > > > Hi Takahiro, > > > > On Thu, 7 Oct 2021 at 00:25, AKASHI Takahiro <takahiro.akashi@linaro.org> > wrote: > > > > > > The commit 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB > to > > > .rodata"") failed to revert the removal of efi_get_public_key_data(). > > > > > > Add back this function and move it under lib/efi_loader so that other > > > platforms can utilize it. It is now declared as a weak function so that > > > it can be replaced with a platform-specific implementation. > > > > > > Fixes: 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to > > > .rodata"") > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > --- > > > lib/efi_loader/efi_capsule.c | 36 ++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 36 insertions(+) > > > > > > diff --git a/lib/efi_loader/efi_capsule.c > b/lib/efi_loader/efi_capsule.c > > > index b75e4bcba1a9..44f5da61a9be 100644 > > > --- a/lib/efi_loader/efi_capsule.c > > > +++ b/lib/efi_loader/efi_capsule.c > > > @@ -11,15 +11,20 @@ > > > #include <common.h> > > > #include <efi_loader.h> > > > #include <efi_variable.h> > > > +#include <env.h> > > > +#include <fdtdec.h> > > > #include <fs.h> > > > #include <malloc.h> > > > #include <mapmem.h> > > > #include <sort.h> > > > +#include <asm/global_data.h> > > > > > > #include <crypto/pkcs7.h> > > > #include <crypto/pkcs7_parser.h> > > > #include <linux/err.h> > > > > > > +DECLARE_GLOBAL_DATA_PTR; > > > + > > > const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID; > > > static const efi_guid_t efi_guid_firmware_management_capsule_id = > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > > @@ -251,6 +256,37 @@ out: > > > } > > > > > > #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) > > > +int __weak efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) > > > > I don't think this should be weak. What other way is there of handling > > this and why would it be platform-specific? > > I have a question about the current design of the capsule auth key. > If the platform has its own key-storage, how can the platform use the > platform specific storage? Does such platform load the key from the storage > and generate the dtb node in the platform initialization code? (or > device driver?) it depends on what the capsule contains. If the capsule contains SCP firmware or secure firmware or TAs, U-Boot may not be even allowed to see the key. If the capsule contains U-Boot itself it may be again outside scope of U-Boot because that may be secure firmware that verifies the signature. We may allow U-Boot to update itself but the final say is the secure firmware that may prevent the boot. If the capsule contains device firmware then it may depend on the device: secure device U-Boot can do anything, otherwise then it is to be decided by U-Boot. > > Thank you, > > > > > > > +{ > > > + 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) { > > > + log_err("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) { > > > + log_err("Unable to get capsule-key value\n"); > > > + *pkey = NULL; > > > + *pkey_len = 0; > > > + > > > + return -FDT_ERR_NOTFOUND; > > > + } > > > + > > > + *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) > > > -- > > > 2.33.0 > > > > > > > Regards, > > Simon > > > > -- > Masami Hiramatsu >
Hi Masami, On Wed, 20 Oct 2021 at 02:18, Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote: > > Hi Simon, > > 2021年10月15日(金) 9:40 Simon Glass <sjg@chromium.org>: > > > > Hi Takahiro, > > > > On Thu, 7 Oct 2021 at 00:25, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > The commit 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to > > > .rodata"") failed to revert the removal of efi_get_public_key_data(). > > > > > > Add back this function and move it under lib/efi_loader so that other > > > platforms can utilize it. It is now declared as a weak function so that > > > it can be replaced with a platform-specific implementation. > > > > > > Fixes: 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to > > > .rodata"") > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > --- > > > lib/efi_loader/efi_capsule.c | 36 ++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 36 insertions(+) > > > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > index b75e4bcba1a9..44f5da61a9be 100644 > > > --- a/lib/efi_loader/efi_capsule.c > > > +++ b/lib/efi_loader/efi_capsule.c > > > @@ -11,15 +11,20 @@ > > > #include <common.h> > > > #include <efi_loader.h> > > > #include <efi_variable.h> > > > +#include <env.h> > > > +#include <fdtdec.h> > > > #include <fs.h> > > > #include <malloc.h> > > > #include <mapmem.h> > > > #include <sort.h> > > > +#include <asm/global_data.h> > > > > > > #include <crypto/pkcs7.h> > > > #include <crypto/pkcs7_parser.h> > > > #include <linux/err.h> > > > > > > +DECLARE_GLOBAL_DATA_PTR; > > > + > > > const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID; > > > static const efi_guid_t efi_guid_firmware_management_capsule_id = > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > > @@ -251,6 +256,37 @@ out: > > > } > > > > > > #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) > > > +int __weak efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) > > > > I don't think this should be weak. What other way is there of handling > > this and why would it be platform-specific? > > I have a question about the current design of the capsule auth key. > If the platform has its own key-storage, how can the platform use the > platform specific storage? Does such platform load the key from the storage > and generate the dtb node in the platform initialization code? (or > device driver?) Are we talking about a public key (which I assume from the function naming) or some secret key. What is an auth key? As I understand it, the public key should be provided by the platform in devicetree that U-Boot uses, by whatever prior stage has access to the key. Regards, Simon
On Wed, Oct 20, 2021 at 07:39:37AM -0600, Simon Glass wrote: > Hi Masami, > > On Wed, 20 Oct 2021 at 02:18, Masami Hiramatsu > <masami.hiramatsu@linaro.org> wrote: > > > > Hi Simon, > > > > 2021年10月15日(金) 9:40 Simon Glass <sjg@chromium.org>: > > > > > > Hi Takahiro, > > > > > > On Thu, 7 Oct 2021 at 00:25, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > > > > > > The commit 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to > > > > .rodata"") failed to revert the removal of efi_get_public_key_data(). > > > > > > > > Add back this function and move it under lib/efi_loader so that other > > > > platforms can utilize it. It is now declared as a weak function so that > > > > it can be replaced with a platform-specific implementation. > > > > > > > > Fixes: 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to > > > > .rodata"") > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > --- > > > > lib/efi_loader/efi_capsule.c | 36 ++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 36 insertions(+) > > > > > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > > > index b75e4bcba1a9..44f5da61a9be 100644 > > > > --- a/lib/efi_loader/efi_capsule.c > > > > +++ b/lib/efi_loader/efi_capsule.c > > > > @@ -11,15 +11,20 @@ > > > > #include <common.h> > > > > #include <efi_loader.h> > > > > #include <efi_variable.h> > > > > +#include <env.h> > > > > +#include <fdtdec.h> > > > > #include <fs.h> > > > > #include <malloc.h> > > > > #include <mapmem.h> > > > > #include <sort.h> > > > > +#include <asm/global_data.h> > > > > > > > > #include <crypto/pkcs7.h> > > > > #include <crypto/pkcs7_parser.h> > > > > #include <linux/err.h> > > > > > > > > +DECLARE_GLOBAL_DATA_PTR; > > > > + > > > > const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID; > > > > static const efi_guid_t efi_guid_firmware_management_capsule_id = > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > > > @@ -251,6 +256,37 @@ out: > > > > } > > > > > > > > #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) > > > > +int __weak efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) > > > > > > I don't think this should be weak. What other way is there of handling > > > this and why would it be platform-specific? > > > > I have a question about the current design of the capsule auth key. > > If the platform has its own key-storage, how can the platform use the > > platform specific storage? Does such platform load the key from the storage > > and generate the dtb node in the platform initialization code? (or > > device driver?) > > Are we talking about a public key (which I assume from the function > naming) or some secret key. What is an auth key? Surely, a public key (more strictly, x509 certificate under the current implementation) > As I understand it, the public key should be provided by the platform > in devicetree that U-Boot uses, by whatever prior stage has access to > the key. I still believe that some platform provider may want to save the key in a *safer* storage, which should be at least read-only for non-authorized writers. But if this issue (__weak or not) is the only blocking factor for my entire patch series, I'm willing to drop __weak for now since someone with production system may change it in the future when he has a good reason for that :) -Takahiro Akashi > Regards, > Simon
Le lun. 25 oct. 2021 à 07:14, AKASHI Takahiro <takahiro.akashi@linaro.org> a écrit : > On Wed, Oct 20, 2021 at 07:39:37AM -0600, Simon Glass wrote: > > Hi Masami, > > > > On Wed, 20 Oct 2021 at 02:18, Masami Hiramatsu > > <masami.hiramatsu@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > 2021年10月15日(金) 9:40 Simon Glass <sjg@chromium.org>: > > > > > > > > Hi Takahiro, > > > > > > > > On Thu, 7 Oct 2021 at 00:25, AKASHI Takahiro < > takahiro.akashi@linaro.org> wrote: > > > > > > > > > > The commit 47a25e81d35c ("Revert "efi_capsule: Move signature from > DTB to > > > > > .rodata"") failed to revert the removal of > efi_get_public_key_data(). > > > > > > > > > > Add back this function and move it under lib/efi_loader so that > other > > > > > platforms can utilize it. It is now declared as a weak function so > that > > > > > it can be replaced with a platform-specific implementation. > > > > > > > > > > Fixes: 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB > to > > > > > .rodata"") > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > --- > > > > > lib/efi_loader/efi_capsule.c | 36 > ++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 36 insertions(+) > > > > > > > > > > diff --git a/lib/efi_loader/efi_capsule.c > b/lib/efi_loader/efi_capsule.c > > > > > index b75e4bcba1a9..44f5da61a9be 100644 > > > > > --- a/lib/efi_loader/efi_capsule.c > > > > > +++ b/lib/efi_loader/efi_capsule.c > > > > > @@ -11,15 +11,20 @@ > > > > > #include <common.h> > > > > > #include <efi_loader.h> > > > > > #include <efi_variable.h> > > > > > +#include <env.h> > > > > > +#include <fdtdec.h> > > > > > #include <fs.h> > > > > > #include <malloc.h> > > > > > #include <mapmem.h> > > > > > #include <sort.h> > > > > > +#include <asm/global_data.h> > > > > > > > > > > #include <crypto/pkcs7.h> > > > > > #include <crypto/pkcs7_parser.h> > > > > > #include <linux/err.h> > > > > > > > > > > +DECLARE_GLOBAL_DATA_PTR; > > > > > + > > > > > const efi_guid_t efi_guid_capsule_report = > EFI_CAPSULE_REPORT_GUID; > > > > > static const efi_guid_t efi_guid_firmware_management_capsule_id = > > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > > > > @@ -251,6 +256,37 @@ out: > > > > > } > > > > > > > > > > #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) > > > > > +int __weak efi_get_public_key_data(void **pkey, efi_uintn_t > *pkey_len) > > > > > > > > I don't think this should be weak. What other way is there of > handling > > > > this and why would it be platform-specific? > > > > > > I have a question about the current design of the capsule auth key. > > > If the platform has its own key-storage, how can the platform use the > > > platform specific storage? Does such platform load the key from the > storage > > > and generate the dtb node in the platform initialization code? (or > > > device driver?) > > > > Are we talking about a public key (which I assume from the function > > naming) or some secret key. What is an auth key? > > Surely, a public key (more strictly, x509 certificate under the current > implementation) > > > As I understand it, the public key should be provided by the platform > > in devicetree that U-Boot uses, by whatever prior stage has access to > > the key. > > I still believe that some platform provider may want to save the key > in a *safer* storage, which should be at least read-only for non-authorized > writers. indeed. And U-Boot may not be entitled at all to check the capsule signature. For example updating SCP firmware with a key that is in secure world and will never leave this world. But if this issue (__weak or not) is the only blocking factor > for my entire patch series, I'm willing to drop __weak for now since > someone with production system may change it in the future when he has > a good reason for that :) If that need be…. > > -Takahiro Akashi > > > > Regards, > > Simon >
Hi Francois, 2021年10月25日(月) 15:28 François Ozog <francois.ozog@linaro.org>: > > > > Le lun. 25 oct. 2021 à 07:14, AKASHI Takahiro <takahiro.akashi@linaro.org> a écrit : >> >> On Wed, Oct 20, 2021 at 07:39:37AM -0600, Simon Glass wrote: >> > Hi Masami, >> > >> > On Wed, 20 Oct 2021 at 02:18, Masami Hiramatsu >> > <masami.hiramatsu@linaro.org> wrote: >> > > >> > > Hi Simon, >> > > >> > > 2021年10月15日(金) 9:40 Simon Glass <sjg@chromium.org>: >> > > > >> > > > Hi Takahiro, >> > > > >> > > > On Thu, 7 Oct 2021 at 00:25, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: >> > > > > >> > > > > The commit 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to >> > > > > .rodata"") failed to revert the removal of efi_get_public_key_data(). >> > > > > >> > > > > Add back this function and move it under lib/efi_loader so that other >> > > > > platforms can utilize it. It is now declared as a weak function so that >> > > > > it can be replaced with a platform-specific implementation. >> > > > > >> > > > > Fixes: 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to >> > > > > .rodata"") >> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> > > > > --- >> > > > > lib/efi_loader/efi_capsule.c | 36 ++++++++++++++++++++++++++++++++++++ >> > > > > 1 file changed, 36 insertions(+) >> > > > > >> > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c >> > > > > index b75e4bcba1a9..44f5da61a9be 100644 >> > > > > --- a/lib/efi_loader/efi_capsule.c >> > > > > +++ b/lib/efi_loader/efi_capsule.c >> > > > > @@ -11,15 +11,20 @@ >> > > > > #include <common.h> >> > > > > #include <efi_loader.h> >> > > > > #include <efi_variable.h> >> > > > > +#include <env.h> >> > > > > +#include <fdtdec.h> >> > > > > #include <fs.h> >> > > > > #include <malloc.h> >> > > > > #include <mapmem.h> >> > > > > #include <sort.h> >> > > > > +#include <asm/global_data.h> >> > > > > >> > > > > #include <crypto/pkcs7.h> >> > > > > #include <crypto/pkcs7_parser.h> >> > > > > #include <linux/err.h> >> > > > > >> > > > > +DECLARE_GLOBAL_DATA_PTR; >> > > > > + >> > > > > const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID; >> > > > > static const efi_guid_t efi_guid_firmware_management_capsule_id = >> > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; >> > > > > @@ -251,6 +256,37 @@ out: >> > > > > } >> > > > > >> > > > > #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) >> > > > > +int __weak efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) >> > > > >> > > > I don't think this should be weak. What other way is there of handling >> > > > this and why would it be platform-specific? >> > > >> > > I have a question about the current design of the capsule auth key. >> > > If the platform has its own key-storage, how can the platform use the >> > > platform specific storage? Does such platform load the key from the storage >> > > and generate the dtb node in the platform initialization code? (or >> > > device driver?) >> > >> > Are we talking about a public key (which I assume from the function >> > naming) or some secret key. What is an auth key? >> >> Surely, a public key (more strictly, x509 certificate under the current >> implementation) >> >> > As I understand it, the public key should be provided by the platform >> > in devicetree that U-Boot uses, by whatever prior stage has access to >> > the key. >> >> I still believe that some platform provider may want to save the key >> in a *safer* storage, which should be at least read-only for non-authorized >> writers. > > > indeed. And U-Boot may not be entitled at all to check the capsule signature. For example updating SCP firmware with a key that is in secure world and will never leave this world. I think secure world firmware updates should be discussed in another thread, like with FWU. At this point, the capsule signature will be only authenticated by U-Boot, because we haven't passed the capsule image to the secure world yet. >> But if this issue (__weak or not) is the only blocking factor >> for my entire patch series, I'm willing to drop __weak for now since >> someone with production system may change it in the future when he has >> a good reason for that :) > > > If that need be…. Agreed. Thank you, > >> >> >> -Takahiro Akashi >> >> >> > Regards, >> > Simon > > -- > François-Frédéric Ozog | Director Business Development > T: +33.67221.6485 > francois.ozog@linaro.org | Skype: ffozog > -- Masami Hiramatsu
Le lun. 25 oct. 2021 à 09:05, Masami Hiramatsu <masami.hiramatsu@linaro.org> a écrit : > Hi Francois, > > 2021年10月25日(月) 15:28 François Ozog <francois.ozog@linaro.org>: > > > > > > > > Le lun. 25 oct. 2021 à 07:14, AKASHI Takahiro < > takahiro.akashi@linaro.org> a écrit : > >> > >> On Wed, Oct 20, 2021 at 07:39:37AM -0600, Simon Glass wrote: > >> > Hi Masami, > >> > > >> > On Wed, 20 Oct 2021 at 02:18, Masami Hiramatsu > >> > <masami.hiramatsu@linaro.org> wrote: > >> > > > >> > > Hi Simon, > >> > > > >> > > 2021年10月15日(金) 9:40 Simon Glass <sjg@chromium.org>: > >> > > > > >> > > > Hi Takahiro, > >> > > > > >> > > > On Thu, 7 Oct 2021 at 00:25, AKASHI Takahiro < > takahiro.akashi@linaro.org> wrote: > >> > > > > > >> > > > > The commit 47a25e81d35c ("Revert "efi_capsule: Move signature > from DTB to > >> > > > > .rodata"") failed to revert the removal of > efi_get_public_key_data(). > >> > > > > > >> > > > > Add back this function and move it under lib/efi_loader so that > other > >> > > > > platforms can utilize it. It is now declared as a weak function > so that > >> > > > > it can be replaced with a platform-specific implementation. > >> > > > > > >> > > > > Fixes: 47a25e81d35c ("Revert "efi_capsule: Move signature from > DTB to > >> > > > > .rodata"") > >> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >> > > > > --- > >> > > > > lib/efi_loader/efi_capsule.c | 36 > ++++++++++++++++++++++++++++++++++++ > >> > > > > 1 file changed, 36 insertions(+) > >> > > > > > >> > > > > diff --git a/lib/efi_loader/efi_capsule.c > b/lib/efi_loader/efi_capsule.c > >> > > > > index b75e4bcba1a9..44f5da61a9be 100644 > >> > > > > --- a/lib/efi_loader/efi_capsule.c > >> > > > > +++ b/lib/efi_loader/efi_capsule.c > >> > > > > @@ -11,15 +11,20 @@ > >> > > > > #include <common.h> > >> > > > > #include <efi_loader.h> > >> > > > > #include <efi_variable.h> > >> > > > > +#include <env.h> > >> > > > > +#include <fdtdec.h> > >> > > > > #include <fs.h> > >> > > > > #include <malloc.h> > >> > > > > #include <mapmem.h> > >> > > > > #include <sort.h> > >> > > > > +#include <asm/global_data.h> > >> > > > > > >> > > > > #include <crypto/pkcs7.h> > >> > > > > #include <crypto/pkcs7_parser.h> > >> > > > > #include <linux/err.h> > >> > > > > > >> > > > > +DECLARE_GLOBAL_DATA_PTR; > >> > > > > + > >> > > > > const efi_guid_t efi_guid_capsule_report = > EFI_CAPSULE_REPORT_GUID; > >> > > > > static const efi_guid_t > efi_guid_firmware_management_capsule_id = > >> > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > >> > > > > @@ -251,6 +256,37 @@ out: > >> > > > > } > >> > > > > > >> > > > > #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) > >> > > > > +int __weak efi_get_public_key_data(void **pkey, efi_uintn_t > *pkey_len) > >> > > > > >> > > > I don't think this should be weak. What other way is there of > handling > >> > > > this and why would it be platform-specific? > >> > > > >> > > I have a question about the current design of the capsule auth key. > >> > > If the platform has its own key-storage, how can the platform use > the > >> > > platform specific storage? Does such platform load the key from the > storage > >> > > and generate the dtb node in the platform initialization code? (or > >> > > device driver?) > >> > > >> > Are we talking about a public key (which I assume from the function > >> > naming) or some secret key. What is an auth key? > >> > >> Surely, a public key (more strictly, x509 certificate under the current > >> implementation) > >> > >> > As I understand it, the public key should be provided by the platform > >> > in devicetree that U-Boot uses, by whatever prior stage has access to > >> > the key. > >> > >> I still believe that some platform provider may want to save the key > >> in a *safer* storage, which should be at least read-only for > non-authorized > >> writers. > > > > > > indeed. And U-Boot may not be entitled at all to check the capsule > signature. For example updating SCP firmware with a key that is in secure > world and will never leave this world. > > I think secure world firmware updates should be discussed in another > thread, like with FWU. At this point, the capsule signature will be > only authenticated by U-Boot, because we haven't passed the capsule > image to the secure world yet. i took a wrong example. The choice of authentication is to be done by the capsule driver designer and is outside scope of U-Boot. And the key may be in a separate storage or even the driver may invoke secure world for the authentication (FF-A API or other platform specific). U-Boot may have a capsule driver to update U-Boot components such as external env file. The location of the key can be in a u-boot specific device tree. > > >> But if this issue (__weak or not) is the only blocking factor > >> for my entire patch series, I'm willing to drop __weak for now since > >> someone with production system may change it in the future when he has > >> a good reason for that :) > > > > > > If that need be…. > > Agreed. > > Thank you, > > > > >> > >> > >> -Takahiro Akashi > >> > >> > >> > Regards, > >> > Simon > > > > -- > > François-Frédéric Ozog | Director Business Development > > T: +33.67221.6485 > > francois.ozog@linaro.org | Skype: ffozog > > > > > -- > Masami Hiramatsu >
Hi François, On Mon, 25 Oct 2021 at 01:14, François Ozog <francois.ozog@linaro.org> wrote: > > > > Le lun. 25 oct. 2021 à 09:05, Masami Hiramatsu <masami.hiramatsu@linaro.org> a écrit : >> >> Hi Francois, >> >> 2021年10月25日(月) 15:28 François Ozog <francois.ozog@linaro.org>: >> > >> > >> > >> > Le lun. 25 oct. 2021 à 07:14, AKASHI Takahiro <takahiro.akashi@linaro.org> a écrit : >> >> >> >> On Wed, Oct 20, 2021 at 07:39:37AM -0600, Simon Glass wrote: >> >> > Hi Masami, >> >> > >> >> > On Wed, 20 Oct 2021 at 02:18, Masami Hiramatsu >> >> > <masami.hiramatsu@linaro.org> wrote: >> >> > > >> >> > > Hi Simon, >> >> > > >> >> > > 2021年10月15日(金) 9:40 Simon Glass <sjg@chromium.org>: >> >> > > > >> >> > > > Hi Takahiro, >> >> > > > >> >> > > > On Thu, 7 Oct 2021 at 00:25, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: >> >> > > > > >> >> > > > > The commit 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to >> >> > > > > .rodata"") failed to revert the removal of efi_get_public_key_data(). >> >> > > > > >> >> > > > > Add back this function and move it under lib/efi_loader so that other >> >> > > > > platforms can utilize it. It is now declared as a weak function so that >> >> > > > > it can be replaced with a platform-specific implementation. >> >> > > > > >> >> > > > > Fixes: 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to >> >> > > > > .rodata"") >> >> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> >> > > > > --- >> >> > > > > lib/efi_loader/efi_capsule.c | 36 ++++++++++++++++++++++++++++++++++++ >> >> > > > > 1 file changed, 36 insertions(+) >> >> > > > > >> >> > > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c >> >> > > > > index b75e4bcba1a9..44f5da61a9be 100644 >> >> > > > > --- a/lib/efi_loader/efi_capsule.c >> >> > > > > +++ b/lib/efi_loader/efi_capsule.c >> >> > > > > @@ -11,15 +11,20 @@ >> >> > > > > #include <common.h> >> >> > > > > #include <efi_loader.h> >> >> > > > > #include <efi_variable.h> >> >> > > > > +#include <env.h> >> >> > > > > +#include <fdtdec.h> >> >> > > > > #include <fs.h> >> >> > > > > #include <malloc.h> >> >> > > > > #include <mapmem.h> >> >> > > > > #include <sort.h> >> >> > > > > +#include <asm/global_data.h> >> >> > > > > >> >> > > > > #include <crypto/pkcs7.h> >> >> > > > > #include <crypto/pkcs7_parser.h> >> >> > > > > #include <linux/err.h> >> >> > > > > >> >> > > > > +DECLARE_GLOBAL_DATA_PTR; >> >> > > > > + >> >> > > > > const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID; >> >> > > > > static const efi_guid_t efi_guid_firmware_management_capsule_id = >> >> > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; >> >> > > > > @@ -251,6 +256,37 @@ out: >> >> > > > > } >> >> > > > > >> >> > > > > #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) >> >> > > > > +int __weak efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len) >> >> > > > >> >> > > > I don't think this should be weak. What other way is there of handling >> >> > > > this and why would it be platform-specific? >> >> > > >> >> > > I have a question about the current design of the capsule auth key. >> >> > > If the platform has its own key-storage, how can the platform use the >> >> > > platform specific storage? Does such platform load the key from the storage >> >> > > and generate the dtb node in the platform initialization code? (or >> >> > > device driver?) >> >> > >> >> > Are we talking about a public key (which I assume from the function >> >> > naming) or some secret key. What is an auth key? >> >> >> >> Surely, a public key (more strictly, x509 certificate under the current >> >> implementation) >> >> >> >> > As I understand it, the public key should be provided by the platform >> >> > in devicetree that U-Boot uses, by whatever prior stage has access to >> >> > the key. >> >> >> >> I still believe that some platform provider may want to save the key >> >> in a *safer* storage, which should be at least read-only for non-authorized >> >> writers. >> > >> > >> > indeed. And U-Boot may not be entitled at all to check the capsule signature. For example updating SCP firmware with a key that is in secure world and will never leave this world. >> >> I think secure world firmware updates should be discussed in another >> thread, like with FWU. At this point, the capsule signature will be >> only authenticated by U-Boot, because we haven't passed the capsule >> image to the secure world yet. > > i took a wrong example. The choice of authentication is to be done by the capsule driver designer and is outside scope of U-Boot. And the key may be in a separate storage or even the driver may invoke secure world for the authentication (FF-A API or other platform specific). U-Boot may have a capsule driver to update U-Boot components such as external env file. The location of the key can be in a u-boot specific device tree. There is no such thing, in practice. There is just one devicetree. We need to resolve this as we are still not aligned on this, after many months. I think in fact there is difference of opinion about the nature of the firmware image itself. I see it is a combined whole, where the various blobs are an unfortunate result of firmware fragmentation and must be stitched together with binman, so they can each see the full picture to the extent needed. > >> >> >> >> But if this issue (__weak or not) is the only blocking factor >> >> for my entire patch series, I'm willing to drop __weak for now since >> >> someone with production system may change it in the future when he has >> >> a good reason for that :) >> > >> > >> > If that need be…. >> >> Agreed. >> Yes please - Simon >> >> Thank you, >> >> > >> >> >> >> >> >> -Takahiro Akashi >> >> >> >> >> >> > Regards, >> >> > Simon >> > >> > -- >> > François-Frédéric Ozog | Director Business Development >> > T: +33.67221.6485 >> > francois.ozog@linaro.org | Skype: ffozog >> > >> >> >> -- >> Masami Hiramatsu > > -- > François-Frédéric Ozog | Director Business Development > T: +33.67221.6485 > francois.ozog@linaro.org | Skype: ffozog >
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index b75e4bcba1a9..44f5da61a9be 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -11,15 +11,20 @@ #include <common.h> #include <efi_loader.h> #include <efi_variable.h> +#include <env.h> +#include <fdtdec.h> #include <fs.h> #include <malloc.h> #include <mapmem.h> #include <sort.h> +#include <asm/global_data.h> #include <crypto/pkcs7.h> #include <crypto/pkcs7_parser.h> #include <linux/err.h> +DECLARE_GLOBAL_DATA_PTR; + const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID; static const efi_guid_t efi_guid_firmware_management_capsule_id = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; @@ -251,6 +256,37 @@ out: } #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE) +int __weak 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) { + log_err("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) { + log_err("Unable to get capsule-key value\n"); + *pkey = NULL; + *pkey_len = 0; + + return -FDT_ERR_NOTFOUND; + } + + *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)
The commit 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to .rodata"") failed to revert the removal of efi_get_public_key_data(). Add back this function and move it under lib/efi_loader so that other platforms can utilize it. It is now declared as a weak function so that it can be replaced with a platform-specific implementation. Fixes: 47a25e81d35c ("Revert "efi_capsule: Move signature from DTB to .rodata"") Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- lib/efi_loader/efi_capsule.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)