Re: [PATCH v4 3/3] crypto: Add Allwinner Security System crypto accelerator

2014-07-26 Thread Corentin LABBE 
Le 24/07/2014 15:38, Herbert Xu a écrit :
 On Thu, Jul 24, 2014 at 01:04:55PM +0200, Corentin LABBE wrote:
 Le 24/07/2014 08:00, Herbert Xu a écrit :
 On Sat, Jul 12, 2014 at 02:59:13PM +0200, LABBE Corentin wrote:

 +/* sunxi_hash_init: initialize request context
 + * Activate the SS, and configure it for MD5 or SHA1
 + */
 +int sunxi_hash_init(struct ahash_request *areq)
 +{
 +  const char *hash_type;
 +  struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
 +  struct sunxi_req_ctx *op = crypto_ahash_ctx(tfm);
 +
 +  mutex_lock(ss-lock);
 +
 +  hash_type = crypto_tfm_alg_name(areq-base.tfm);
 +
 +  op-byte_count = 0;
 +  op-nbwait = 0;
 +  op-waitbuf = 0;
 +
 +  /* Enable and configure SS for MD5 or SHA1 */
 +  if (strcmp(hash_type, sha1) == 0)
 +  op-mode = SS_OP_SHA1;
 +  else
 +  op-mode = SS_OP_MD5;
 +
 +  writel(op-mode | SS_ENABLED, ss-base + SS_CTL);
 +  return 0;

 The hash driver is completely broken.  You are modifying tfm
 ctx data which is shared by all users of a single tfm.  So
 if two users conduct hashes in parallel they will step all
 over each other.

 So where can I store data for each request ?
 
 Well, first of all you need to stop storing state in the hardware.
 After each operation the hardware may be used by some other user
 for a completely different hash request.  So leaving the hash state
 in the hardware is a no-no.
 
 If your hardware supports exporting the hash state then you just
 have to export it after each operation and reimporting before the
 next one.

Even if it is undocumented, the hardware seems to support it.
Since crypto_ahash_ctx is for a tfm, does ahash_request_ctx is the good place 
to store data ?
(after a call to crypto_ahash_set_reqsize in cra_init)

I have also seen export/import function, does I need to use it ?


 
 If your hardware is incapable of exporting partial hash state then
 you will have to use a software fallback for init/update.  If your
 hardware is incapable of importing partial hash state then you will
 also have to do finup/final using a software fallback.
 
 Cheers,
 

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] crypto: Add Allwinner Security System crypto accelerator

