[U-Boot] [PATCH v3 4/6] lib: rsa: generate additional parameters for public key
AKASHI Takahiro
takahiro.akashi at linaro.org
Wed Nov 20 05:53:28 UTC 2019
Simon,
On Tue, Nov 19, 2019 at 06:59:59PM -0800, Simon Glass wrote:
> Hi Takahiro,
>
> On Tue, 12 Nov 2019 at 16:47, AKASHI Takahiro
> <takahiro.akashi at 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 at linaro.org>
> > ---
> > include/u-boot/rsa-mod-exp.h | 21 +
> > lib/rsa/Kconfig | 1 +
> > lib/rsa/Makefile | 1 +
> > lib/rsa/rsa-keyprop.c | 717 +++++++++++++++++++++++++++++++++++
> > 4 files changed, 740 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..374169b8304e 100644
> > --- a/include/u-boot/rsa-mod-exp.h
> > +++ b/include/u-boot/rsa-mod-exp.h
> > @@ -26,6 +26,27 @@ struct key_prop {
> > uint32_t exp_len; /* Exponent length in number of uint8_t */
> > };
> >
> > +/**
> > + * rsa_gen_key_prop() - Generate key properties of RSA public key
> > + * @key: Specifies key data in DER format
> > + * @keylen: Length of @key
> > + *
> > + * This function takes a blob of encoded RSA public key data in DER
> > + * format, parse it and generate all the relevant properties
> > + * in key_prop structure.
> > + *
> > + * Return: Pointer to struct key_prop on success, NULL on error
> > + */
> > +struct key_prop *rsa_gen_key_prop(const void *key, uint32_t keylen);
> > +
> > +/**
> > + * rsa_free_key_prop() - Free key properties
> > + * @prop: Pointer to struct key_prop
> > + *
> > + * This function frees all the memories allocated by rsa_gen_key_prop().
> > + */
> > +void rsa_free_key_prop(struct key_prop *prop);
> > +
> > /**
> > * rsa_mod_exp_sw() - Perform RSA Modular Exponentiation in sw
> > *
> > diff --git a/lib/rsa/Kconfig b/lib/rsa/Kconfig
> > index 71e4c06bf883..d1d6f6cf64a3 100644
> > --- a/lib/rsa/Kconfig
> > +++ b/lib/rsa/Kconfig
> > @@ -33,6 +33,7 @@ config RSA_VERIFY
> > config RSA_VERIFY_WITH_PKEY
> > bool "Execute RSA verification without key parameters from FDT"
> > depends on RSA
> > + imply RSA_PUBLIC_KEY_PARSER
> > help
> > The standard RSA-signature verification code (FIT_SIGNATURE) uses
> > pre-calculated key properties, that are stored in fdt blob, in
> > 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..9458337cc608
> > --- /dev/null
> > +++ b/lib/rsa/rsa-keyprop.c
> > @@ -0,0 +1,717 @@
> > +// 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:
>
> Can we put these in their own file if we expect to update them? Really
> should be called by the original filename and go in lib/bearsll.
Honestly, I'd rather disagree with you because
* in the original code, each file has only a single function, then
we will have to introduce bunch of small source files.
* it is quite unlikely for other components to re-use some of
those functions in the future.
So splitting the file would practically make little use.
> [..]
>
> > +/**
> > + * rsa_gen_key_prop() - Generate key properties of RSA public key
> > + * @key: Specifies key data in DER format
> > + * @keylen: Length of @key
> > + *
> > + * This function takes a blob of encoded RSA public key data in DER
> > + * format, parse it and generate all the relevant properties
> > + * in key_prop structure.
> > + *
> > + * Return: Pointer to struct key_prop on success, NULL on error
> > + */
> > +struct key_prop *rsa_gen_key_prop(const void *key, uint32_t keylen)
> > +{
> > + struct key_prop *prop;
> > + struct rsa_key rsa_key;
> > +#define BR_MAX_RSA_SIZE 4096
>
> const int?
Will change it to
const int max_rsa_size = 4096;
> > + uint32_t *n, *rr, *rrtmp;
> > + int rlen, i, ret;
> > +
> > + prop = calloc(sizeof(*prop), 1);
> > + if (!prop)
> > + return NULL;
> > + n = calloc(sizeof(uint32_t), 1 + (BR_MAX_RSA_SIZE >> 5));
> > + rr = calloc(sizeof(uint32_t), 1 + (BR_MAX_RSA_SIZE >> 5));
> > + rrtmp = calloc(sizeof(uint32_t), 1 + (BR_MAX_RSA_SIZE >> 5));
> > + if (!n || !rr || !rrtmp)
> > + return NULL;
>
> This can cause memory leak and will likely generate a coverity warning.
Okay, will fix it.
> > +
> > + ret = rsa_parse_pub_key(&rsa_key, key, keylen);
> > + if (ret)
> > + goto err;
> > +
> > + /* modulus */
> > + /* removing leading 0's */
> > + for (i = 0; i < rsa_key.n_sz && !rsa_key.n[i]; i++)
> > + ;
> > + prop->num_bits = (rsa_key.n_sz - i) * 8;
> > + prop->modulus = malloc(rsa_key.n_sz - i);
> > + if (!prop->modulus)
> > + goto err;
> > + memcpy((void *)prop->modulus, &rsa_key.n[i], rsa_key.n_sz - i);
> > +
> > + /* exponent */
> > + prop->public_exponent = calloc(1, sizeof(uint64_t));
> > + if (!prop->public_exponent)
> > + goto err;
> > + memcpy((void *)prop->public_exponent + sizeof(uint64_t) - rsa_key.e_sz,
> > + rsa_key.e, rsa_key.e_sz);
> > + prop->exp_len = rsa_key.e_sz;
> > +
> > + /* n0 inverse */
> > + br_i32_decode(n, &rsa_key.n[i], rsa_key.n_sz - i);
> > + prop->n0inv = br_i32_ninv32(n[1]);
> > +
> > + /* R^2 mod n; R = 2^(num_bits) */
> > + rlen = prop->num_bits * 2; /* #bits of R^2 = (2^num_bits)^2 */
> > + rr[0] = 0;
> > + *(uint8_t *)&rr[0] = (1 << (rlen % 8));
> > + for (i = 1; i < (((rlen + 31) >> 5) + 1); i++)
> > + rr[i] = 0;
> > + br_i32_decode(rrtmp, rr, ((rlen + 7) >> 3) + 1);
> > + br_i32_reduce(rr, rrtmp, n);
> > +
> > + rlen = (prop->num_bits + 7) >> 3; /* #bytes of R^2 mod n */
> > + prop->rr = malloc(rlen);
> > + if (!prop->rr)
> > + goto err;
> > + br_i32_encode((void *)prop->rr, rlen, rr);
> > +
> > + return prop;
> > +
> > +err:
> > + free(n);
> > + free(rr);
> > + free(rrtmp);
> > + rsa_free_key_prop(prop);
> > + return NULL;
>
> This should return -ENOMEM if we are out of memory. The caller cannot
> divine what went wrong.
Agree.
Thank you for your review,
-Takahiro Akashi
> > +}
> > --
> > 2.21.0
> >
>
> Regards,
> Simon
More information about the U-Boot
mailing list