| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 6 Sep 2019 09:04:35 +0200
From: Corentin Labbe <clabbe.montjoie@...il.com>
To: Kalyani Akula <kalyania@...inx.com>
Cc: "herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
"kstewart@...uxfoundation.org" <kstewart@...uxfoundation.org>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"pombredanne@...b.com" <pombredanne@...b.com>,
"linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Sarat Chand Savitala <saratcha@...inx.com>
Subject: Re: [PATCH V2 4/4] crypto: Add Xilinx AES driver
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@...il.com>
> > Sent: Monday, September 2, 2019 12:29 PM
> > To: Kalyani Akula <kalyania@...inx.com>
> > Cc: herbert@...dor.apana.org.au; kstewart@...uxfoundation.org;
> > gregkh@...uxfoundation.org; tglx@...utronix.de; pombredanne@...b.com;
> > linux-crypto@...r.kernel.org; linux-kernel@...r.kernel.org;
> > netdev@...r.kernel.org; Kalyani Akula <kalyania@...inx.com>
> > 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@...inx.com>
> > > ---
> >
> > 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
Powered by blists - more mailing lists