2014-07-25 Thread Maxime Ripard 
On Sat, Jul 12, 2014 at 02:59:13PM +0200, LABBE Corentin wrote:
 Add support for the Security System included in Allwinner SoC A20.
 The Security System is a hardware cryptographic accelerator that support 
 AES/MD5/SHA1/DES/3DES/PRNG algorithms.
 
 Signed-off-by: LABBE Corentin clabbe.montj...@gmail.com
 ---
  drivers/crypto/Kconfig|  17 ++
  drivers/crypto/Makefile   |   1 +
  drivers/crypto/sunxi-ss/Makefile  |   2 +
  drivers/crypto/sunxi-ss/sunxi-ss-cipher.c | 461 
 ++
  drivers/crypto/sunxi-ss/sunxi-ss-core.c   | 308 
  drivers/crypto/sunxi-ss/sunxi-ss-hash.c   | 241 
  drivers/crypto/sunxi-ss/sunxi-ss.h| 183 
  7 files changed, 1213 insertions(+)
  create mode 100644 drivers/crypto/sunxi-ss/Makefile
  create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
  create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-core.c
  create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-hash.c
  create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss.h
 
 diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
 index 03ccdb0..a2acda4 100644
 --- a/drivers/crypto/Kconfig
 +++ b/drivers/crypto/Kconfig
 @@ -418,4 +418,21 @@ config CRYPTO_DEV_MXS_DCP
 To compile this driver as a module, choose M here: the module
 will be called mxs-dcp.
  
 +config CRYPTO_DEV_SUNXI_SS
 + tristate Support for Allwinner Security System cryptographic 
 accelerator
 + depends on ARCH_SUNXI
 + select CRYPTO_MD5
 + select CRYPTO_SHA1
 + select CRYPTO_AES
 + select CRYPTO_DES
 + select CRYPTO_BLKCIPHER
 + help
 +   Some Allwinner SoC have a crypto accelerator named
 +   Security System. Select this if you want to use it.
 +   The Security System handle AES/DES/3DES ciphers in CBC mode
 +   and SHA1 and MD5 hash algorithms.
 +
 +   To compile this driver as a module, choose M here: the module
 +   will be called sunxi-ss.
 +
  endif # CRYPTO_HW
 diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
 index 482f090..855292a 100644
 --- a/drivers/crypto/Makefile
 +++ b/drivers/crypto/Makefile
 @@ -23,3 +23,4 @@ obj-$(CONFIG_CRYPTO_DEV_S5P) += s5p-sss.o
  obj-$(CONFIG_CRYPTO_DEV_SAHARA) += sahara.o
  obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o
  obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/
 +obj-$(CONFIG_CRYPTO_DEV_SUNXI_SS) += sunxi-ss/
 diff --git a/drivers/crypto/sunxi-ss/Makefile 
 b/drivers/crypto/sunxi-ss/Makefile
 new file mode 100644
 index 000..8bb287d
 --- /dev/null
 +++ b/drivers/crypto/sunxi-ss/Makefile
 @@ -0,0 +1,2 @@
 +obj-$(CONFIG_CRYPTO_DEV_SUNXI_SS) += sunxi-ss.o
 +sunxi-ss-y += sunxi-ss-core.o sunxi-ss-hash.o sunxi-ss-cipher.o
 diff --git a/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c 
 b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
 new file mode 100644
 index 000..c2422f7
 --- /dev/null
 +++ b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
 @@ -0,0 +1,461 @@
 +/*
 + * sunxi-ss-cipher.c - hardware cryptographic accelerator for Allwinner A20 
 SoC
 + *
 + * Copyright (C) 2013-2014 Corentin LABBE clabbe.montj...@gmail.com
 + *
 + * This file add support for AES cipher with 128,192,256 bits
 + * keysize in CBC mode.
 + *
 + * You could find the datasheet at
 + * http://dl.linux-sunxi.org/A20/A20%20User%20Manual%202013-03-22.pdf

It's already documented in Documentation/arm/sunxi/README, you don't
need this.

 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; either version 2 of the License, or
 + * (at your option) any later version.
 + */
 +#include sunxi-ss.h
 +
 +extern struct sunxi_ss_ctx *ss;
 +
 +static int sunxi_ss_cipher(struct ablkcipher_request *areq, u32 mode)
 +{
 + struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq);
 + struct sunxi_req_ctx *op = crypto_ablkcipher_ctx(tfm);
 + const char *cipher_type;
 +
 + cipher_type = crypto_tfm_alg_name(crypto_ablkcipher_tfm(tfm));
 +
 + if (areq-nbytes == 0) {
 + mutex_unlock(ss-lock);
 + return 0;
 + }
 +
 + if (areq-info == NULL) {
 + dev_err(ss-dev, ERROR: Empty IV\n);
 + mutex_unlock(ss-lock);
 + return -EINVAL;
 + }
 +
 + if (areq-src == NULL || areq-dst == NULL) {
 + dev_err(ss-dev, ERROR: Some SGs are NULL\n);
 + mutex_unlock(ss-lock);
 + return -EINVAL;
 + }
 +
 + if (strcmp(cbc(aes), cipher_type) == 0) {
 + op-mode |= SS_OP_AES | SS_CBC | SS_ENABLED | mode;
 + return sunxi_ss_aes_poll(areq);
 + }

Newline

 + if (strcmp(cbc(des), cipher_type) == 0) {
 + op-mode = SS_OP_DES | SS_CBC | SS_ENABLED | mode;
 + return sunxi_ss_des_poll(areq);
 + }

