[U-Boot] [PATCH v1 2/3] lib: rsa: generate additional parameters for public key

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Simon, Tom,

On Fri, Oct 25, 2019 at 12:29:08PM -0600, Simon Glass wrote:
> Hi,
> 
> On Fri, 25 Oct 2019 at 12:27, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Oct 24, 2019 at 03:36:07PM -0600, Simon Glass wrote:
> > > Hi,
> > >
> > > On Tue, 22 Oct 2019 at 23:23, AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Mon, Oct 21, 2019 at 06:17:03PM -0600, Simon Glass wrote:
> > > > > Hi Takahiro,
> > > > >
> > > > > On Tue, 8 Oct 2019 at 23:27, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > > > >
> > > > > > In the current implementation of FIT_SIGNATURE, five parameters for
> > > > > > a RSA public key are required while only two of them are essential.
> > > > > > (See rsa-mod-exp.h and uImage.FIT/signature.txt)
> > > > > > This is a result of considering relatively limited computer power
> > > > > > and resources on embedded systems, while such a assumption may not
> > > > > > be quite practical for other use cases.
> > > > > >
> > > > > > In this patch, added is a function, rsa_gen_key_prop(), which will
> > > > > > generate additional parameters for other uses, in particular
> > > > > > UEFI secure boot, on the fly.
> > > > > >
> > > > > > Note: the current code uses some "big number" routines from BearSSL
> > > > > > for the calculation.
> > > > > >
> > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > > ---
> > > > > >  include/u-boot/rsa-mod-exp.h |   3 +
> > > > > >  lib/rsa/Kconfig              |   7 +
> > > > > >  lib/rsa/Makefile             |   1 +
> > > > > >  lib/rsa/rsa-keyprop.c        | 585 +++++++++++++++++++++++++++++++++++
> > > > > >  4 files changed, 596 insertions(+)
> > > > > >  create mode 100644 lib/rsa/rsa-keyprop.c
> > > > > >
> > > > > > diff --git a/include/u-boot/rsa-mod-exp.h b/include/u-boot/rsa-mod-exp.h
> > > > > > index 8a428c4b6a1a..ca189292d869 100644
> > > > > > --- a/include/u-boot/rsa-mod-exp.h
> > > > > > +++ b/include/u-boot/rsa-mod-exp.h
> > > > > > @@ -26,6 +26,9 @@ struct key_prop {
> > > > > >         uint32_t exp_len;       /* Exponent length in number of uint8_t */
> > > > > >  };
> > > > > >
> > > > > > +struct key_prop *rsa_gen_key_prop(const void *key, uint32_t keylen);
> > > > > > +void rsa_free_key_prop(struct key_prop *prop);
> > > > >
> > > > > Please add full function comments.
> > > >
> > > > Sure.
> > > >
> > > > > > +
> > > > > >  /**
> > > > > >   * rsa_mod_exp_sw() - Perform RSA Modular Exponentiation in sw
> > > > > >   *
> > > > > > diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> > > > > > index 62b7ab9c5e5c..d1743d7a4c47 100644
> > > > > > --- a/lib/rsa/Kconfig
> > > > > > +++ b/lib/rsa/Kconfig
> > > > > > @@ -30,6 +30,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.
> > > > >
> > > > > How about something like...
> > > > >
> > > > > The standard RSA-signature verification code uses ....
> > > > >
> > > > > This does not suit the use case where ...
> > > > >
> > > > > This option enables ...
> > > >
> > > > I will try to improve ...
> > > >
> > > > > > +
> > > > > >  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 c07305188e0c..14ed3cb4012b 100644
> > > > > > --- a/lib/rsa/Makefile
> > > > > > +++ b/lib/rsa/Makefile
> > > > > > @@ -6,4 +6,5 @@
> > > > > >  # Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> > > > > >
> > > > > >  obj-$(CONFIG_$(SPL_)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-keyprop.c b/lib/rsa/rsa-keyprop.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..d7d222e9bed9
> > > > > > --- /dev/null
> > > > > > +++ b/lib/rsa/rsa-keyprop.c
> > > > > > @@ -0,0 +1,585 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+ and MIT
> > > > > > +/*
> > > > > > + * RSA library - generate parameters for a public key
> > > > > > + *
> > > > > > + * Copyright (c) 2019 Linaro Limited
> > > > > > + * Author: AKASHI Takahiro
> > > > > > + *
> > > > > > + * Big number routines in this file come from BearSSL:
> > > > > > + * Copyright (c) 2016 Thomas Pornin <pornin@bolet.org>
> > > > > > + */
> > > > > > +
> > > > > > +#include <common.h>
> > > > > > +#include <image.h>
> > > > > > +#include <malloc.h>
> > > > > > +#include <asm/byteorder.h>
> > > > > > +#include <crypto/internal/rsa.h>
> > > > > > +#include <u-boot/rsa-mod-exp.h>
> > > > > > +
> > > > > > +static inline unsigned
> > > > >
> > > > > Please drop the inlines unless needed. Would prefer to leave this to
> > > > > the compiler.
> > > >
> > > > Okay.
> > > >
> > > > > Also please keep the function name on the same line as the 'static'.
> > > >
> > > > Okay.
> > > >
> > > > > > +br_dec16be(const void *src)
> > > > > > +{
> > > > > > +       return be16_to_cpup(src);
> > > > > > +}
> > > > > > +
> > > > > > +static inline uint32_t
> > > > > > +br_dec32be(const void *src)
> > > > > > +{
> > > > > > +       return be32_to_cpup(src);
> > > > > > +}
> > > > > > +
> > > > > > +static inline void
> > > > > > +br_enc32be(void *dst, uint32_t x)
> > > > > > +{
> > > > > > +       __be32 tmp;
> > > > > > +
> > > > > > +       tmp = cpu_to_be32(x);
> > > > > > +       memcpy(dst, &tmp, sizeof(tmp));
> > > > > > +}
> > > > > > +
> > > > > > +/* stripped version of src/inner.h */
> > > > >
> > > > > ??
> > > >
> > > > I mean that only a small subset of BearSSL's src/inner.h was imported here.
> > > >
> > > > > > +
> > > > > > +static inline uint32_t
> > > > > > +NOT(uint32_t ctl)
> > > > > > +{
> > > > > > +       return ctl ^ 1;
> > > > > > +}
> > > > > > +
> > > > >
> > > > > All of these functions need full comments please.
> > > >
> > > > All the functions imported from BearSSL?
> > > > Please note that all the functions, except rsa_gen_key_prop()
> > > > rsa_free_key_prop(), are just imported without changes and
> > > > all are declared as static.
> > > >
> > > > > > +static inline uint32_t
> > > > > > +MUX(uint32_t ctl, uint32_t x, uint32_t y)
> > > > > > +{
> > > > > > +       return y ^ (-ctl & (x ^ y));
> > > > > > +}
> > > > > > +
> > > > > > +static inline uint32_t
> > > > > > +EQ(uint32_t x, uint32_t y)
> > > > >
> > > > > Should use lower case. Please run patman.
> > > >
> > > > I would like to honer the original code(BearSSL)'s notation including
> > > > upper/lower cases for minimizing differences.
> > >
> > > There is precedent for this. It's just that I'm not sure the code is
> > > complicated enough to worry about keeping the comment-free
> > > single-char-variable style?
> > >
> > > +Tom Rini I assume you are OK with this?
> >
> > When we import code from another project that we expect to re-sync we do
> > what we need to do to keep the overall re-sync pain down, even we we
> > otherwise may disagree on style points.
> 
> OK ta.

Thank you for your understanding.
Yes, this patch is small but if I had to modify my "import x509/pkcs7
parsers from linux" patch, it would be quite painful.

-Takahiro Akashi


> Regards,
> Simon