[RFC PATCH 0/5] Add Xilinx's ZynqMP AES driver support

[RFC PATCH 0/5] Add Xilinx's ZynqMP AES driver support

[Kalyani Akula]

This patch set adds support for
- dt-binding docs for Xilinx ZynqMP AES driver
- Adds device tree node for ZynqMP SHA3 driver
- Adds communication layer support for aes in zynqmp.c
- Adds user space interface for ALG_SET_KEY_TYPE
- Adds Xilinx ZynqMP driver for AES Algorithm

Kalyani Akula (5):
  dt-bindings: crypto: Add bindings for ZynqMP AES driver
  ARM64: zynqmp: Add Xilinix AES node.
  firmware: xilinx: Add ZynqMP aes API for AES functionality
  crypto: Adds user space interface for ALG_SET_KEY_TYPE
  crypto: Add Xilinx AES driver

 .../devicetree/bindings/crypto/xlnx,zynqmp-aes.txt |  12 +
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi             |   4 +
 crypto/af_alg.c                                    |   7 +
 crypto/algif_skcipher.c                            |   7 +
 crypto/blkcipher.c                                 |   9 +
 crypto/skcipher.c                                  |  18 ++
 drivers/crypto/Kconfig                             |  11 +
 drivers/crypto/Makefile                            |   1 +
 drivers/crypto/zynqmp-aes.c                        | 331 +++++++++++++++++++++
 drivers/firmware/xilinx/zynqmp.c                   |  24 ++
 include/crypto/if_alg.h                            |   2 +
 include/crypto/skcipher.h                          |  10 +
 include/linux/crypto.h                             |  12 +
 include/linux/firmware/xlnx-zynqmp.h               |   2 +
 include/uapi/linux/if_alg.h                        |   1 +
 15 files changed, 451 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/xlnx,zynqmp-aes.txt
 create mode 100644 drivers/crypto/zynqmp-aes.c

-- 
1.9.5

[RFC PATCH 1/5] dt-bindings: crypto: Add bindings for ZynqMP AES driver

[Kalyani Akula]

Add documentation to describe Xilinx ZynqMP AES driver
bindings.

Signed-off-by: Kalyani Akula <kalyani.akula@xilinx.com>
---
 Documentation/devicetree/bindings/crypto/xlnx,zynqmp-aes.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/crypto/xlnx,zynqmp-aes.txt

diff --git a/Documentation/devicetree/bindings/crypto/xlnx,zynqmp-aes.txt b/Documentation/devicetree/bindings/crypto/xlnx,zynqmp-aes.txt
new file mode 100644
index 0000000..226bfb9
--- /dev/null
+++ b/Documentation/devicetree/bindings/crypto/xlnx,zynqmp-aes.txt
@@ -0,0 +1,12 @@
+Xilinx ZynqMP AES hw acceleration support
+
+The ZynqMP PS-AES hw accelerator is used to encrypt/decrypt
+the given user data.
+
+Required properties:
+- compatible: should contain "xlnx,zynqmp-aes"
+
+Example:
+	zynqmp_aes {
+		compatible = "xlnx,zynqmp-aes";
+	};
-- 
1.9.5

[RFC PATCH 2/5] ARM64: zynqmp: Add Xilinix AES node.

[Kalyani Akula]

This patch adds a AES DT node for Xilinx ZynqMP SoC.

Signed-off-by: Kalyani Akula <kalyani.akula@xilinx.com>
---
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index fa4fd77..a3e8332 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -124,6 +124,10 @@
 			     <1 10 0xf08>;
 	};
 
+	xlnx_aes: zynqmp_aes {
+		compatible = "xlnx,zynqmp-aes";
+	};
+
 	amba_apu: amba-apu@0 {
 		compatible = "simple-bus";
 		#address-cells = <2>;
-- 
1.9.5

[RFC PATCH 3/5] firmware: xilinx: Add ZynqMP aes API for AES functionality

[Kalyani Akula]

Add ZynqMP firmware AES API to perform encryption/decryption
of given data.

Signed-off-by: Kalyani Akula <kalyani.akula@xilinx.com>
---
 drivers/firmware/xilinx/zynqmp.c     | 24 ++++++++++++++++++++++++
 include/linux/firmware/xlnx-zynqmp.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index 9a1c72a..332f98c1 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -469,6 +469,29 @@ static int zynqmp_pm_ioctl(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2,
 				   arg1, arg2, out);
 }
 
+/**
+ * zynqmp_pm_aes - Access AES hardware to encrypt/decrypt the data using
+ * AES-GCM core.
+ * @address:	Address of the AesParams structure.
+ * @out:	Returned output value
+ *
+ * Return:	Returns status, either success or error code.
+ */
+static int zynqmp_pm_aes_engine(const u64 address, u32 *out)
+{
+	u32 ret_payload[PAYLOAD_ARG_CNT];
+	int ret;
+
+	if (!out)
+		return -EINVAL;
+
+	ret = zynqmp_pm_invoke_fn(PM_SECURE_AES, upper_32_bits(address),
+				  lower_32_bits(address),
+				  0, 0, ret_payload);
+	*out = ret_payload[1];
+	return ret;
+}
+
 static const struct zynqmp_eemi_ops eemi_ops = {
 	.get_api_version = zynqmp_pm_get_api_version,
 	.query_data = zynqmp_pm_query_data,
@@ -482,6 +505,7 @@ static int zynqmp_pm_ioctl(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2,
 	.clock_setparent = zynqmp_pm_clock_setparent,
 	.clock_getparent = zynqmp_pm_clock_getparent,
 	.ioctl = zynqmp_pm_ioctl,
+	.aes = zynqmp_pm_aes_engine,
 };
 
 /**
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 3c3c28e..815fa30 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -45,6 +45,7 @@ enum pm_api_id {
 	PM_CLOCK_GETRATE,
 	PM_CLOCK_SETPARENT,
 	PM_CLOCK_GETPARENT,
+	PM_SECURE_AES = 47,
 };
 
 /* PMU-FW return status codes */
@@ -102,6 +103,7 @@ struct zynqmp_eemi_ops {
 	int (*clock_setparent)(u32 clock_id, u32 parent_id);
 	int (*clock_getparent)(u32 clock_id, u32 *parent_id);
 	int (*ioctl)(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2, u32 *out);
+	int (*aes)(const u64 address, u32 *out);
 };
 
 #if IS_REACHABLE(CONFIG_ARCH_ZYNQMP)
-- 
1.9.5

[RFC PATCH 4/5] crypto: Adds user space interface for ALG_SET_KEY_TYPE

[Kalyani Akula]

ALG_SET_KEY_TYPE requires caller to pass the key_type to be used
for AES encryption/decryption.

Sometimes the cipher key will be stored in the
device's hardware. So, there is a need to specify
the information about the key to use for AES operations.

In Xilinx ZynqMP SoC, below key types are available

1. Device key, which is flashed in the HW.

2. PUF KEK, which can be regenerated using the
   helper data programmed in the HW.

3. User supplied key.

So to choose the AES key to be used, this patch adds key-type attribute.

Signed-off-by: Kalyani Akula <kalyani.akula@xilinx.com>
---
 crypto/af_alg.c             |  7 +++++++
 crypto/algif_skcipher.c     |  7 +++++++
 crypto/blkcipher.c          |  9 +++++++++
 crypto/skcipher.c           | 18 ++++++++++++++++++
 include/crypto/if_alg.h     |  2 ++
 include/crypto/skcipher.h   | 10 ++++++++++
 include/linux/crypto.h      | 12 ++++++++++++
 include/uapi/linux/if_alg.h |  1 +
 8 files changed, 66 insertions(+)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 17eb09d..c3c0660 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -261,6 +261,13 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
 		if (!type->setauthsize)
 			goto unlock;
 		err = type->setauthsize(ask->private, optlen);
+		break;
+	case ALG_SET_KEY_TYPE:
+		if (sock->state == SS_CONNECTED)
+			goto unlock;
+		if (!type->setkeytype)
+			goto unlock;
+		err = type->setkeytype(ask->private, optval, optlen);
 	}
 
 unlock:
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index cfdaab2..9911a56 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -320,6 +320,12 @@ static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
 	return crypto_skcipher_setkey(private, key, keylen);
 }
 