Ditto.

 + if (strcmp(cbc(des3_ede), cipher_type) == 0) {
 +

Re: [PATCH v4 3/3] crypto: Add Allwinner Security System crypto accelerator

2014-07-24 Thread Herbert Xu 
On Sat, Jul 12, 2014 at 02:59:13PM +0200, LABBE Corentin wrote:

 +/* sunxi_hash_init: initialize request context
 + * Activate the SS, and configure it for MD5 or SHA1
 + */
 +int sunxi_hash_init(struct ahash_request *areq)
 +{
 + const char *hash_type;
 + struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
 + struct sunxi_req_ctx *op = crypto_ahash_ctx(tfm);
 +
 + mutex_lock(ss-lock);
 +
 + hash_type = crypto_tfm_alg_name(areq-base.tfm);
 +
 + op-byte_count = 0;
 + op-nbwait = 0;
 + op-waitbuf = 0;
 +
 + /* Enable and configure SS for MD5 or SHA1 */
 + if (strcmp(hash_type, sha1) == 0)
 + op-mode = SS_OP_SHA1;
 + else
 + op-mode = SS_OP_MD5;
 +
 + writel(op-mode | SS_ENABLED, ss-base + SS_CTL);
 + return 0;

The hash driver is completely broken.  You are modifying tfm
ctx data which is shared by all users of a single tfm.  So
if two users conduct hashes in parallel they will step all
over each other.

Worse, the unpaired mutex_lock will quickly lead to dead locks.

You cannot assume that final will be called.

Cheers,
-- 
Email: Herbert Xu herb...@gondor.apana.org.au
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] crypto: Add Allwinner Security System crypto accelerator

2014-07-24 Thread Corentin LABBE 
Le 24/07/2014 08:00, Herbert Xu a écrit :
 On Sat, Jul 12, 2014 at 02:59:13PM +0200, LABBE Corentin wrote:

 +/* sunxi_hash_init: initialize request context
 + * Activate the SS, and configure it for MD5 or SHA1
 + */
 +int sunxi_hash_init(struct ahash_request *areq)
 +{
 +const char *hash_type;
 +struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
 +struct sunxi_req_ctx *op = crypto_ahash_ctx(tfm);
 +
 +mutex_lock(ss-lock);
 +
 +hash_type = crypto_tfm_alg_name(areq-base.tfm);
 +
 +op-byte_count = 0;
 +op-nbwait = 0;
 +op-waitbuf = 0;
 +
 +/* Enable and configure SS for MD5 or SHA1 */
 +if (strcmp(hash_type, sha1) == 0)
 +op-mode = SS_OP_SHA1;
 +else
 +op-mode = SS_OP_MD5;
 +
 +writel(op-mode | SS_ENABLED, ss-base + SS_CTL);
 +return 0;
 
 The hash driver is completely broken.  You are modifying tfm
 ctx data which is shared by all users of a single tfm.  So
 if two users conduct hashes in parallel they will step all
 over each other.

So where can I store data for each request ?

 
 Worse, the unpaired mutex_lock will quickly lead to dead locks.
 
 You cannot assume that final will be called.

An user reported an equivalent problem when using openssl speed test with 
cryptodev.
Does cryptoqueue is a good answer to that problem since the device could handle 
only one transformation at a time ?
And perhaps with cryptoqueue,  my first question is useless.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] crypto: Add Allwinner Security System crypto accelerator

2014-07-24 Thread Herbert Xu 
On Thu, Jul 24, 2014 at 01:04:55PM +0200, Corentin LABBE wrote:
 Le 24/07/2014 08:00, Herbert Xu a écrit :
  On Sat, Jul 12, 2014 at 02:59:13PM +0200, LABBE Corentin wrote:
 
  +/* sunxi_hash_init: initialize request context
  + * Activate the SS, and configure it for MD5 or SHA1
  + */
  +int sunxi_hash_init(struct ahash_request *areq)
  +{
  +  const char *hash_type;
  +  struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
  +  struct sunxi_req_ctx *op = crypto_ahash_ctx(tfm);
  +
  +  mutex_lock(ss-lock);
  +
  +  hash_type = crypto_tfm_alg_name(areq-base.tfm);
  +
  +  op-byte_count = 0;
  +  op-nbwait = 0;
  +  op-waitbuf = 0;
  +
  +  /* Enable and configure SS for MD5 or SHA1 */
  +  if (strcmp(hash_type, sha1) == 0)
  +  op-mode = SS_OP_SHA1;
  +  else
  +  op-mode = SS_OP_MD5;
  +
  +  writel(op-mode | SS_ENABLED, ss-base + SS_CTL);
  +  return 0;
  
  The hash driver is completely broken.  You are modifying tfm
  ctx data which is shared by all users of a single tfm.  So
  if two users conduct hashes in parallel they will step all
  over each other.
 
 So where can I store data for each request ?

Well, first of all you need to stop storing state in the hardware.
After each operation the hardware may be used by some other user
for a completely different hash request.  So leaving the hash state
in the hardware is a no-no.

If your hardware supports exporting the hash state then you just
have to export it after each operation and reimporting before the
next one.

If your hardware is incapable of exporting partial hash state then
you will have to use a software fallback for init/update.  If your
hardware is incapable of importing partial hash state then you will
also have to do finup/final using a software fallback.

Cheers,
-- 
Email: Herbert Xu herb...@gondor.apana.org.au
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] crypto: Add Allwinner Security System crypto accelerator

2014-07-23 Thread Herbert Xu 
On Sat, Jul 12, 2014 at 02:59:13PM +0200, LABBE Corentin wrote:
 Add support for the Security System included in Allwinner SoC A20.
 The Security System is a hardware cryptographic accelerator that support 
 AES/MD5/SHA1/DES/3DES/PRNG algorithms.
 
 Signed-off-by: LABBE Corentin clabbe.montj...@gmail.com

This is essentially a synchronous driver, no? If so please
switch to the blkcipher/shash interface.

Thanks,
-- 
Email: Herbert Xu herb...@gondor.apana.org.au
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] crypto: Add Allwinner Security System crypto accelerator

