diff mbox series

[U-Boot,RFC,3/3] lib: rsa: add rsa_verify_with_pkey()

Message ID 20190906070808.1198-4-takahiro.akashi@linaro.org
State RFC
Delegated to: Tom Rini
Headers show
Series rsa: extend rsa_verify() for UEFI secure boot | expand

Commit Message

AKASHI Takahiro Sept. 6, 2019, 7:08 a.m. UTC
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.akashi@linaro.org>
---
 lib/rsa/Kconfig      |  7 +++++
 lib/rsa/Makefile     |  3 ++-
 lib/rsa/rsa-verify.c | 63 ++++++++++++++++++++++++++++++++++++++------
 3 files changed, 64 insertions(+), 9 deletions(-)

Comments

Simon Glass Sept. 17, 2019, 5:48 a.m. UTC | #1
Hi AKASHI,

On Fri, 6 Sep 2019 at 00:06, AKASHI Takahiro <takahiro.akashi@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.akashi@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.

> +
>  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, wd@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)
> +       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?

> +       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?

Please add to the documentation for secure boot in uImage, as this
seems to be a new case.

Also, how do we test this new code?


> +
> +               return ret;
> +       }
> +#endif
> +
> +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> +       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 */
>         if (info->required_keynode != -1) {
>                 ret = rsa_verify_with_keynode(info, hash, sig, sig_len,
> @@ -459,6 +505,7 @@ int rsa_verify(struct image_sign_info *info,
>                                 break;
>                 }
>         }
> +#endif
>
>         return ret;
>  }
> --
> 2.21.0
>
AKASHI Takahiro Sept. 18, 2019, 3:03 a.m. UTC | #2
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.akashi@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.akashi@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, wd@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)
> > +       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) ?

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

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

[1] https://lists.denx.de/pipermail/u-boot/2019-September/383897.html

Thanks,
-Takahiro Akashi


> 
> > +
> > +               return ret;
> > +       }
> > +#endif
> > +
> > +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> > +       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 */
> >         if (info->required_keynode != -1) {
> >                 ret = rsa_verify_with_keynode(info, hash, sig, sig_len,
> > @@ -459,6 +505,7 @@ int rsa_verify(struct image_sign_info *info,
> >                                 break;
> >                 }
> >         }
> > +#endif
> >
> >         return ret;
> >  }
> > --
> > 2.21.0
> >
AKASHI Takahiro Oct. 3, 2019, 5:48 a.m. UTC | #3
Simon,

On Wed, Sep 18, 2019 at 12:03:25PM +0900, AKASHI Takahiro wrote:
> Simon,
> 
> Overall, do you agree to my approach here?

Ping. Do you mind my sending out v2?

-Takahiro Akashi


> 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.akashi@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.akashi@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, wd@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)
> > > +       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) ?
> 
> > > +       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.
> 
> > 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?
> 
> [1] https://lists.denx.de/pipermail/u-boot/2019-September/383897.html
> 
> Thanks,
> -Takahiro Akashi
> 
> 
> > 
> > > +
> > > +               return ret;
> > > +       }
> > > +#endif
> > > +
> > > +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> > > +       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 */
> > >         if (info->required_keynode != -1) {
> > >                 ret = rsa_verify_with_keynode(info, hash, sig, sig_len,
> > > @@ -459,6 +505,7 @@ int rsa_verify(struct image_sign_info *info,
> > >                                 break;
> > >                 }
> > >         }
> > > +#endif
> > >
> > >         return ret;
> > >  }
> > > --
> > > 2.21.0
> > >
Simon Glass Oct. 22, 2019, 1:50 p.m. UTC | #4
Hi Takahiro,

On Tue, 17 Sep 2019 at 20:59, AKASHI Takahiro
<takahiro.akashi@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.akashi@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.akashi@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, wd@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 ?

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

>
> [1] https://lists.denx.de/pipermail/u-boot/2019-September/383897.html
>
> Thanks,
> -Takahiro Akashi

Regards,
Simon
AKASHI Takahiro Oct. 23, 2019, 5:44 a.m. UTC | #5
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.akashi@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.akashi@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.akashi@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, wd@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
Simon Glass Oct. 27, 2019, 4:31 p.m. UTC | #6
Hi Takahiro,

On Tue, 22 Oct 2019 at 23:43, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> 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.akashi@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.akashi@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.akashi@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, wd@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)":

Yes if you use if() then you don't need to #ifdef the variables.

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

Regards,
Simon
AKASHI Takahiro Oct. 28, 2019, 12:43 a.m. UTC | #7
Simon,

On Sun, Oct 27, 2019 at 10:31:59AM -0600, Simon Glass wrote:
> Hi Takahiro,
> 
> On Tue, 22 Oct 2019 at 23:43, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > 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.akashi@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.akashi@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.akashi@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, wd@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)":
> 
> Yes if you use if() then you don't need to #ifdef the variables.

Okay, I will change the code as below.
-Takahiro Akashi

> > === 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.
> >
> 
> Regards,
> Simon
diff mbox series

Patch

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.
+
 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, wd@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)
+	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
+	if (!info->fdt_blob) {
+		/* don't rely on fdt properties */
+		ret = rsa_verify_with_pkey(info, hash, sig, sig_len);
+
+		return ret;
+	}
+#endif
+
+#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
+	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 */
 	if (info->required_keynode != -1) {
 		ret = rsa_verify_with_keynode(info, hash, sig, sig_len,
@@ -459,6 +505,7 @@  int rsa_verify(struct image_sign_info *info,
 				break;
 		}
 	}
+#endif
 
 	return ret;
 }