Re: [PATCH V2 4/4] crypto: Add Xilinx AES driver

[Corentin Labbe]

On Wed, Sep 04, 2019 at 05:40:22PM +0000, Kalyani Akula wrote:
> Hi Corentin,
> 
> Thanks for the review comments.
> Please find my response/queries inline.
> 
> > -----Original Message-----
> > From: Corentin Labbe <clabbe.montjoie@xxxxxxxxx>
> > Sent: Monday, September 2, 2019 12:29 PM
> > To: Kalyani Akula <kalyania@xxxxxxxxxx>
> > Cc: herbert@xxxxxxxxxxxxxxxxxxx; kstewart@xxxxxxxxxxxxxxxxxxx;
> > gregkh@xxxxxxxxxxxxxxxxxxx; tglx@xxxxxxxxxxxxx; pombredanne@xxxxxxxx;
> > linux-crypto@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > netdev@xxxxxxxxxxxxxxx; Kalyani Akula <kalyania@xxxxxxxxxx>
> > Subject: Re: [PATCH V2 4/4] crypto: Add Xilinx AES driver
> > 
> > On Sun, Sep 01, 2019 at 07:24:58PM +0530, Kalyani Akula wrote:
> > > This patch adds AES driver support for the Xilinx ZynqMP SoC.
> > >
> > > Signed-off-by: Kalyani Akula <kalyani.akula@xxxxxxxxxx>
> > > ---
> > 
> > Hello
> > 
> > I have some comment below
> > 
> > >  drivers/crypto/Kconfig          |  11 ++
> > >  drivers/crypto/Makefile         |   1 +
> > >  drivers/crypto/zynqmp-aes-gcm.c | 297
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 309 insertions(+)
> > >  create mode 100644 drivers/crypto/zynqmp-aes-gcm.c
> > >
> > > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig index
> > > 603413f..a0d058a 100644
> > > --- a/drivers/crypto/Kconfig
> > > +++ b/drivers/crypto/Kconfig
> > > @@ -677,6 +677,17 @@ config CRYPTO_DEV_ROCKCHIP
> > >  	  This driver interfaces with the hardware crypto accelerator.
> > >  	  Supporting cbc/ecb chainmode, and aes/des/des3_ede cipher mode.
> > >
> > > +config CRYPTO_DEV_ZYNQMP_AES
> > > +	tristate "Support for Xilinx ZynqMP AES hw accelerator"
> > > +	depends on ARCH_ZYNQMP || COMPILE_TEST
> > > +	select CRYPTO_AES
> > > +	select CRYPTO_SKCIPHER
> > > +	help
> > > +	  Xilinx ZynqMP has AES-GCM engine used for symmetric key
> > > +	  encryption and decryption. This driver interfaces with AES hw
> > > +	  accelerator. Select this if you want to use the ZynqMP module
> > > +	  for AES algorithms.
> > > +
> > >  config CRYPTO_DEV_MEDIATEK
> > >  	tristate "MediaTek's EIP97 Cryptographic Engine driver"
> > >  	depends on (ARM && ARCH_MEDIATEK) || COMPILE_TEST diff --git
> > > a/drivers/crypto/Makefile b/drivers/crypto/Makefile index
> > > afc4753..c99663a 100644
> > > --- a/drivers/crypto/Makefile
> > > +++ b/drivers/crypto/Makefile
> > > @@ -48,3 +48,4 @@ obj-$(CONFIG_CRYPTO_DEV_BCM_SPU) += bcm/
> > >  obj-$(CONFIG_CRYPTO_DEV_SAFEXCEL) += inside-secure/
> > >  obj-$(CONFIG_CRYPTO_DEV_ARTPEC6) += axis/  obj-y += hisilicon/
> > > +obj-$(CONFIG_CRYPTO_DEV_ZYNQMP_AES) += zynqmp-aes-gcm.o
> > > diff --git a/drivers/crypto/zynqmp-aes-gcm.c
> > > b/drivers/crypto/zynqmp-aes-gcm.c new file mode 100644 index
> > > 0000000..d65f038
> > > --- /dev/null
> > > +++ b/drivers/crypto/zynqmp-aes-gcm.c
> > > @@ -0,0 +1,297 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Xilinx ZynqMP AES Driver.
> > > + * Copyright (c) 2019 Xilinx Inc.
> > > + */
> > > +
> > > +#include <crypto/aes.h>
> > > +#include <crypto/scatterwalk.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/scatterlist.h>
> > > +#include <linux/firmware/xlnx-zynqmp.h>
> > > +
> > > +#define ZYNQMP_AES_IV_SIZE			12
> > > +#define ZYNQMP_AES_GCM_SIZE			16
> > > +#define ZYNQMP_AES_KEY_SIZE			32
> > > +
> > > +#define ZYNQMP_AES_DECRYPT			0
> > > +#define ZYNQMP_AES_ENCRYPT			1
> > > +
> > > +#define ZYNQMP_AES_KUP_KEY			0
> > > +#define ZYNQMP_AES_DEVICE_KEY			1
> > > +#define ZYNQMP_AES_PUF_KEY			2
> > > +
> > > +#define ZYNQMP_AES_GCM_TAG_MISMATCH_ERR		0x01
> > > +#define ZYNQMP_AES_SIZE_ERR			0x06
> > > +#define ZYNQMP_AES_WRONG_KEY_SRC_ERR		0x13
> > > +#define ZYNQMP_AES_PUF_NOT_PROGRAMMED		0xE300
> > > +
> > > +#define ZYNQMP_AES_BLOCKSIZE			0x04
> > > +
> > > +static const struct zynqmp_eemi_ops *eemi_ops; struct zynqmp_aes_dev
> > > +*aes_dd;
> > 
> > I still think that using a global variable for storing device driver data is bad.
> 
> I think storing the list of dd's would solve up the issue with global variable, but there is only one AES instance here.
> Please suggest
> 