2014-07-23 Thread Maxime Ripard 
Hi,

On Wed, Jul 23, 2014 at 09:16:20PM +0800, Herbert Xu wrote:
 On Sat, Jul 12, 2014 at 02:59:13PM +0200, LABBE Corentin wrote:
  Add support for the Security System included in Allwinner SoC A20.
  The Security System is a hardware cryptographic accelerator that support 
  AES/MD5/SHA1/DES/3DES/PRNG algorithms.
  
  Signed-off-by: LABBE Corentin clabbe.montj...@gmail.com
 
 This is essentially a synchronous driver, no? If so please
 switch to the blkcipher/shash interface.

The exact opposite has been asked for during v1's review...

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH v4 3/3] crypto: Add Allwinner Security System crypto accelerator

2014-07-23 Thread Herbert Xu 
On Wed, Jul 23, 2014 at 03:48:57PM +0200, Maxime Ripard wrote:

 The exact opposite has been asked for during v1's review...

Indeed but unfortunately it was bogus advice.  The async interface
brings with it a lot of complexity which should be avoided unless
you actually need it.

Even if you use the sync interface your driver will still be
available to all async users.

Cheers,
-- 
Email: Herbert Xu herb...@gondor.apana.org.au
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 3/3] crypto: Add Allwinner Security System crypto accelerator

2014-07-12 Thread LABBE Corentin 
Add support for the Security System included in Allwinner SoC A20.
The Security System is a hardware cryptographic accelerator that support 
AES/MD5/SHA1/DES/3DES/PRNG algorithms.

Signed-off-by: LABBE Corentin clabbe.montj...@gmail.com
---
 drivers/crypto/Kconfig|  17 ++
 drivers/crypto/Makefile   |   1 +
 drivers/crypto/sunxi-ss/Makefile  |   2 +
 drivers/crypto/sunxi-ss/sunxi-ss-cipher.c | 461 ++
 drivers/crypto/sunxi-ss/sunxi-ss-core.c   | 308 
 drivers/crypto/sunxi-ss/sunxi-ss-hash.c   | 241 
 drivers/crypto/sunxi-ss/sunxi-ss.h| 183 
 7 files changed, 1213 insertions(+)
 create mode 100644 drivers/crypto/sunxi-ss/Makefile
 create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
 create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-core.c
 create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-hash.c
 create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss.h

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 03ccdb0..a2acda4 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -418,4 +418,21 @@ config CRYPTO_DEV_MXS_DCP
  To compile this driver as a module, choose M here: the module
  will be called mxs-dcp.
 
