Hi Ruchika,
On 28 December 2014 at 20:53, Ruchika Gupta <ruchika.gu...@freescale.com> wrote:
> Re-sending as delivery to uboot mailing list failed last time...
>
> Hi Simon,
>
> Thanks for the review comments.
>
>> -----Original Message-----
>> From: s...@google.com [mailto:s...@google.com] On Behalf Of Simon Glass
>> Sent: Wednesday, December 24, 2014 6:19 AM
>> To: Gupta Ruchika-R66431
>> Cc: U-Boot Mailing List; Sun York-R58495
>> Subject: Re: [PATCH 3/9] [v3] DM: crypto/rsa: Add rsa Modular
>> Exponentiation DM driver
>>
>> Hi Ruchika,
>>
>> On 23 December 2014 at 04:32, Ruchika Gupta
>> <ruchika.gu...@freescale.com>
>> wrote:
>> > Add a new rsa uclass for performing modular exponentiation and
>> > implement the software driver basing on this uclass.
>> >
>> > Signed-off-by: Ruchika Gupta <ruchika.gu...@freescale.com>
>> > CC: Simon Glass <s...@chromium.org>
>> > ---
>> > Changes in v3:
>> > New patch with driver model for RSA UCLASS
>> >
>> > drivers/crypto/Kconfig | 1 +
>> > drivers/crypto/Makefile | 1 +
>> > drivers/crypto/rsa/Kconfig | 5 +++++
>> > drivers/crypto/rsa/Makefile | 8 ++++++++
>> > drivers/crypto/rsa/rsa_sw.c | 39
>> +++++++++++++++++++++++++++++++++++++++
>> > drivers/crypto/rsa/rsa_uclass.c | 31 +++++++++++++++++++++++++++++++
>> > include/dm/uclass-id.h | 1 +
>> > include/u-boot/rsa-mod-exp.h | 40
>> ++++++++++++++++++++++++++++++++++++++++
>> > 8 files changed, 126 insertions(+)
>> > create mode 100644 drivers/crypto/rsa/Kconfig create mode 100644
>> > drivers/crypto/rsa/Makefile create mode 100644
>> > drivers/crypto/rsa/rsa_sw.c create mode 100644
>> > drivers/crypto/rsa/rsa_uclass.c
>> >
>> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig index
>> > e69de29..75f3479 100644
>> > --- a/drivers/crypto/Kconfig
>> > +++ b/drivers/crypto/Kconfig
>> > @@ -0,0 +1 @@
>> > +source drivers/crypto/rsa/Kconfig
>> > diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile index
>> > 7b79237..a2f30fc 100644
>> > --- a/drivers/crypto/Makefile
>> > +++ b/drivers/crypto/Makefile
>> > @@ -6,4 +6,5 @@
>> > #
>> >
>> > obj-$(CONFIG_EXYNOS_ACE_SHA) += ace_sha.o
>> > +obj-y += rsa/
>> > obj-y += fsl/
>> > diff --git a/drivers/crypto/rsa/Kconfig b/drivers/crypto/rsa/Kconfig
>> > new file mode 100644 index 0000000..7eb90a1
>> > --- /dev/null
>> > +++ b/drivers/crypto/rsa/Kconfig
>> > @@ -0,0 +1,5 @@
>> > +config DM_RSA
>> > + bool "Enable Driver Model for RSA "
>> > + depends on DM
>> > + help
>> > + If you want to use driver model for RSA Modular
>> > +Exponentiation,
>> say Y.
>>
>> Can you send a new patch (later if you prefer) which removes this
>> option altogether? It should be the default. In other words, RSA
>> should always use driver model.
>
> If we make it mandatory for RSA to use driver model, all platforms would need
> to use DM by default in their defconfig. Would that be acceptable ?
> Secondly if I force select DM and DM_RSA when CONFIG_RSA is selected, this
> may result in duplication of CONFIG_DM with platforms having it defined
> already.
> Please suggest.
Yes that is correct, all platforms with RSA can use DM. As far as I
know they do, but if not we can add a patch to fix that.
>
>>
>> > diff --git a/drivers/crypto/rsa/Makefile
>> > b/drivers/crypto/rsa/Makefile new file mode 100644 index
>> > 0000000..fae4f8c
>> > --- /dev/null
>> > +++ b/drivers/crypto/rsa/Makefile
>> > @@ -0,0 +1,8 @@
>> > +#
>> > +# (C) Copyright 2014 Freescale Semiconductor, Inc.
>> > +#
>> > +# SPDX-License-Identifier: GPL-2.0+
>> > +#
>> > +
>> > +obj-$(CONFIG_DM_RSA) += rsa_uclass.o
>> > +obj-$(CONFIG_RSA_SW) += rsa_sw.o
>> > diff --git a/drivers/crypto/rsa/rsa_sw.c
>> > b/drivers/crypto/rsa/rsa_sw.c new file mode 100644 index
>> > 0000000..5d94754
>> > --- /dev/null
>> > +++ b/drivers/crypto/rsa/rsa_sw.c
>> > @@ -0,0 +1,39 @@
>> > +/*
>> > + * (C) Copyright 2014 Freescale Semiconductor, Inc.
>> > + * Author: Ruchika Gupta <ruchika.gu...@freescale.com>
>> > + *
>> > + * SPDX-License-Identifier: GPL-2.0+
>> > + */
>> > +
>> > +#include <config.h>
>> > +#include <common.h>
>> > +#include <dm.h>
>> > +#include <u-boot/rsa-mod-exp.h>
>> > +
>> > +int mod_exp_sw(struct udevice *dev, const uint8_t *sig, uint32_t sig_len,
>> > + struct key_prop *prop, uint8_t *out) {
>> > + int ret = 0;
>> > +
>> > + ret = rsa_mod_exp_sw(sig, sig_len, prop, out);
>> > +
>> > + if (ret) {
>> > + debug("%s: RSA failed to verify: %d\n", __func__, ret);
>> > + return ret;
>> > + }
>> > + return 0;
>> > +}
>> > +
>> > +static const struct rsa_ops rsa_ops_sw = {
>> > + .get_mod_exp = mod_exp_sw,
>> > +};
>> > +
>> > +U_BOOT_DRIVER(fsl_rsa) = {
>> > + .name = "rsa_sw",
>> > + .id = UCLASS_RSA,
>> > + .ops = &rsa_ops_sw,
>> > +};
>> > +
>> > +U_BOOT_DEVICE(rsa_sw) = {
>> > + .name = "rsa_sw",
>> > +};
>> > diff --git a/drivers/crypto/rsa/rsa_uclass.c
>> > b/drivers/crypto/rsa/rsa_uclass.c new file mode 100644 index
>> > 0000000..f4f4f39
>> > --- /dev/null
>> > +++ b/drivers/crypto/rsa/rsa_uclass.c
>> > @@ -0,0 +1,31 @@
>> > +/*
>> > + * (C) Copyright 2014 Freescale Semiconductor, Inc
>> > + * Author: Ruchika Gupta <ruchika.gu...@freescale.com>
>> > + *
>> > + * SPDX-License-Identifier: GPL-2.0+
>> > + */
>> > +
>> > +#include <common.h>
>> > +#include <dm.h>
>> > +#include <u-boot/rsa-mod-exp.h>
>> > +#include <errno.h>
>> > +#include <fdtdec.h>
>> > +#include <malloc.h>
>> > +#include <asm/io.h>
>> > +#include <linux/list.h>
>> > +
>> > +int rsa_mod_exp(struct udevice *dev, const uint8_t *sig, uint32_t sig_len,
>> > + struct key_prop *node, uint8_t *out) {
>> > + const struct rsa_ops *ops = device_get_ops(dev);
>> > +
>> > + if (!ops->get_mod_exp)
>> > + return -ENOSYS;
>> > +
>> > + return ops->get_mod_exp(dev, sig, sig_len, node, out); }
>> > +
>> > +UCLASS_DRIVER(rsa) = {
>> > + .id = UCLASS_RSA,
>> > + .name = "rsa",
>> > +};
>> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index
>> > f17c3c2..659369e 100644
>> > --- a/include/dm/uclass-id.h
>> > +++ b/include/dm/uclass-id.h
>> > @@ -33,6 +33,7 @@ enum uclass_id {
>> > UCLASS_I2C, /* I2C bus */
>> > UCLASS_I2C_GENERIC, /* Generic I2C device */
>> > UCLASS_I2C_EEPROM, /* I2C EEPROM device */
>> > + UCLASS_RSA , /* RSA Mod Exp device */
>>
>> Funny spacing here.
> Sorry, I will correct that.
>>
>> >
>> > UCLASS_COUNT,
>> > UCLASS_INVALID = -1,
>> > diff --git a/include/u-boot/rsa-mod-exp.h
>> > b/include/u-boot/rsa-mod-exp.h index 59cd9ea..7f7e196 100644
>> > --- a/include/u-boot/rsa-mod-exp.h
>> > +++ b/include/u-boot/rsa-mod-exp.h
>> > @@ -40,4 +40,44 @@ struct key_prop { int rsa_mod_exp_sw(const
>> > uint8_t *sig, uint32_t sig_len,
>> > struct key_prop *node, uint8_t *out);
>> >
>> > +/**
>> > + * rsa_mod_exp - Perform RSA Modular Exponentiation
>> > + *
>> > + * Operation: out[] = sig ^ exponent % modulus
>> > + *
>> > + * @udev: RSA Device
>> > + * @sig: RSA PKCS1.5 signature
>> > + * @sig_len: Length of signature in number of bytes
>> > + * @node: Node with RSA key elements like modulus, exponent, R^2,
>> n0inv
>> > + * @out: Result in form of byte array
>>
>> How big is this array?
> Thanks for pointing out. In next set of patch, I will change it to uint8_t
> **out and introduce a uin32_t *out_len also. Ideally the implemntor of this
> function should allocate out as per length required and return back the
> pointer and the allocated length. This will also require change in patch 1 of
> this series. I will change and float next version.
OK.
>
>>
>> > + */
>> > +int rsa_mod_exp(struct udevice *dev, const uint8_t *sig, uint32_t sig_len,
>> > + struct key_prop *node, uint8_t *out);
>> > +
>> > +/**
>> > + * struct struct rsa_ops - Driver model for RSA operations
>> > + *
>> > + * The uclass interface is implemented by all crypto devices which
>> > +use
>> > + * driver model.
>> > + */
>> > +struct rsa_ops {
>> > + /**
>> > + * Perform Modular Exponentiation
>> > + *
>> > + * Operation: out[] = sig ^ exponent % modulus
>> > + *
>> > + * @dev: RSA Device
>> > + * @sig: RSA PKCS1.5 signature
>> > + * @sig_len: Length of signature in number of bytes
>> > + * @node: Node with RSA key elements like modulus, exponent,
>> > + * R^2, n0inv
>> > + * @out: Result in form of byte array
>>
>> How big is this array?
> Same as above, I will change it to **outp and add a pointer for length
>>
>> > + * Returns: 0 if exponentiation is succesful, or a negative
>> > + value
>>
>> successful
> Ok
>>
>> > + * if it wasn't.
>> > + */
>> > + int (*get_mod_exp)(struct udevice *dev, const uint8_t *sig,
>>
>> mod_exp() is better I think, since it matches your function above.
> Ok. I will change it.
>>
>> > + uint32_t sig_len, struct key_prop *node,
>> > + uint8_t *out); };
>> > +
>> > #endif
>> > --
>> > 1.8.1.4
>> >
>>
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
