On Tue, Oct 22, 2019 at 07:50:20AM -0600, Simon Glass wrote:
> Hi Takahiro,
>
> On Tue, 17 Sep 2019 at 20:59, AKASHI Takahiro
> <takahiro.aka...@linaro.org> wrote:
> >
> > Simon,
> >
> > Overall, do you agree to my approach here?
> >
> > On Mon, Sep 16, 2019 at 10:48:07PM -0700, Simon Glass wrote:
> > > Hi AKASHI,
> > >
> > > On Fri, 6 Sep 2019 at 00:06, AKASHI Takahiro <takahiro.aka...@linaro.org>
> > > wrote:
> > > >
> > > > This function, and hence rsa_verify(), will perform RSA verification
> > > > with two essential parameters for a RSA public key in contract of
> > > > rsa_verify_with_keynode(), which requires additional three parameters
> > > > stored in FIT image.
> > > >
> > > > It will be used in implementing UEFI secure boot, i.e. image
> > > > authentication
> > > > and variable authentication.
> > > >
> > > > Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
> > > > ---
> > > > lib/rsa/Kconfig | 7 +++++
> > > > lib/rsa/Makefile | 3 ++-
> > > > lib/rsa/rsa-verify.c | 63 ++++++++++++++++++++++++++++++++++++++------
> > > > 3 files changed, 64 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> > > > index 338c8124da59..3c1986a26f8c 100644
> > > > --- a/lib/rsa/Kconfig
> > > > +++ b/lib/rsa/Kconfig
> > > > @@ -25,6 +25,13 @@ config RSA_VERIFY
> > > > help
> > > > Add RSA signature verification support.
> > > >
> > > > +config RSA_VERIFY_WITH_PKEY
> > > > + bool "Execute RSA verification without key parameters from FDT"
> > > > + depends on RSA
> > > > + help
> > > > + This options enables RSA signature verification without
> > > > + using public key parameters which is embedded control FDT.
> > >
> > > Please expand this, a lot. It is too brief.
> >
> > Will add more description.
> >
> > > > +
> > > > config RSA_SOFTWARE_EXP
> > > > bool "Enable driver for RSA Modular Exponentiation in software"
> > > > depends on DM
> > > > diff --git a/lib/rsa/Makefile b/lib/rsa/Makefile
> > > > index d66eef74c514..fd4592fd6a8a 100644
> > > > --- a/lib/rsa/Makefile
> > > > +++ b/lib/rsa/Makefile
> > > > @@ -5,5 +5,6 @@
> > > > # (C) Copyright 2000-2007
> > > > # Wolfgang Denk, DENX Software Engineering, w...@denx.de.
> > > >
> > > > -obj-$(CONFIG_RSA_VERIFY) += rsa-verify.o rsa-checksum.o rsa-keyprop.o
> > > > +obj-$(CONFIG_RSA_VERIFY) += rsa-verify.o rsa-checksum.o
> > > > +obj-$(CONFIG_RSA_VERIFY_WITH_PKEY) += rsa-keyprop.o
> > > > obj-$(CONFIG_RSA_SOFTWARE_EXP) += rsa-mod-exp.o
> > > > diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
> > > > index 287fcc4d234d..80eabff3e940 100644
> > > > --- a/lib/rsa/rsa-verify.c
> > > > +++ b/lib/rsa/rsa-verify.c
> > > > @@ -17,9 +17,14 @@
> > > > #include "mkimage.h"
> > > > #include <fdt_support.h>
> > > > #endif
> > > > +#include <linux/kconfig.h>
> > > > #include <u-boot/rsa-mod-exp.h>
> > > > #include <u-boot/rsa.h>
> > > >
> > > > +#ifndef __UBOOT__ /* for host tools */
> > > > +#undef CONFIG_RSA_VERIFY_WITH_PKEY
> > > > +#endif
> > > > +
> > > > /* Default public exponent for backward compatibility */
> > > > #define RSA_DEFAULT_PUBEXP 65537
> > > >
> > > > @@ -342,6 +347,34 @@ static int rsa_verify_key(struct image_sign_info
> > > > *info,
> > > > return 0;
> > > > }
> > > >
> > > > +#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
> > > > +/**
> > > > + * rsa_verify_with_pkey()
> > > > + *
> > > > + */
> > > > +static int rsa_verify_with_pkey(struct image_sign_info *info,
> > > > + const void *hash, uint8_t *sig, uint
> > > > sig_len)
> > > > +{
> > > > + struct key_prop *prop;
> > > > + int ret;
> > > > +
> > > > + /* Public key is self-described to fill key_prop */
> > > > + prop = rsa_gen_key_prop(info->key, info->keylen);
> > > > + if (!prop) {
> > > > + debug("Generating necessary parameter for decoding
> > > > failed\n");
> > > > + return -EACCES;
> > > > + }
> > > > +
> > > > + ret = rsa_verify_key(info, prop, sig, sig_len, hash,
> > > > + info->crypto->key_len);
> > > > +
> > > > + rsa_free_key_prop(prop);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +#endif
> > > > +
> > > > +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> > > > /**
> > > > * rsa_verify_with_keynode() - Verify a signature against some data
> > > > using
> > > > * information in node with prperties of RSA Key like modulus,
> > > > exponent etc.
> > > > @@ -395,18 +428,21 @@ static int rsa_verify_with_keynode(struct
> > > > image_sign_info *info,
> > > >
> > > > return ret;
> > > > }
> > > > +#endif
> > > >
> > > > int rsa_verify(struct image_sign_info *info,
> > > > const struct image_region region[], int region_count,
> > > > uint8_t *sig, uint sig_len)
> > > > {
> > > > - const void *blob = info->fdt_blob;
> > > > /* Reserve memory for maximum checksum-length */
> > > > uint8_t hash[info->crypto->key_len];
> > > > + int ret = -EACCES;
> > > > +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
>
> Can you use if() instead of #if ?
This code is for local variable declaration, and so changing as you suggested
can be difficult. Or do you accept moving this stuff further down to the next
"#if CONFIG_IS_ENABLED(FIT_SIGNATURE)":
=== 8< ===
if (IS_ENABLED(CONFIG_RSA_VERIFY_WITH_PKEY) && !info->fdt_blob) {
/* don't rely on fdt properties */
ret = rsa_verify_with_pkey(info, hash, sig, sig_len);
return ret;
}
if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) {
/*
* declare variables here!
*/
const void *blob = info->fdt_blob;
int ndepth, noffset;
int sig_node, node;
char name[100];
sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
if (sig_node < 0) {
debug("%s: No signature node found\n", __func__);
return -ENOENT;
}
/* See if we must use a particular key */
...
}
=== >8 ===
Otherwise, we will have to introduce another function, rsa_verify_with_fdt?,
just for this purpose.
> > > > + const void *blob = info->fdt_blob;
> > > > int ndepth, noffset;
> > > > int sig_node, node;
> > > > char name[100];
> > > > - int ret;
> > > > +#endif
> > > >
> > > > /*
> > > > * Verify that the checksum-length does not exceed the
> > > > @@ -419,12 +455,6 @@ int rsa_verify(struct image_sign_info *info,
> > > > return -EINVAL;
> > > > }
> > > >
> > > > - sig_node = fdt_subnode_offset(blob, 0, FIT_SIG_NODENAME);
> > > > - if (sig_node < 0) {
> > > > - debug("%s: No signature node found\n", __func__);
> > > > - return -ENOENT;
> > > > - }
> > > > -
> > > > /* Calculate checksum with checksum-algorithm */
> > > > ret = info->checksum->calculate(info->checksum->name,
> > > > region, region_count, hash);
> > > > @@ -433,6 +463,22 @@ int rsa_verify(struct image_sign_info *info,
> > > > return -EINVAL;
> > > > }
> > > >
> > > > +#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
> > >
> > > Can this use if() instead of #ifdef?
> >
> > Do you mean
> > if (IS_ENABLED(CONFIG_RSA_VERIFY_WITH_PKEY) && !info->fdt_blob) ?
>
> Yes
>
> >
> > > > + if (!info->fdt_blob) {
> > > > + /* don't rely on fdt properties */
> > > > + ret = rsa_verify_with_pkey(info, hash, sig, sig_len);
> > >
> > > Does this support required_keynode?
> >
> > IICU, no because required_keynode is an offset into fdt.
> >
> > > Please add to the documentation for secure boot in uImage, as this
> > > seems to be a new case.
> >
> > It may work with FIT in case of no extra key parameters,
> > I intended that the feature is used only by UEFI.
> > So I don't think it appropriate to add a doc under doc/uImage.FIT.
>
> Sounds good.
>
> >
> > > Also, how do we test this new code?
> >
> > The code is exercised and tested only through UEFI's verification code
> > and relevant pytest (in my secure boot patch[1]). Like test_vboot?
>
> OK good.
Thanks,
-Takahiro Akashi
> >
> > [1] https://lists.denx.de/pipermail/u-boot/2019-September/383897.html
> >
> > Thanks,
> > -Takahiro Akashi
>
> Regards,
> Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