+config CRYPTO_DEV_SUNXI_SS
+   tristate Support for Allwinner Security System cryptographic 
accelerator
+   depends on ARCH_SUNXI
+   select CRYPTO_MD5
+   select CRYPTO_SHA1
+   select CRYPTO_AES
+   select CRYPTO_DES
+   select CRYPTO_BLKCIPHER
+   help
+ Some Allwinner SoC have a crypto accelerator named
+ Security System. Select this if you want to use it.
+ The Security System handle AES/DES/3DES ciphers in CBC mode
+ and SHA1 and MD5 hash algorithms.
+
+ To compile this driver as a module, choose M here: the module
+ will be called sunxi-ss.
+
 endif # CRYPTO_HW
diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
index 482f090..855292a 100644
--- a/drivers/crypto/Makefile
+++ b/drivers/crypto/Makefile
@@ -23,3 +23,4 @@ obj-$(CONFIG_CRYPTO_DEV_S5P) += s5p-sss.o
 obj-$(CONFIG_CRYPTO_DEV_SAHARA) += sahara.o
 obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o
 obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/
+obj-$(CONFIG_CRYPTO_DEV_SUNXI_SS) += sunxi-ss/
diff --git a/drivers/crypto/sunxi-ss/Makefile b/drivers/crypto/sunxi-ss/Makefile
new file mode 100644
index 000..8bb287d
--- /dev/null
+++ b/drivers/crypto/sunxi-ss/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_CRYPTO_DEV_SUNXI_SS) += sunxi-ss.o
+sunxi-ss-y += sunxi-ss-core.o sunxi-ss-hash.o sunxi-ss-cipher.o
diff --git a/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c 
b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
new file mode 100644
index 000..c2422f7
--- /dev/null
+++ b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
@@ -0,0 +1,461 @@
+/*
+ * sunxi-ss-cipher.c - hardware cryptographic accelerator for Allwinner A20 SoC
+ *
+ * Copyright (C) 2013-2014 Corentin LABBE clabbe.montj...@gmail.com
+ *
+ * This file add support for AES cipher with 128,192,256 bits
+ * keysize in CBC mode.
+ *
+ * You could find the datasheet at
+ * http://dl.linux-sunxi.org/A20/A20%20User%20Manual%202013-03-22.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include sunxi-ss.h
+
+extern struct sunxi_ss_ctx *ss;
+
+static int sunxi_ss_cipher(struct ablkcipher_request *areq, u32 mode)
+{
+   struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq);
+   struct sunxi_req_ctx *op = crypto_ablkcipher_ctx(tfm);
+   const char *cipher_type;
+
+   cipher_type = crypto_tfm_alg_name(crypto_ablkcipher_tfm(tfm));
+
+   if (areq-nbytes == 0) {
+   mutex_unlock(ss-lock);
+   return 0;
+   }
+
+   if (areq-info == NULL) {
+   dev_err(ss-dev, ERROR: Empty IV\n);
+   mutex_unlock(ss-lock);
+   return -EINVAL;
+   }
+
+   if (areq-src == NULL || areq-dst == NULL) {
+   dev_err(ss-dev, ERROR: Some SGs are NULL\n);
+   mutex_unlock(ss-lock);
+   return -EINVAL;
+   }
+
+   if (strcmp(cbc(aes), cipher_type) == 0) {
+   op-mode |= SS_OP_AES | SS_CBC | SS_ENABLED | mode;
+   return sunxi_ss_aes_poll(areq);
+   }
+   if (strcmp(cbc(des), cipher_type) == 0) {
+   op-mode = SS_OP_DES | SS_CBC | SS_ENABLED | mode;
+   return sunxi_ss_des_poll(areq);
+   }
+   if (strcmp(cbc(des3_ede), cipher_type) == 0) {
+   op-mode = SS_OP_3DES | SS_CBC | SS_ENABLED | mode;
+   return sunxi_ss_des_poll(areq);
+   }
+   dev_err(ss-dev, ERROR: Cipher %s not handled\n, cipher_type);
+   mutex_unlock(ss-lock);
+