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

[Simon Glass]

Hi Takahiro,

On Tue, 8 Oct 2019 at 23:27, 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 |   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.

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

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

Also please keep the function name on the same line as the 'static'.

> +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 */

??

> +
> +static inline uint32_t
> +NOT(uint32_t ctl)
> +{
> +       return ctl ^ 1;
> +}
> +

All of these functions need full comments please.

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

> +{
> +       uint32_t q;
> +
> +       q = x ^ y;
> +       return NOT((q | -q) >> 31);
> +}
> +

[...]

Regards,
Simon