Look what I do for amlogic driver https://patchwork.kernel.org/patch/11059633/.
I store the device driver in the instatiation of a crypto template.

[...]
> > > +static int zynqmp_setkey_blk(struct crypto_tfm *tfm, const u8 *key,
> > > +			     unsigned int len)
> > > +{
> > > +	struct zynqmp_aes_op *op = crypto_tfm_ctx(tfm);
> > > +
> > > +	if (((len != 1) && (len !=  ZYNQMP_AES_KEY_SIZE)) || (!key))
> > 
> > typo, two space
> 
> Will fix in the next version
> 
> > 
> > > +		return -EINVAL;
> > > +
> > > +	if (len == 1) {
> > > +		op->keytype = *key;
> > > +
> > > +		if ((op->keytype < ZYNQMP_AES_KUP_KEY) ||
> > > +			(op->keytype > ZYNQMP_AES_PUF_KEY))
> > > +			return -EINVAL;
> > > +
> > > +	} else if (len == ZYNQMP_AES_KEY_SIZE) {
> > > +		op->keytype = ZYNQMP_AES_KUP_KEY;
> > > +		op->keylen = len;
> > > +		memcpy(op->key, key, len);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > 
> > It seems your driver does not support AES keysize of 128/196, you need to
> > fallback in that case.
> 
> [Kalyani] In case of 128/196 keysize, returning the error would suffice ?
> Or still algorithm need to work ?
> If error is enough, it is taken care by this condition 
> if (((len != 1) && (len !=  ZYNQMP_AES_KEY_SIZE)) || (!key))

I think this problem just simply show us that your driver is not tested against crypto selftest.
I have tried to refuse 128/196 in my driver and I get:
alg: skcipher: cbc-aes-sun8i-ce setkey failed on test vector 0; expected_error=0, actual_error=-22, flags=0x1

So if your hardware lack 128/196 keys support, you must fallback to a software version.

Anyway please test your driver with crypto selftest enabled (and also CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y)

Regards