+static int skcipher_setkeytype(void *private, const u8 *key,
+			       unsigned int keylen)
+{
+	return crypto_skcipher_setkeytype(private, key, keylen);
+}
+
 static void skcipher_sock_destruct(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
@@ -384,6 +390,7 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 	.bind		=	skcipher_bind,
 	.release	=	skcipher_release,
 	.setkey		=	skcipher_setkey,
+	.setkeytype	=	skcipher_setkeytype,
 	.accept		=	skcipher_accept_parent,
 	.accept_nokey	=	skcipher_accept_parent_nokey,
 	.ops		=	&algif_skcipher_ops,
diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
index c5398bd..8922f58 100644
--- a/crypto/blkcipher.c
+++ b/crypto/blkcipher.c
@@ -408,6 +408,14 @@ static int setkey(struct crypto_tfm *tfm, const u8 *key, unsigned int keylen)
 	return cipher->setkey(tfm, key, keylen);
 }
 
+static int setkeytype(struct crypto_tfm *tfm, const u8 *key,
+		      unsigned int keylen)
+{
+	struct blkcipher_alg *cipher = &tfm->__crt_alg->cra_blkcipher;
+
+	return cipher->setkeytype(tfm, key, keylen);
+}
+
 static int async_setkey(struct crypto_ablkcipher *tfm, const u8 *key,
 			unsigned int keylen)
 {
@@ -478,6 +486,7 @@ static int crypto_init_blkcipher_ops_sync(struct crypto_tfm *tfm)
 	unsigned long addr;
 
 	crt->setkey = setkey;
+	crt->setkeytype = setkeytype;
 	crt->encrypt = alg->encrypt;
 	crt->decrypt = alg->decrypt;
 
diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index 2a96929..6a2a0dd 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c
@@ -605,6 +605,23 @@ static int skcipher_setkey_blkcipher(struct crypto_skcipher *tfm,
 	return 0;
 }
 
+static int skcipher_setkeytype_blkcipher(struct crypto_skcipher *tfm,
+					 const u8 *key, unsigned int keylen)
+{
+	struct crypto_blkcipher **ctx = crypto_skcipher_ctx(tfm);
+	struct crypto_blkcipher *blkcipher = *ctx;
+	int err;
+
+	crypto_blkcipher_clear_flags(blkcipher, ~0);
+	crypto_blkcipher_set_flags(blkcipher, crypto_skcipher_get_flags(tfm) &
+			CRYPTO_TFM_REQ_MASK);
+	err = crypto_blkcipher_setkeytype(blkcipher, key, keylen);
+	crypto_skcipher_set_flags(tfm, crypto_blkcipher_get_flags(blkcipher) &
+			CRYPTO_TFM_RES_MASK);
+
+	return err;
+}
+
 static int skcipher_crypt_blkcipher(struct skcipher_request *req,
 				    int (*crypt)(struct blkcipher_desc *,
 						 struct scatterlist *,
@@ -671,6 +688,7 @@ static int crypto_init_skcipher_ops_blkcipher(struct crypto_tfm *tfm)
 	tfm->exit = crypto_exit_skcipher_ops_blkcipher;
 
 	skcipher->setkey = skcipher_setkey_blkcipher;
+	skcipher->setkeytype = skcipher_setkeytype_blkcipher;
 	skcipher->encrypt = skcipher_encrypt_blkcipher;
 	skcipher->decrypt = skcipher_decrypt_blkcipher;
 
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 482461d..202298e 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -51,6 +51,8 @@ struct af_alg_type {
 	void *(*bind)(const char *name, u32 type, u32 mask);
 	void (*release)(void *private);
 	int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+	int (*setkeytype)(void *private, const u8 *keytype,
+			  unsigned int keylen);
 	int (*accept)(void *private, struct sock *sk);
 	int (*accept_nokey)(void *private, struct sock *sk);
 	int (*setauthsize)(void *private, unsigned int authsize);
diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
index e555294..a6e1eda 100644
--- a/include/crypto/skcipher.h
+++ b/include/crypto/skcipher.h
@@ -42,6 +42,8 @@ struct skcipher_request {
 struct crypto_skcipher {
 	int (*setkey)(struct crypto_skcipher *tfm, const u8 *key,
 	              unsigned int keylen);
+	int (*setkeytype)(struct crypto_skcipher *tfm, const u8 *key,
+			  unsigned int keylen);
 	int (*encrypt)(struct skcipher_request *req);
 	int (*decrypt)(struct skcipher_request *req);
 
@@ -116,6 +118,8 @@ struct crypto_sync_skcipher {
 struct skcipher_alg {
 	int (*setkey)(struct crypto_skcipher *tfm, const u8 *key,
 	              unsigned int keylen);
+	int (*setkeytype)(struct crypto_skcipher *tfm, const u8 *key,
+			  unsigned int keylen);
 	int (*encrypt)(struct skcipher_request *req);
 	int (*decrypt)(struct skcipher_request *req);
 	int (*init)(struct crypto_skcipher *tfm);
@@ -444,6 +448,12 @@ static inline int crypto_sync_skcipher_setkey(struct crypto_sync_skcipher *tfm,
 	return crypto_skcipher_setkey(&tfm->base, key, keylen);
 }
 
+static inline int crypto_skcipher_setkeytype(struct crypto_skcipher *tfm,
+					     const u8 *key, unsigned int keylen)
+{
+	return tfm->setkeytype(tfm, key, keylen);
+}
+
 static inline unsigned int crypto_skcipher_default_keysize(
 	struct crypto_skcipher *tfm)
 {
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 902ec17..a47c9a8 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -268,6 +268,8 @@ struct ablkcipher_alg {
 struct blkcipher_alg {
 	int (*setkey)(struct crypto_tfm *tfm, const u8 *key,
 	              unsigned int keylen);
+	int (*setkeytype)(struct crypto_tfm *tfm, const u8 *keytype,
+			  unsigned int keylen);
 	int (*encrypt)(struct blkcipher_desc *desc,
 		       struct scatterlist *dst, struct scatterlist *src,
 		       unsigned int nbytes);
@@ -734,6 +736,8 @@ struct blkcipher_tfm {
 	void *iv;
 	int (*setkey)(struct crypto_tfm *tfm, const u8 *key,
 		      unsigned int keylen);
+	int (*setkeytype)(struct crypto_tfm *tfm, const u8 *key,
+			  unsigned int keylen);
 	int (*encrypt)(struct blkcipher_desc *desc, struct scatterlist *dst,
 		       struct scatterlist *src, unsigned int nbytes);
 	int (*decrypt)(struct blkcipher_desc *desc, struct scatterlist *dst,
@@ -1466,6 +1470,14 @@ static inline int crypto_blkcipher_setkey(struct crypto_blkcipher *tfm,
 						 key, keylen);
 }
 
+static inline int crypto_blkcipher_setkeytype(struct crypto_blkcipher *tfm,
+					      const u8 *key,
+					      unsigned int keylen)
+{
+	return crypto_blkcipher_crt(tfm)->setkeytype(crypto_blkcipher_tfm(tfm),
+						     key, keylen);
+}
+
 /**
  * crypto_blkcipher_encrypt() - encrypt plaintext
  * @desc: reference to the block cipher handle with meta data
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index bc2bcde..aa31b18 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,7 @@ struct af_alg_iv {
 #define ALG_SET_OP			3
 #define ALG_SET_AEAD_ASSOCLEN		4
 #define ALG_SET_AEAD_AUTHSIZE		5
+#define ALG_SET_KEY_TYPE		6
 
 /* Operations */
 #define ALG_OP_DECRYPT			0
-- 
1.9.5

Re: [RFC PATCH 4/5] crypto: Adds user space interface for ALG_SET_KEY_TYPE

[Stephan Mueller]

Am Donnerstag, 17. Januar 2019, 08:02:20 CET schrieb Kalyani Akula:

Hi Kalyani,

> ALG_SET_KEY_TYPE requires caller to pass the key_type to be used
> for AES encryption/decryption.
> 
> Sometimes the cipher key will be stored in the
> device's hardware. So, there is a need to specify
> the information about the key to use for AES operations.
> 
> In Xilinx ZynqMP SoC, below key types are available
> 
> 1. Device key, which is flashed in the HW.
> 
> 2. PUF KEK, which can be regenerated using the
>    helper data programmed in the HW.
> 
> 3. User supplied key.
> 
> So to choose the AES key to be used, this patch adds key-type attribute.

You expose your particular driver interface to user space. So, user space 
would need the details of you driver to know what to set. If another driver 
has such key type support, user space would need to know about that, too. I do 
not think this is a wise idea.

If we are going to have such a keytype selection, there must be a common user 
space interface for all drivers. I.e. define common key types the drivers then 
can map to their particular key type interface.

Besides, seem to be more a key handling issue. Wouldn't it make sense to 
rather have such issue solved with key rings than in the kernel crypto API?

Ciao
Stephan

RE: [RFC PATCH 4/5] crypto: Adds user space interface for ALG_SET_KEY_TYPE

[Kalyani Akula]

Hi Stephan,

Sorry for the delayed response.

Please find my response/doubts inline.

> -----Original Message-----
> From: Stephan Mueller <smueller@chronox.de>
> Sent: Thursday, January 17, 2019 5:04 PM
> To: Kalyani Akula <kalyania@xilinx.com>
> Cc: herbert@gondor.apana.org.au; davem@davemloft.net; linux-
> crypto@vger.kernel.org; linux-kernel@vger.kernel.org; Kalyani Akula
> <kalyania@xilinx.com>; Sarat Chand Savitala <saratcha@xilinx.com>
> Subject: Re: [RFC PATCH 4/5] crypto: Adds user space interface for
> ALG_SET_KEY_TYPE
> 
> Am Donnerstag, 17. Januar 2019, 08:02:20 CET schrieb Kalyani Akula:
> 
> Hi Kalyani,
> 
> > ALG_SET_KEY_TYPE requires caller to pass the key_type to be used for
> > AES encryption/decryption.
> >
> > Sometimes the cipher key will be stored in the device's hardware. So,
> > there is a need to specify the information about the key to use for
> > AES operations.
> >
> > In Xilinx ZynqMP SoC, below key types are available
> >
> > 1. Device key, which is flashed in the HW.
> >
> > 2. PUF KEK, which can be regenerated using the
> >    helper data programmed in the HW.
> >
> > 3. User supplied key.
> >
> > So to choose the AES key to be used, this patch adds key-type attribute.
> 
> You expose your particular driver interface to user space. So, user space
> would need the details of you driver to know what to set. If another driver
> has such key type support, user space would need to know about that, too. I
> do not think this is a wise idea.
> 
> If we are going to have such a keytype selection, there must be a common
> user space interface for all drivers. I.e. define common key types the drivers
> then can map to their particular key type interface.
[kalyani] Agree, now we have 3 basic key types and we can define them as below
eFuse key
PUF KEK
User supplied key
But for our upcoming platform there are multiple flavors of above keys, 
those may not be common for other drivers. 
I will check on this further and update.
> 
> Besides, seem to be more a key handling issue. Wouldn't it make sense to
> rather have such issue solved with key rings than in the kernel crypto API?
[kalyani] Can you please elaborate on this further ?
> 
> Ciao
> Stephan
> 

Re: [RFC PATCH 4/5] crypto: Adds user space interface for ALG_SET_KEY_TYPE

[Stephan Mueller]

Am Montag, 22. April 2019, 11:17:55 CEST schrieb Kalyani Akula:

Hi Kalyani,

> > Besides, seem to be more a key handling issue. Wouldn't it make sense to
> > rather have such issue solved with key rings than in the kernel crypto
> > API?
> 
> [kalyani] Can you please elaborate on this further ?

The kernel has a keyring support in security/keys which has a user space 
interface with keyutils. That interface is commonly used for any sort of key 
manipulation.

Ciao
Stephan

RE: [RFC PATCH 4/5] crypto: Adds user space interface for ALG_SET_KEY_TYPE

[Kalyani Akula]

Hi Stephan,

Keyrings is in-kernel key-management and retention facility. User can use it to manage keys used for applications. 

Xilinx cryptographic hardware has a mechanism to store keys in its internal hardware and do not have mechanism to read it back due to security reasons. 
It stores key internally in different forms like simple key, key encrypted with unique hardware DNA, key encrypted with hardware family key, 
key stored in eFUSEs/BBRAM etc. 
Based on security level expected, user can select one of the key for AES operation. Since AES hardware internally has access to these keys, 
user do not require to provide key to hardware, but need to tell which internal hardware key user application like to use for AES operation. 

Basic need is to pass information to AES hardware about which internal hardware key to be used for AES operation. 

I agree that from general use case perspective we are not selecting key type but selecting internal hardware keys provided by user. 
How about providing option to select custom hardware keys provided by hardware (AES_SEL_HW_KEY)?

Thanks
kalyani

> -----Original Message-----
> From: Stephan Mueller <smueller@chronox.de>
> Sent: Thursday, April 25, 2019 12:01 AM
> To: Kalyani Akula <kalyania@xilinx.com>
> Cc: herbert@gondor.apana.org.au; davem@davemloft.net; linux-
> crypto@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH 4/5] crypto: Adds user space interface for
> ALG_SET_KEY_TYPE
> 
> Am Montag, 22. April 2019, 11:17:55 CEST schrieb Kalyani Akula:
> 
> Hi Kalyani,
> 
> > > Besides, seem to be more a key handling issue. Wouldn't it make
> > > sense to rather have such issue solved with key rings than in the
> > > kernel crypto API?
> >
> > [kalyani] Can you please elaborate on this further ?
> 
> The kernel has a keyring support in security/keys which has a user space
> interface with keyutils. That interface is commonly used for any sort of key
> manipulation.
> 
> Ciao
> Stephan
> 

RE: [RFC PATCH 4/5] crypto: Adds user space interface for ALG_SET_KEY_TYPE

[Kalyani Akula]

Ping!!

> -----Original Message-----
> From: Kalyani Akula
> Sent: Wednesday, May 8, 2019 3:01 PM
> To: Stephan Mueller <smueller@chronox.de>
> Cc: herbert@gondor.apana.org.au; davem@davemloft.net; linux-
> crypto@vger.kernel.org; linux-kernel@vger.kernel.org; Sarat Chand Savitala
> <saratcha@xilinx.com>
> Subject: RE: [RFC PATCH 4/5] crypto: Adds user space interface for
> ALG_SET_KEY_TYPE
> 
> Hi Stephan,
> 
> Keyrings is in-kernel key-management and retention facility. User can use it to
> manage keys used for applications.
> 
> Xilinx cryptographic hardware has a mechanism to store keys in its internal
> hardware and do not have mechanism to read it back due to security reasons.
> It stores key internally in different forms like simple key, key encrypted with
> unique hardware DNA, key encrypted with hardware family key, key stored in
> eFUSEs/BBRAM etc.
> Based on security level expected, user can select one of the key for AES
> operation. Since AES hardware internally has access to these keys, user do not
> require to provide key to hardware, but need to tell which internal hardware key
> user application like to use for AES operation.
> 
> Basic need is to pass information to AES hardware about which internal
> hardware key to be used for AES operation.
> 
> I agree that from general use case perspective we are not selecting key type but
> selecting internal hardware keys provided by user.
> How about providing option to select custom hardware keys provided by
> hardware (AES_SEL_HW_KEY)?
> 
> Thanks
> kalyani
> 
> > -----Original Message-----
> > From: Stephan Mueller <smueller@chronox.de>
> > Sent: Thursday, April 25, 2019 12:01 AM
> > To: Kalyani Akula <kalyania@xilinx.com>
> > Cc: herbert@gondor.apana.org.au; davem@davemloft.net; linux-
> > crypto@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [RFC PATCH 4/5] crypto: Adds user space interface for
> > ALG_SET_KEY_TYPE
> >
> > Am Montag, 22. April 2019, 11:17:55 CEST schrieb Kalyani Akula:
> >
> > Hi Kalyani,
> >
> > > > Besides, seem to be more a key handling issue. Wouldn't it make
> > > > sense to rather have such issue solved with key rings than in the
> > > > kernel crypto API?
> > >
> > > [kalyani] Can you please elaborate on this further ?
> >
> > The kernel has a keyring support in security/keys which has a user
> > space interface with keyutils. That interface is commonly used for any
> > sort of key manipulation.
> >
> > Ciao
> > Stephan
> >

Re: [RFC PATCH 4/5] crypto: Adds user space interface for ALG_SET_KEY_TYPE

[Stephan Mueller]

Am Mittwoch, 8. Mai 2019, 11:31:08 CEST schrieb Kalyani Akula:

Hi Kalyani,

> Hi Stephan,
> 
> Keyrings is in-kernel key-management and retention facility. User can use it
> to manage keys used for applications.
> 
> Xilinx cryptographic hardware has a mechanism to store keys in its internal
> hardware and do not have mechanism to read it back due to security reasons.
> It stores key internally in different forms like simple key, key encrypted
> with unique hardware DNA, key encrypted with hardware family key, key
> stored in eFUSEs/BBRAM etc.
> Based on security level expected, user can select one of the key for AES
> operation. Since AES hardware internally has access to these keys, user do
> not require to provide key to hardware, but need to tell which internal
> hardware key user application like to use for AES operation.
> 
> Basic need is to pass information to AES hardware about which internal
> hardware key to be used for AES operation.
> 
> I agree that from general use case perspective we are not selecting key type
> but selecting internal hardware keys provided by user. How about providing
> option to select custom hardware keys provided by hardware
> (AES_SEL_HW_KEY)?

I am not intimately familiary with the keyring facility. Thus, let us ask the 
experts at the keyring mailing list :-)

> 
> Thanks
> kalyani
> 
> > -----Original Message-----
> > From: Stephan Mueller <smueller@chronox.de>
> > Sent: Thursday, April 25, 2019 12:01 AM
> > To: Kalyani Akula <kalyania@xilinx.com>
> > Cc: herbert@gondor.apana.org.au; davem@davemloft.net; linux-
> > crypto@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [RFC PATCH 4/5] crypto: Adds user space interface for
> > ALG_SET_KEY_TYPE
> > 
> > Am Montag, 22. April 2019, 11:17:55 CEST schrieb Kalyani Akula:
> > 
> > Hi Kalyani,
> > 
> > > > Besides, seem to be more a key handling issue. Wouldn't it make
> > > > sense to rather have such issue solved with key rings than in the
> > > > kernel crypto API?
> > > 
> > > [kalyani] Can you please elaborate on this further ?
> > 
> > The kernel has a keyring support in security/keys which has a user space
> > interface with keyutils. That interface is commonly used for any sort of
> > key manipulation.
> > 
> > Ciao
> > Stephan



Ciao
Stephan

RE: [RFC PATCH 4/5] crypto: Adds user space interface for ALG_SET_KEY_TYPE

[Kalyani Akula]

Ping!!

> -----Original Message-----
> From: Stephan Mueller <smueller@chronox.de>
> Sent: Friday, May 24, 2019 12:50 PM
> To: Kalyani Akula <kalyania@xilinx.com>; keyrings@vger.kernel.org
> Cc: herbert@gondor.apana.org.au; davem@davemloft.net; linux-
> crypto@vger.kernel.org; linux-kernel@vger.kernel.org; Sarat Chand Savitala
> <saratcha@xilinx.com>
> Subject: Re: [RFC PATCH 4/5] crypto: Adds user space interface for
> ALG_SET_KEY_TYPE
> 
> Am Mittwoch, 8. Mai 2019, 11:31:08 CEST schrieb Kalyani Akula:
> 
> Hi Kalyani,
> 
> > Hi Stephan,
> >
> > Keyrings is in-kernel key-management and retention facility. User can
> > use it to manage keys used for applications.
> >
> > Xilinx cryptographic hardware has a mechanism to store keys in its
> > internal hardware and do not have mechanism to read it back due to security
> reasons.
> > It stores key internally in different forms like simple key, key
> > encrypted with unique hardware DNA, key encrypted with hardware family
> > key, key stored in eFUSEs/BBRAM etc.
> > Based on security level expected, user can select one of the key for
> > AES operation. Since AES hardware internally has access to these keys,
> > user do not require to provide key to hardware, but need to tell which
> > internal hardware key user application like to use for AES operation.
> >
> > Basic need is to pass information to AES hardware about which internal
> > hardware key to be used for AES operation.
> >
> > I agree that from general use case perspective we are not selecting
> > key type but selecting internal hardware keys provided by user. How
> > about providing option to select custom hardware keys provided by
> > hardware (AES_SEL_HW_KEY)?
> 
> I am not intimately familiary with the keyring facility. Thus, let us ask the experts
> at the keyring mailing list :-)
> 
> >
> > Thanks
> > kalyani
> >
> > > -----Original Message-----
> > > From: Stephan Mueller <smueller@chronox.de>
> > > Sent: Thursday, April 25, 2019 12:01 AM
> > > To: Kalyani Akula <kalyania@xilinx.com>
> > > Cc: herbert@gondor.apana.org.au; davem@davemloft.net; linux-
> > > crypto@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [RFC PATCH 4/5] crypto: Adds user space interface for
> > > ALG_SET_KEY_TYPE
> > >
> > > Am Montag, 22. April 2019, 11:17:55 CEST schrieb Kalyani Akula:
> > >
> > > Hi Kalyani,
> > >
> > > > > Besides, seem to be more a key handling issue. Wouldn't it make
> > > > > sense to rather have such issue solved with key rings than in
> > > > > the kernel crypto API?
> > > >
> > > > [kalyani] Can you please elaborate on this further ?
> > >
> > > The kernel has a keyring support in security/keys which has a user
> > > space interface with keyutils. That interface is commonly used for
> > > any sort of key manipulation.
> > >
> > > Ciao
> > > Stephan
> 
> 
> 
> Ciao
> Stephan
> 

Re: [RFC PATCH 4/5] crypto: Adds user space interface for ALG_SET_KEY_TYPE

[Herbert Xu]

On Mon, Jun 10, 2019 at 05:20:58AM +0000, Kalyani Akula wrote:
> Ping!!

We already have existing drivers supporting hardware keys.  Please
check out how they're handling this.  You can grep for paes under
drivers/crypto.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [RFC PATCH 4/5] crypto: Adds user space interface for ALG_SET_KEY_TYPE

[Kalyani Akula]

Hi Herbert,

Paes driver is using key expansion algorithm to encrypt and decrypt the plaintext. HW capability of expanding the given plain key is checked based on the provide key length. Here the HW key is the expended version of plain key.

Xilinx AES hardware has a capability to take plain keys/encrypted keys ( these keys are user programmable but for security reasons they are not readable. Only AES accelerator has read access to these keys) stored on chip ( in eFuse/BBRAM etc, ) and used for AES encryption/decryption.
Xilinx software is giving the Customer, the flexibility to choose among the different on-chip AES keys.
So, we chosen a way to add AES_SEL_HW_KEY option.

In Paes driver , The ALG_SET_KEY interface is used to distinguish between HW Vs SW expansion of plain key based on the key_len. 

How about using same interface to distinguish between the User supplied key Vs HW key selection based on key_len parameter.

Thanks
kalyani  	

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Monday, June 10, 2019 12:05 PM
> To: Kalyani Akula <kalyania@xilinx.com>
> Cc: Stephan Mueller <smueller@chronox.de>; keyrings@vger.kernel.org;
> davem@davemloft.net; linux-crypto@vger.kernel.org; linux-
> kernel@vger.kernel.org; Sarat Chand Savitala <saratcha@xilinx.com>
> Subject: Re: [RFC PATCH 4/5] crypto: Adds user space interface for
> ALG_SET_KEY_TYPE
> 
> On Mon, Jun 10, 2019 at 05:20:58AM +0000, Kalyani Akula wrote:
> > Ping!!
> 
> We already have existing drivers supporting hardware keys.  Please check
> out how they're handling this.  You can grep for paes under drivers/crypto.
> 
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page:
> http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC PATCH 4/5] crypto: Adds user space interface for ALG_SET_KEY_TYPE

[Herbert Xu]

On Thu, Jul 11, 2019 at 09:25:38AM +0000, Kalyani Akula wrote:
>
> How about using same interface to distinguish between the User supplied key Vs HW key selection based on key_len parameter.

As long as you use the paes name instead of aes you can do whatever
you want with the key encoding.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[RFC PATCH 5/5] crypto: Add Xilinx AES driver

[Kalyani Akula]

This patch adds AES driver support for the Xilinx
ZynqMP SoC.

Signed-off-by: Kalyani Akula <kalyani.akula@xilinx.com>
---
 drivers/crypto/Kconfig      |  11 ++
 drivers/crypto/Makefile     |   1 +
 drivers/crypto/zynqmp-aes.c | 331 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 343 insertions(+)
 create mode 100644 drivers/crypto/zynqmp-aes.c

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 5a90075..66d9faa 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -667,6 +667,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_BLKCIPHER
+	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 8e7e225..991b343 100644
--- a/drivers/crypto/Makefile
+++ b/drivers/crypto/Makefile
@@ -47,3 +47,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.o
diff --git a/drivers/crypto/zynqmp-aes.c b/drivers/crypto/zynqmp-aes.c
new file mode 100644
index 0000000..3a38d2d
--- /dev/null
+++ b/drivers/crypto/zynqmp-aes.c
@@ -0,0 +1,331 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx ZynqMP AES Driver.
+ * Copyright (c) 2018 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/spinlock.h>
+#include <linux/firmware/xlnx-zynqmp.h>
+
+#define ZYNQMP_AES_QUEUE_LENGTH			1
+#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_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
+
+struct zynqmp_aes_dev {
+	struct list_head list;
+	struct device *dev;
+	/* the lock protects queue and dev list */
+	spinlock_t lock;
+	struct crypto_queue queue;
+};
+
+struct zynqmp_aes_op {
+	struct zynqmp_aes_dev *dd;
+	void *src;
+	void *dst;
+	int len;
+	u8 key[ZYNQMP_AES_KEY_SIZE];
+	u8 *iv;
+	u32 keylen;
+	u32 keytype;
+};
+
+struct zynqmp_aes_data {
+	u64 src;
+	u64 iv;
+	u64 key;
+	u64 dst;
+	u64 size;
+	u64 optype;
+	u64 keysrc;
+};
+
+struct zynqmp_aes_drv {
+	struct list_head dev_list;
+	/* the lock protects dev list */
+	spinlock_t lock;
+};
+
+static struct zynqmp_aes_drv zynqmp_aes = {
+	.dev_list = LIST_HEAD_INIT(zynqmp_aes.dev_list),
+	.lock = __SPIN_LOCK_UNLOCKED(zynqmp_aes.lock),
+};
+
+static struct zynqmp_aes_dev *zynqmp_aes_find_dev(struct zynqmp_aes_op *ctx)
+{
+	struct zynqmp_aes_dev *aes_dd = NULL;
+	struct zynqmp_aes_dev *tmp;
+
+	spin_lock_bh(&zynqmp_aes.lock);
+	if (!ctx->dd) {
+		list_for_each_entry(tmp, &zynqmp_aes.dev_list, list) {
+			aes_dd = tmp;
+			break;
+		}
+		ctx->dd = aes_dd;
+	} else {
+		aes_dd = ctx->dd;
+	}
+	spin_unlock_bh(&zynqmp_aes.lock);
+
+	return aes_dd;
+}
+
+static int zynqmp_setkey_blk(struct crypto_tfm *tfm, const u8 *key,
+			     unsigned int len)
+{
+	struct zynqmp_aes_op *op = crypto_tfm_ctx(tfm);
+
+	op->keylen = len;
+	memcpy(op->key, key, len);
+
+	return 0;
+}
+
+static int zynqmp_setkeytype(struct crypto_tfm *tfm, const u8 *keytype,
+			     unsigned int len)
+{
+	struct zynqmp_aes_op *op = crypto_tfm_ctx(tfm);
+
+	op->keytype = (u32)(*keytype);
+
+	return 0;
+}
+
+static int zynqmp_aes_xcrypt(struct blkcipher_desc *desc,
+			     struct scatterlist *dst,
+			     struct scatterlist *src,
+			     unsigned int nbytes,
+			     unsigned int flags)
+{
+	struct zynqmp_aes_op *op = crypto_blkcipher_ctx(desc->tfm);
+	struct zynqmp_aes_dev *dd = zynqmp_aes_find_dev(op);
+	const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+	int err, ret, copy_bytes, src_data = 0, dst_data = 0;
+	dma_addr_t dma_addr, dma_addr_buf;
+	struct zynqmp_aes_data *abuf;
+	struct blkcipher_walk walk;
+	unsigned int data_size;
+	size_t dma_size;
+	char *kbuf;
+
+	if (!eemi_ops || !eemi_ops->aes)
+		return -ENOTSUPP;
+
+	if (op->keytype == ZYNQMP_AES_KUP_KEY)
+		dma_size = nbytes + ZYNQMP_AES_KEY_SIZE
+			+ ZYNQMP_AES_IV_SIZE;
+	else
+		dma_size = nbytes + ZYNQMP_AES_IV_SIZE;
+
+	kbuf = dma_alloc_coherent(dd->dev, dma_size, &dma_addr, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+
+	abuf = dma_alloc_coherent(dd->dev, sizeof(struct zynqmp_aes_data),
+				  &dma_addr_buf, GFP_KERNEL);
+	if (!abuf) {
+		dma_free_coherent(dd->dev, dma_size, kbuf, dma_addr);
+		return -ENOMEM;
+	}
+
+	data_size = nbytes;
+	blkcipher_walk_init(&walk, dst, src, data_size);
+	err = blkcipher_walk_virt(desc, &walk);
+	op->iv = walk.iv;
+
+	while ((nbytes = walk.nbytes)) {
+		op->src = walk.src.virt.addr;
+		memcpy(kbuf + src_data, op->src, nbytes);
+		src_data = src_data + nbytes;
+		nbytes &= (ZYNQMP_AES_BLOCKSIZE - 1);
+		err = blkcipher_walk_done(desc, &walk, nbytes);
+	}
+	memcpy(kbuf + data_size, op->iv, ZYNQMP_AES_IV_SIZE);
+	abuf->src = dma_addr;
+	abuf->dst = dma_addr;
+	abuf->iv = abuf->src + data_size;
+	abuf->size = data_size - ZYNQMP_AES_GCM_SIZE;
+	abuf->optype = flags;
+	abuf->keysrc = op->keytype;
+
+	if (op->keytype == ZYNQMP_AES_KUP_KEY) {
+		memcpy(kbuf + data_size + ZYNQMP_AES_IV_SIZE,
+		       op->key, ZYNQMP_AES_KEY_SIZE);
+
+		abuf->key = abuf->src + data_size + ZYNQMP_AES_IV_SIZE;
+	} else {
+		abuf->key = 0;
+	}
+	eemi_ops->aes(dma_addr_buf, &ret);
+
+	if (ret != 0) {
+		switch (ret) {
+		case ZYNQMP_AES_GCM_TAG_MISMATCH_ERR:
+			dev_err(dd->dev, "ERROR: Gcm Tag mismatch\n\r");
+			break;
+		case ZYNQMP_AES_SIZE_ERR:
+			dev_err(dd->dev, "ERROR : Non word aligned data\n\r");
+			break;
+		case ZYNQMP_AES_WRONG_KEY_SRC_ERR:
+			dev_err(dd->dev, "ERROR: Wrong KeySrc, enable secure mode\n\r");
+			break;
+		case ZYNQMP_AES_PUF_NOT_PROGRAMMED:
+			dev_err(dd->dev, "ERROR: PUF is not registered\r\n");
+			break;
+		default:
+			dev_err(dd->dev, "ERROR: Invalid");
+			break;
+		}
+		goto END;
+	}
+	if (flags)
+		copy_bytes = data_size;
+	else
+		copy_bytes = data_size - ZYNQMP_AES_GCM_SIZE;
+
+	blkcipher_walk_init(&walk, dst, src, copy_bytes);
+	err = blkcipher_walk_virt(desc, &walk);
+
+	while ((nbytes = walk.nbytes)) {
+		memcpy(walk.dst.virt.addr, kbuf + dst_data, nbytes);
+		dst_data = dst_data + nbytes;
+		nbytes &= (ZYNQMP_AES_BLOCKSIZE - 1);
+		err = blkcipher_walk_done(desc, &walk, nbytes);
+	}
+END:
+	dma_free_coherent(dd->dev, dma_size, kbuf, dma_addr);
+	dma_free_coherent(dd->dev, sizeof(struct zynqmp_aes_data),
+			  abuf, dma_addr_buf);
+	return err;
+}
+
+static int zynqmp_aes_decrypt(struct blkcipher_desc *desc,
+			      struct scatterlist *dst,
+			      struct scatterlist *src,
+			      unsigned int nbytes)
+{
+	return zynqmp_aes_xcrypt(desc, dst, src, nbytes, ZYNQMP_AES_DECRYPT);
+}
+
+static int zynqmp_aes_encrypt(struct blkcipher_desc *desc,
+			      struct scatterlist *dst,
+			      struct scatterlist *src,
+			      unsigned int nbytes)
+{
+	return zynqmp_aes_xcrypt(desc, dst, src, nbytes, ZYNQMP_AES_ENCRYPT);
+}
+
+static struct crypto_alg zynqmp_alg = {
+	.cra_name		=	"xilinx-zynqmp-aes",
+	.cra_driver_name	=	"zynqmp-aes",
+	.cra_priority		=	400,
+	.cra_flags		=	CRYPTO_ALG_TYPE_BLKCIPHER |
+					CRYPTO_ALG_KERN_DRIVER_ONLY,
+	.cra_blocksize		=	ZYNQMP_AES_BLOCKSIZE,
+	.cra_ctxsize		=	sizeof(struct zynqmp_aes_op),
+	.cra_alignmask		=	15,
+	.cra_type		=	&crypto_blkcipher_type,
+	.cra_module		=	THIS_MODULE,
+	.cra_u			=	{
+	.blkcipher	=	{
+			.min_keysize	=	0,
+			.max_keysize	=	ZYNQMP_AES_KEY_SIZE,
+			.setkey		=	zynqmp_setkey_blk,
+			.setkeytype	=	zynqmp_setkeytype,
+			.encrypt	=	zynqmp_aes_encrypt,
+			.decrypt	=	zynqmp_aes_decrypt,
+			.ivsize		=	ZYNQMP_AES_IV_SIZE,
+		}
+	}
+};
+
+static const struct of_device_id zynqmp_aes_dt_ids[] = {
+	{ .compatible = "xlnx,zynqmp-aes" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, zynqmp_aes_dt_ids);
+
+static int zynqmp_aes_probe(struct platform_device *pdev)
+{
+	struct zynqmp_aes_dev *aes_dd;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	aes_dd = devm_kzalloc(dev, sizeof(*aes_dd), GFP_KERNEL);
+	if (!aes_dd)
+		return -ENOMEM;
+
+	aes_dd->dev = dev;
+	platform_set_drvdata(pdev, aes_dd);
+
+	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(44));
+	if (ret < 0) {
+		dev_err(dev, "no usable DMA configuration");
+		return ret;
+	}
+
+	INIT_LIST_HEAD(&aes_dd->list);
+	crypto_init_queue(&aes_dd->queue, ZYNQMP_AES_QUEUE_LENGTH);
+	list_add_tail(&aes_dd->list, &zynqmp_aes.dev_list);
+
+	ret = crypto_register_alg(&zynqmp_alg);
+	if (ret)
+		goto err_algs;
+
+	dev_info(dev, "AES Successfully Registered\n\r");
+	return 0;
+
+err_algs:
+	list_del(&aes_dd->list);
+	dev_err(dev, "initialization failed.\n");
+
+	return ret;
+}
+
+static int zynqmp_aes_remove(struct platform_device *pdev)
+{
+	struct zynqmp_aes_dev *aes_dd;
+
+	aes_dd = platform_get_drvdata(pdev);
+	if (!aes_dd)
+		return -ENODEV;
+	list_del(&aes_dd->list);
+	crypto_unregister_alg(&zynqmp_alg);
+	return 0;
+}
+
+static struct platform_driver xilinx_aes_driver = {
+	.probe = zynqmp_aes_probe,
+	.remove = zynqmp_aes_remove,
+	.driver = {
+		.name = "zynqmp_aes",
+		.of_match_table = of_match_ptr(zynqmp_aes_dt_ids),
+	},
+};
+
+module_platform_driver(xilinx_aes_driver);
+
+MODULE_DESCRIPTION("Xilinx ZynqMP AES hw acceleration support.");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Nava kishore Manne <nava.manne@xilinx.com>");
+MODULE_AUTHOR("Kalyani Akula <kalyani.akula@xilinx.com>");
-- 
1.9.5

Re: [RFC PATCH 5/5] crypto: Add Xilinx AES driver

[Corentin Labbe]

On Thu, Jan 17, 2019 at 12:32:21PM +0530, Kalyani Akula wrote:
> This patch adds AES driver support for the Xilinx
> ZynqMP SoC.
> 
> Signed-off-by: Kalyani Akula <kalyani.akula@xilinx.com>
> ---
>  drivers/crypto/Kconfig      |  11 ++
>  drivers/crypto/Makefile     |   1 +
>  drivers/crypto/zynqmp-aes.c | 331 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 343 insertions(+)
>  create mode 100644 drivers/crypto/zynqmp-aes.c
> 

Hello I have some comment below

> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 5a90075..66d9faa 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -667,6 +667,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_BLKCIPHER

BLKCIPHER is deprecated, please use 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 8e7e225..991b343 100644
> --- a/drivers/crypto/Makefile
> +++ b/drivers/crypto/Makefile
> @@ -47,3 +47,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.o
> diff --git a/drivers/crypto/zynqmp-aes.c b/drivers/crypto/zynqmp-aes.c
> new file mode 100644
> index 0000000..3a38d2d
> --- /dev/null
> +++ b/drivers/crypto/zynqmp-aes.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx ZynqMP AES Driver.
> + * Copyright (c) 2018 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/spinlock.h>
> +#include <linux/firmware/xlnx-zynqmp.h>
> +
> +#define ZYNQMP_AES_QUEUE_LENGTH			1
> +#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_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
> +
> +struct zynqmp_aes_dev {
> +	struct list_head list;
> +	struct device *dev;
> +	/* the lock protects queue and dev list */
> +	spinlock_t lock;
> +	struct crypto_queue queue;
> +};
> +
> +struct zynqmp_aes_op {
> +	struct zynqmp_aes_dev *dd;
> +	void *src;
> +	void *dst;
> +	int len;
> +	u8 key[ZYNQMP_AES_KEY_SIZE];
> +	u8 *iv;
> +	u32 keylen;
> +	u32 keytype;
> +};
> +
> +struct zynqmp_aes_data {
> +	u64 src;
> +	u64 iv;
> +	u64 key;
> +	u64 dst;
> +	u64 size;
> +	u64 optype;
> +	u64 keysrc;
> +};
> +
> +struct zynqmp_aes_drv {
> +	struct list_head dev_list;
> +	/* the lock protects dev list */
> +	spinlock_t lock;
> +};
> +
> +static struct zynqmp_aes_drv zynqmp_aes = {
> +	.dev_list = LIST_HEAD_INIT(zynqmp_aes.dev_list),
> +	.lock = __SPIN_LOCK_UNLOCKED(zynqmp_aes.lock),
> +};
> +
> +static struct zynqmp_aes_dev *zynqmp_aes_find_dev(struct zynqmp_aes_op *ctx)
> +{
> +	struct zynqmp_aes_dev *aes_dd = NULL;
> +	struct zynqmp_aes_dev *tmp;
> +
> +	spin_lock_bh(&zynqmp_aes.lock);
> +	if (!ctx->dd) {
> +		list_for_each_entry(tmp, &zynqmp_aes.dev_list, list) {
> +			aes_dd = tmp;
> +			break;
> +		}
> +		ctx->dd = aes_dd;
> +	} else {
> +		aes_dd = ctx->dd;
> +	}
> +	spin_unlock_bh(&zynqmp_aes.lock);
> +
> +	return aes_dd;
> +}
> +
> +static int zynqmp_setkey_blk(struct crypto_tfm *tfm, const u8 *key,
> +			     unsigned int len)
> +{
> +	struct zynqmp_aes_op *op = crypto_tfm_ctx(tfm);
> +

You should validate more the keylen.

> +	op->keylen = len;
> +	memcpy(op->key, key, len);
> +
> +	return 0;
> +}
> +
> +static int zynqmp_setkeytype(struct crypto_tfm *tfm, const u8 *keytype,
> +			     unsigned int len)
> +{
> +	struct zynqmp_aes_op *op = crypto_tfm_ctx(tfm);
> +
> +	op->keytype = (u32)(*keytype);

You should validate more what keytype is given.

> +
> +	return 0;
> +}
> +
> +static int zynqmp_aes_xcrypt(struct blkcipher_desc *desc,
> +			     struct scatterlist *dst,
> +			     struct scatterlist *src,
> +			     unsigned int nbytes,
> +			     unsigned int flags)
> +{
> +	struct zynqmp_aes_op *op = crypto_blkcipher_ctx(desc->tfm);
> +	struct zynqmp_aes_dev *dd = zynqmp_aes_find_dev(op);
> +	const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
> +	int err, ret, copy_bytes, src_data = 0, dst_data = 0;
> +	dma_addr_t dma_addr, dma_addr_buf;
> +	struct zynqmp_aes_data *abuf;
> +	struct blkcipher_walk walk;
> +	unsigned int data_size;
> +	size_t dma_size;
> +	char *kbuf;
> +
> +	if (!eemi_ops || !eemi_ops->aes)
> +		return -ENOTSUPP;
> +
> +	if (op->keytype == ZYNQMP_AES_KUP_KEY)
> +		dma_size = nbytes + ZYNQMP_AES_KEY_SIZE
> +			+ ZYNQMP_AES_IV_SIZE;
> +	else
> +		dma_size = nbytes + ZYNQMP_AES_IV_SIZE;
> +
> +	kbuf = dma_alloc_coherent(dd->dev, dma_size, &dma_addr, GFP_KERNEL);
> +	if (!kbuf)
> +		return -ENOMEM;
> +
> +	abuf = dma_alloc_coherent(dd->dev, sizeof(struct zynqmp_aes_data),
> +				  &dma_addr_buf, GFP_KERNEL);
> +	if (!abuf) {
> +		dma_free_coherent(dd->dev, dma_size, kbuf, dma_addr);
> +		return -ENOMEM;
> +	}
> +
> +	data_size = nbytes;
> +	blkcipher_walk_init(&walk, dst, src, data_size);
> +	err = blkcipher_walk_virt(desc, &walk);
> +	op->iv = walk.iv;
> +
> +	while ((nbytes = walk.nbytes)) {
> +		op->src = walk.src.virt.addr;
> +		memcpy(kbuf + src_data, op->src, nbytes);
> +		src_data = src_data + nbytes;
> +		nbytes &= (ZYNQMP_AES_BLOCKSIZE - 1);
> +		err = blkcipher_walk_done(desc, &walk, nbytes);
> +	}
> +	memcpy(kbuf + data_size, op->iv, ZYNQMP_AES_IV_SIZE);
> +	abuf->src = dma_addr;
> +	abuf->dst = dma_addr;
> +	abuf->iv = abuf->src + data_size;
> +	abuf->size = data_size - ZYNQMP_AES_GCM_SIZE;
> +	abuf->optype = flags;
> +	abuf->keysrc = op->keytype;
> +
> +	if (op->keytype == ZYNQMP_AES_KUP_KEY) {
> +		memcpy(kbuf + data_size + ZYNQMP_AES_IV_SIZE,
> +		       op->key, ZYNQMP_AES_KEY_SIZE);
> +
> +		abuf->key = abuf->src + data_size + ZYNQMP_AES_IV_SIZE;
> +	} else {
> +		abuf->key = 0;
> +	}
> +	eemi_ops->aes(dma_addr_buf, &ret);
> +
> +	if (ret != 0) {
> +		switch (ret) {
> +		case ZYNQMP_AES_GCM_TAG_MISMATCH_ERR:
> +			dev_err(dd->dev, "ERROR: Gcm Tag mismatch\n\r");
> +			break;
> +		case ZYNQMP_AES_SIZE_ERR:
> +			dev_err(dd->dev, "ERROR : Non word aligned data\n\r");
> +			break;
> +		case ZYNQMP_AES_WRONG_KEY_SRC_ERR:
> +			dev_err(dd->dev, "ERROR: Wrong KeySrc, enable secure mode\n\r");
> +			break;
> +		case ZYNQMP_AES_PUF_NOT_PROGRAMMED:
> +			dev_err(dd->dev, "ERROR: PUF is not registered\r\n");
> +			break;
> +		default:
> +			dev_err(dd->dev, "ERROR: Invalid");
> +			break;
> +		}
> +		goto END;
> +	}
> +	if (flags)
> +		copy_bytes = data_size;
> +	else
> +		copy_bytes = data_size - ZYNQMP_AES_GCM_SIZE;
> +
> +	blkcipher_walk_init(&walk, dst, src, copy_bytes);
> +	err = blkcipher_walk_virt(desc, &walk);
> +
> +	while ((nbytes = walk.nbytes)) {
> +		memcpy(walk.dst.virt.addr, kbuf + dst_data, nbytes);
> +		dst_data = dst_data + nbytes;
> +		nbytes &= (ZYNQMP_AES_BLOCKSIZE - 1);
> +		err = blkcipher_walk_done(desc, &walk, nbytes);
> +	}
> +END:
> +	dma_free_coherent(dd->dev, dma_size, kbuf, dma_addr);
> +	dma_free_coherent(dd->dev, sizeof(struct zynqmp_aes_data),
> +			  abuf, dma_addr_buf);

You should erase the key/IV from the bounce buffer.

> +	return err;
> +}
> +
> +static int zynqmp_aes_decrypt(struct blkcipher_desc *desc,
> +			      struct scatterlist *dst,
> +			      struct scatterlist *src,
> +			      unsigned int nbytes)
> +{
> +	return zynqmp_aes_xcrypt(desc, dst, src, nbytes, ZYNQMP_AES_DECRYPT);
> +}
> +
> +static int zynqmp_aes_encrypt(struct blkcipher_desc *desc,
> +			      struct scatterlist *dst,
> +			      struct scatterlist *src,
> +			      unsigned int nbytes)
> +{
> +	return zynqmp_aes_xcrypt(desc, dst, src, nbytes, ZYNQMP_AES_ENCRYPT);
> +}
> +
> +static struct crypto_alg zynqmp_alg = {
> +	.cra_name		=	"xilinx-zynqmp-aes",
> +	.cra_driver_name	=	"zynqmp-aes",

Since the driver is for GCM, GCM must appear in the name

> +	.cra_priority		=	400,
> +	.cra_flags		=	CRYPTO_ALG_TYPE_BLKCIPHER |
> +					CRYPTO_ALG_KERN_DRIVER_ONLY,
> +	.cra_blocksize		=	ZYNQMP_AES_BLOCKSIZE,
> +	.cra_ctxsize		=	sizeof(struct zynqmp_aes_op),
> +	.cra_alignmask		=	15,
> +	.cra_type		=	&crypto_blkcipher_type,
> +	.cra_module		=	THIS_MODULE,
> +	.cra_u			=	{
> +	.blkcipher	=	{
> +			.min_keysize	=	0,
> +			.max_keysize	=	ZYNQMP_AES_KEY_SIZE,
> +			.setkey		=	zynqmp_setkey_blk,
> +			.setkeytype	=	zynqmp_setkeytype,
> +			.encrypt	=	zynqmp_aes_encrypt,
> +			.decrypt	=	zynqmp_aes_decrypt,
> +			.ivsize		=	ZYNQMP_AES_IV_SIZE,
> +		}
> +	}
> +};
> +
> +static const struct of_device_id zynqmp_aes_dt_ids[] = {
> +	{ .compatible = "xlnx,zynqmp-aes" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, zynqmp_aes_dt_ids);
> +
> +static int zynqmp_aes_probe(struct platform_device *pdev)
> +{
> +	struct zynqmp_aes_dev *aes_dd;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	aes_dd = devm_kzalloc(dev, sizeof(*aes_dd), GFP_KERNEL);
> +	if (!aes_dd)
> +		return -ENOMEM;
> +
> +	aes_dd->dev = dev;
> +	platform_set_drvdata(pdev, aes_dd);
> +
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(44));
> +	if (ret < 0) {
> +		dev_err(dev, "no usable DMA configuration");
> +		return ret;
> +	}
> +
> +	INIT_LIST_HEAD(&aes_dd->list);
> +	crypto_init_queue(&aes_dd->queue, ZYNQMP_AES_QUEUE_LENGTH);
> +	list_add_tail(&aes_dd->list, &zynqmp_aes.dev_list);
> +

Same problem than the SHA3 driver, you init a queue without never using it.
And same problem for device list that seems useless at all in the current situation.

Regards