[PATCH] s390/crypto: fix aes ctr concurrency issue

[Harald Freudenberger]

The aes-ctr mode used one preallocated page without any concurrency
protection. When multiple threads run aes-ctr encryption or decryption
this could lead to data corruption.

The patch introduces locking for the preallocated page and alternatively
allocating and freeing of an temp page in concurrency situations.
---
 arch/s390/crypto/aes_s390.c |   55 --
 1 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
index 4363528..fcb5297 100644
--- a/arch/s390/crypto/aes_s390.c
+++ b/arch/s390/crypto/aes_s390.c
@@ -25,6 +25,7 @@
 #include linux/err.h
 #include linux/module.h
 #include linux/init.h
+#include linux/mutex.h
 #include crypt_s390.h
 
 #define AES_KEYLEN_128 1
@@ -32,6 +33,7 @@
 #define AES_KEYLEN_256 4
 
 static u8 *ctrblk;
+static DEFINE_MUTEX(ctrblk_lock);
 static char keylen_flag;
 
 struct s390_aes_ctx {
@@ -762,11 +764,25 @@ static int ctr_aes_crypt(struct blkcipher_desc *desc, 
long func,
unsigned int i, n, nbytes;
u8 buf[AES_BLOCK_SIZE];
u8 *out, *in;
+   u8 *ctrpage;
 
if (!walk-nbytes)
return ret;
 
-   memcpy(ctrblk, walk-iv, AES_BLOCK_SIZE);
+   if (mutex_trylock(ctrblk_lock)) {
+   /* ctrblk is now reserved for us */
+   ctrpage = ctrblk;
+   } else {
+   /* ctrblk is in use by someone else, alloc our own page */
+   ctrpage = (u8*) __get_free_page(GFP_ATOMIC);
+   if (!ctrpage) {
+   /* gfp failed, wait until ctrblk becomes available */
+   mutex_lock(ctrblk_lock);
+   ctrpage = ctrblk;
+   }
+   }
+
+   memcpy(ctrpage, walk-iv, AES_BLOCK_SIZE);
while ((nbytes = walk-nbytes) = AES_BLOCK_SIZE) {
out = walk-dst.virt.addr;
in = walk-src.virt.addr;
@@ -775,17 +791,19 @@ static int ctr_aes_crypt(struct blkcipher_desc *desc, 
long func,
n = (nbytes  PAGE_SIZE) ? PAGE_SIZE :
 nbytes  ~(AES_BLOCK_SIZE - 1);
for (i = AES_BLOCK_SIZE; i  n; i += AES_BLOCK_SIZE) {
-   memcpy(ctrblk + i, ctrblk + i - AES_BLOCK_SIZE,
+   memcpy(ctrpage + i, ctrpage + i - 
AES_BLOCK_SIZE,
   AES_BLOCK_SIZE);
-   crypto_inc(ctrblk + i, AES_BLOCK_SIZE);
+   crypto_inc(ctrpage + i, AES_BLOCK_SIZE);
+   }
+   ret = crypt_s390_kmctr(func, sctx-key, out, in, n, 
ctrpage);
+   if (ret  0 || ret != n) {
+   ret = -EIO;
+   goto out;
}
-   ret = crypt_s390_kmctr(func, sctx-key, out, in, n, 
ctrblk);
-   if (ret  0 || ret != n)
-   return -EIO;
if (n  AES_BLOCK_SIZE)
-   memcpy(ctrblk, ctrblk + n - AES_BLOCK_SIZE,
+   memcpy(ctrpage, ctrpage + n - AES_BLOCK_SIZE,
   AES_BLOCK_SIZE);
-   crypto_inc(ctrblk, AES_BLOCK_SIZE);
+   crypto_inc(ctrpage, AES_BLOCK_SIZE);
out += n;
in += n;
nbytes -= n;
@@ -799,14 +817,25 @@ static int ctr_aes_crypt(struct blkcipher_desc *desc, 
long func,
out = walk-dst.virt.addr;
in = walk-src.virt.addr;
ret = crypt_s390_kmctr(func, sctx-key, buf, in,
-  AES_BLOCK_SIZE, ctrblk);
-   if (ret  0 || ret != AES_BLOCK_SIZE)
-   return -EIO;
+  AES_BLOCK_SIZE, ctrpage);
+   if (ret  0 || ret != AES_BLOCK_SIZE) {
+   ret = -EIO;
+   goto out;
+   }
memcpy(out, buf, nbytes);
-   crypto_inc(ctrblk, AES_BLOCK_SIZE);
+   crypto_inc(ctrpage, AES_BLOCK_SIZE);
ret = blkcipher_walk_done(desc, walk, 0);
}
-   memcpy(walk-iv, ctrblk, AES_BLOCK_SIZE);
+   memcpy(walk-iv, ctrpage, AES_BLOCK_SIZE);
+
+out:
+   if (ctrpage == ctrblk) {
+   /* free the reservation for ctrblk now */
+   mutex_unlock(ctrblk_lock);
+   } else {
+   /* free the page allocated above */
+   free_page((unsigned long) ctrpage);
+   }
return ret;
 }
 
-- 
1.7.0.4


--
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] s390/crypto: fix aes ctr concurrency issue

[Harald Freudenberger]

The aes-ctr mode used one preallocated page without any concurrency
protection. When multiple threads run aes-ctr encryption or decryption
this could lead to data corruption.

The patch introduces locking for the preallocated page and alternatively
allocating and freeing of an temp page in concurrency situations.

Signed-off-by: Harald Freudenberger fre...@linux.vnet.ibm.com

Harald Freudenberger (1):
  s390/crypto: fix aes ctr concurrency issue

 arch/s390/crypto/aes_s390.c |   55 --
 1 files changed, 42 insertions(+), 13 deletions(-)


--
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] s390/crypto: fix aes ctr concurrency issue

[Harald Freudenberger]

On Thu, 2013-11-28 at 22:00 +0800, Herbert Xu wrote:
 On Tue, Nov 19, 2013 at 11:22:12AM +0100, Harald Freudenberger wrote:
  The aes-ctr mode used one preallocated page without any concurrency
  protection. When multiple threads run aes-ctr encryption or decryption
  this could lead to data corruption.
  
  The patch introduces locking for the preallocated page and alternatively
  allocating and freeing of an temp page in concurrency situations.
 
 You can't use mutex_lock because you may be in a non-sleepable
 context.  Perhaps just fall back to doing it block-by-block, like
 we do in aesni-intel on x86?

The first attempt to lock the mutex is done with mutex_trylock() which
should be safe for non-sleepable context. If this fails, an attempt is
made to allocate a fresh page __get_free_page(GFP_ATOMIC). If this also
fails, well what could be done then ? I think, it is valid to wait for
the preallocated page to get released with an mutex_lock(). Should I
really add code here for handling the 3rd level of the exceptional
path ?

 
 I have to say that your hardware has a funny way of doing CTR.
 Somebody was generalising out of their backside :)
 
 Thanks,



--
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] s390/crypto: fix aes ctr concurrency issue

[Harald Freudenberger]

On Fri, 2013-11-29 at 09:50 +0800, Herbert Xu wrote:
 On Thu, Nov 28, 2013 at 04:39:43PM +0100, Harald Freudenberger wrote:
 
   You can't use mutex_lock because you may be in a non-sleepable
   context.  Perhaps just fall back to doing it block-by-block, like
   we do in aesni-intel on x86?
  
  The first attempt to lock the mutex is done with mutex_trylock() which
  should be safe for non-sleepable context. If this fails, an attempt is
  made to allocate a fresh page __get_free_page(GFP_ATOMIC). If this also
  fails, well what could be done then ? I think, it is valid to wait for
  the preallocated page to get released with an mutex_lock(). Should I
  really add code here for handling the 3rd level of the exceptional
  path ?
 
 If it's wrong per se, how does hiding it behind two if's make it
 OK?
Sorry, I got the point now. Will do a rework of the patch according
to your hints. Thanks



--
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] crypto: s390 - fix concurrency issue in aes-ctr mode

[Harald Freudenberger]

The aes-ctr mode uses one preallocated page without any concurrency
protection. When multiple threads run aes-ctr encryption or decryption
this can lead to data corruption.

The patch introduces locking for the page and a fallback solution with
slower en/decryption performance in concurrency situations.

Signed-off-by: Harald Freudenberger fre...@linux.vnet.ibm.com
---
 arch/s390/crypto/aes_s390.c |   65 ++-
 1 file changed, 46 insertions(+), 19 deletions(-)

diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
index b3feabd..cf3c008 100644
--- a/arch/s390/crypto/aes_s390.c
+++ b/arch/s390/crypto/aes_s390.c
@@ -25,6 +25,7 @@
 #include linux/err.h
 #include linux/module.h
 #include linux/init.h
+#include linux/spinlock.h
 #include crypt_s390.h
 
 #define AES_KEYLEN_128 1
@@ -32,6 +33,7 @@
 #define AES_KEYLEN_256 4
 
 static u8 *ctrblk;
+static DEFINE_SPINLOCK(ctrblk_lock);
 static char keylen_flag;
 
 struct s390_aes_ctx {
@@ -758,43 +760,67 @@ static int ctr_aes_set_key(struct crypto_tfm *tfm, const 
u8 *in_key,
return aes_set_key(tfm, in_key, key_len);
 }
 
+static unsigned int __ctrblk_init(u8 *ctrptr, unsigned int nbytes)
+{
+   unsigned int i, n;
+
+   /* only use complete blocks, max. PAGE_SIZE */
+   n = (nbytes  PAGE_SIZE) ? PAGE_SIZE : nbytes  ~(AES_BLOCK_SIZE - 1);
+   for (i = AES_BLOCK_SIZE; i  n; i += AES_BLOCK_SIZE) {
+   memcpy(ctrptr + i, ctrptr + i - AES_BLOCK_SIZE,
+  AES_BLOCK_SIZE);
+   crypto_inc(ctrptr + i, AES_BLOCK_SIZE);
+   }
+   return n;
+}
+
 static int ctr_aes_crypt(struct blkcipher_desc *desc, long func,
 struct s390_aes_ctx *sctx, struct blkcipher_walk *walk)
 {
int ret = blkcipher_walk_virt_block(desc, walk, AES_BLOCK_SIZE);
-   unsigned int i, n, nbytes;
-   u8 buf[AES_BLOCK_SIZE];
-   u8 *out, *in;
+   unsigned int n, nbytes;
+   u8 buf[AES_BLOCK_SIZE], ctrbuf[AES_BLOCK_SIZE];
+   u8 *out, *in, *ctrptr = ctrbuf;
 
if (!walk-nbytes)
return ret;
 
-   memcpy(ctrblk, walk-iv, AES_BLOCK_SIZE);
+   if (spin_trylock(ctrblk_lock))
+   ctrptr = ctrblk;
+
+   memcpy(ctrptr, walk-iv, AES_BLOCK_SIZE);
while ((nbytes = walk-nbytes) = AES_BLOCK_SIZE) {
out = walk-dst.virt.addr;
in = walk-src.virt.addr;
while (nbytes = AES_BLOCK_SIZE) {
-   /* only use complete blocks, max. PAGE_SIZE */
-   n = (nbytes  PAGE_SIZE) ? PAGE_SIZE :
-nbytes  ~(AES_BLOCK_SIZE - 1);
-   for (i = AES_BLOCK_SIZE; i  n; i += AES_BLOCK_SIZE) {
-   memcpy(ctrblk + i, ctrblk + i - AES_BLOCK_SIZE,
-  AES_BLOCK_SIZE);
-   crypto_inc(ctrblk + i, AES_BLOCK_SIZE);
-   }
-   ret = crypt_s390_kmctr(func, sctx-key, out, in, n, 
ctrblk);
-   if (ret  0 || ret != n)
+   if (ctrptr == ctrblk)
+   n = __ctrblk_init(ctrptr, nbytes);
+   else
+   n = AES_BLOCK_SIZE;
+   ret = crypt_s390_kmctr(func, sctx-key, out, in,
+  n, ctrptr);
+   if (ret  0 || ret != n) {
+   if (ctrptr == ctrblk)
+   spin_unlock(ctrblk_lock);
return -EIO;
+   }
if (n  AES_BLOCK_SIZE)
-   memcpy(ctrblk, ctrblk + n - AES_BLOCK_SIZE,
+   memcpy(ctrptr, ctrptr + n - AES_BLOCK_SIZE,
   AES_BLOCK_SIZE);
-   crypto_inc(ctrblk, AES_BLOCK_SIZE);
+   crypto_inc(ctrptr, AES_BLOCK_SIZE);
out += n;
in += n;
nbytes -= n;
}
ret = blkcipher_walk_done(desc, walk, nbytes);
}
+   if (ctrptr == ctrblk) {
+   if (nbytes)
+   memcpy(ctrbuf, ctrptr, AES_BLOCK_SIZE);
+   else
+   memcpy(walk-iv, ctrptr, AES_BLOCK_SIZE);
+   spin_unlock(ctrblk_lock);
+   }
/*
 * final block may be  AES_BLOCK_SIZE, copy only nbytes
 */
@@ -802,14 +828,15 @@ static int ctr_aes_crypt(struct blkcipher_desc *desc, 
long func,
out = walk-dst.virt.addr;
in = walk-src.virt.addr;
ret = crypt_s390_kmctr(func, sctx-key, buf, in,
-  AES_BLOCK_SIZE, ctrblk

Re: [PATCH] crypto: s390 - fix concurrency issue in aes-ctr mode

[Harald Freudenberger]

On Thu, 2014-01-16 at 16:01 +0100, Harald Freudenberger wrote:

 The aes-ctr mode uses one preallocated page without any concurrency
 protection. When multiple threads run aes-ctr encryption or decryption
 this can lead to data corruption.
 
 The patch introduces locking for the page and a fallback solution with
 slower en/decryption performance in concurrency situations.
 
 Signed-off-by: Harald Freudenberger fre...@linux.vnet.ibm.com
 ---
  arch/s390/crypto/aes_s390.c |   65 
 ++-
  1 file changed, 46 insertions(+), 19 deletions(-)

Herbert, could you please add Cc: sta...@vger.kernel.org to this?

Thanks


--
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] crypto: s390 - fix des and des3_ede cbc concurrency issue

[Harald Freudenberger]

In s390 des and des3_ede cbc mode the iv value is not protected
against concurrency access and modifications from another running
en/decrypt operation which is using the very same tfm struct
instance. This fix copies the iv to the local stack before
the crypto operation and stores the value back when done.

Cc: sta...@vger.kernel.org
Signed-off-by: Harald Freudenberger fre...@linux.vnet.ibm.com
---
 arch/s390/crypto/des_s390.c |   26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/s390/crypto/des_s390.c b/arch/s390/crypto/des_s390.c
index 200f2a1..82a0a2a 100644
--- a/arch/s390/crypto/des_s390.c
+++ b/arch/s390/crypto/des_s390.c
@@ -105,29 +105,35 @@ static int ecb_desall_crypt(struct blkcipher_desc *desc, 
long func,
 }
 
 static int cbc_desall_crypt(struct blkcipher_desc *desc, long func,
-   u8 *iv, struct blkcipher_walk *walk)
+   struct blkcipher_walk *walk)
 {
+   struct s390_des_ctx *ctx = crypto_blkcipher_ctx(desc-tfm);
int ret = blkcipher_walk_virt(desc, walk);
unsigned int nbytes = walk-nbytes;
+   struct {
+   u8 iv[DES_BLOCK_SIZE];
+   u8 key[DES3_KEY_SIZE];
+   } param;
 
if (!nbytes)
goto out;
 
-   memcpy(iv, walk-iv, DES_BLOCK_SIZE);
+   memcpy(param.iv, walk-iv, DES_BLOCK_SIZE);
+   memcpy(param.key, ctx-key, DES3_KEY_SIZE);
do {
/* only use complete blocks */
unsigned int n = nbytes  ~(DES_BLOCK_SIZE - 1);
u8 *out = walk-dst.virt.addr;
u8 *in = walk-src.virt.addr;
 
-   ret = crypt_s390_kmc(func, iv, out, in, n);
+   ret = crypt_s390_kmc(func, param, out, in, n);
if (ret  0 || ret != n)
return -EIO;
 
nbytes = DES_BLOCK_SIZE - 1;
ret = blkcipher_walk_done(desc, walk, nbytes);
} while ((nbytes = walk-nbytes));
-   memcpy(walk-iv, iv, DES_BLOCK_SIZE);
+   memcpy(walk-iv, param.iv, DES_BLOCK_SIZE);
 
 out:
return ret;
@@ -179,22 +185,20 @@ static int cbc_des_encrypt(struct blkcipher_desc *desc,
   struct scatterlist *dst, struct scatterlist *src,
   unsigned int nbytes)
 {
-   struct s390_des_ctx *ctx = crypto_blkcipher_ctx(desc-tfm);
struct blkcipher_walk walk;
 
blkcipher_walk_init(walk, dst, src, nbytes);
-   return cbc_desall_crypt(desc, KMC_DEA_ENCRYPT, ctx-iv, walk);
+   return cbc_desall_crypt(desc, KMC_DEA_ENCRYPT, walk);
 }
 
 static int cbc_des_decrypt(struct blkcipher_desc *desc,
   struct scatterlist *dst, struct scatterlist *src,
   unsigned int nbytes)
 {
-   struct s390_des_ctx *ctx = crypto_blkcipher_ctx(desc-tfm);
struct blkcipher_walk walk;
 
blkcipher_walk_init(walk, dst, src, nbytes);
-   return cbc_desall_crypt(desc, KMC_DEA_DECRYPT, ctx-iv, walk);
+   return cbc_desall_crypt(desc, KMC_DEA_DECRYPT, walk);
 }
 
 static struct crypto_alg cbc_des_alg = {
@@ -327,22 +331,20 @@ static int cbc_des3_encrypt(struct blkcipher_desc *desc,
struct scatterlist *dst, struct scatterlist *src,
unsigned int nbytes)
 {
-   struct s390_des_ctx *ctx = crypto_blkcipher_ctx(desc-tfm);
struct blkcipher_walk walk;
 
blkcipher_walk_init(walk, dst, src, nbytes);
-   return cbc_desall_crypt(desc, KMC_TDEA_192_ENCRYPT, ctx-iv, walk);
+   return cbc_desall_crypt(desc, KMC_TDEA_192_ENCRYPT, walk);
 }
 
 static int cbc_des3_decrypt(struct blkcipher_desc *desc,
struct scatterlist *dst, struct scatterlist *src,
unsigned int nbytes)
 {
-   struct s390_des_ctx *ctx = crypto_blkcipher_ctx(desc-tfm);
struct blkcipher_walk walk;
 
blkcipher_walk_init(walk, dst, src, nbytes);
-   return cbc_desall_crypt(desc, KMC_TDEA_192_DECRYPT, ctx-iv, walk);
+   return cbc_desall_crypt(desc, KMC_TDEA_192_DECRYPT, walk);
 }
 
 static struct crypto_alg cbc_des3_alg = {
-- 
1.7.9.5


--
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] crypto: s390 - fix des and des3_ede ctr concurrency issue

[Harald Freudenberger]

In s390 des and 3des ctr mode there is one preallocated page
used to speed up the en/decryption. This page is not protected
against concurrent usage and thus there is a potential of data
corruption with multiple threads.

The fix introduces locking/unlocking the ctr page and a slower
fallback solution at concurrency situations.

Cc: sta...@vger.kernel.org
Signed-off-by: Harald Freudenberger fre...@linux.vnet.ibm.com
---
 arch/s390/crypto/des_s390.c |   69 ++-
 1 file changed, 48 insertions(+), 21 deletions(-)

diff --git a/arch/s390/crypto/des_s390.c b/arch/s390/crypto/des_s390.c
index 82a0a2a..0a5aac8 100644
--- a/arch/s390/crypto/des_s390.c
+++ b/arch/s390/crypto/des_s390.c
@@ -25,6 +25,7 @@
 #define DES3_KEY_SIZE  (3 * DES_KEY_SIZE)
 
 static u8 *ctrblk;
+static DEFINE_SPINLOCK(ctrblk_lock);
 
 struct s390_des_ctx {
u8 iv[DES_BLOCK_SIZE];
@@ -368,54 +369,80 @@ static struct crypto_alg cbc_des3_alg = {
}
 };
 
+static unsigned int __ctrblk_init(u8 *ctrptr, unsigned int nbytes)
+{
+   unsigned int i, n;
+
+   /* align to block size, max. PAGE_SIZE */
+   n = (nbytes  PAGE_SIZE) ? PAGE_SIZE : nbytes  ~(DES_BLOCK_SIZE - 1);
+   for (i = DES_BLOCK_SIZE; i  n; i += DES_BLOCK_SIZE) {
+   memcpy(ctrptr + i, ctrptr + i - DES_BLOCK_SIZE, DES_BLOCK_SIZE);
+   crypto_inc(ctrptr + i, DES_BLOCK_SIZE);
+   }
+   return n;
+}
+
 static int ctr_desall_crypt(struct blkcipher_desc *desc, long func,
-   struct s390_des_ctx *ctx, struct blkcipher_walk 
*walk)
+   struct s390_des_ctx *ctx,
+   struct blkcipher_walk *walk)
 {
int ret = blkcipher_walk_virt_block(desc, walk, DES_BLOCK_SIZE);
-   unsigned int i, n, nbytes;
-   u8 buf[DES_BLOCK_SIZE];
-   u8 *out, *in;
+   unsigned int n, nbytes;
+   u8 buf[DES_BLOCK_SIZE], ctrbuf[DES_BLOCK_SIZE];
+   u8 *out, *in, *ctrptr = ctrbuf;
+
+   if (!walk-nbytes)
+   return ret;
 
-   memcpy(ctrblk, walk-iv, DES_BLOCK_SIZE);
+   if (spin_trylock(ctrblk_lock))
+   ctrptr = ctrblk;
+
+   memcpy(ctrptr, walk-iv, DES_BLOCK_SIZE);
while ((nbytes = walk-nbytes) = DES_BLOCK_SIZE) {
out = walk-dst.virt.addr;
in = walk-src.virt.addr;
while (nbytes = DES_BLOCK_SIZE) {
-   /* align to block size, max. PAGE_SIZE */
-   n = (nbytes  PAGE_SIZE) ? PAGE_SIZE :
-   nbytes  ~(DES_BLOCK_SIZE - 1);
-   for (i = DES_BLOCK_SIZE; i  n; i += DES_BLOCK_SIZE) {
-   memcpy(ctrblk + i, ctrblk + i - DES_BLOCK_SIZE,
-  DES_BLOCK_SIZE);
-   crypto_inc(ctrblk + i, DES_BLOCK_SIZE);
-   }
-   ret = crypt_s390_kmctr(func, ctx-key, out, in, n, 
ctrblk);
-   if (ret  0 || ret != n)
+   if (ctrptr == ctrblk)
+   n = __ctrblk_init(ctrptr, nbytes);
+   else
+   n = DES_BLOCK_SIZE;
+   ret = crypt_s390_kmctr(func, ctx-key, out, in,
+  n, ctrptr);
+   if (ret  0 || ret != n) {
+   if (ctrptr == ctrblk)
+   spin_unlock(ctrblk_lock);
return -EIO;
+   }
if (n  DES_BLOCK_SIZE)
-   memcpy(ctrblk, ctrblk + n - DES_BLOCK_SIZE,
+   memcpy(ctrptr, ctrptr + n - DES_BLOCK_SIZE,
   DES_BLOCK_SIZE);
-   crypto_inc(ctrblk, DES_BLOCK_SIZE);
+   crypto_inc(ctrptr, DES_BLOCK_SIZE);
out += n;
in += n;
nbytes -= n;
}
ret = blkcipher_walk_done(desc, walk, nbytes);
}
-
+   if (ctrptr == ctrblk) {
+   if (nbytes)
+   memcpy(ctrbuf, ctrptr, DES_BLOCK_SIZE);
+   else
+   memcpy(walk-iv, ctrptr, DES_BLOCK_SIZE);
+   spin_unlock(ctrblk_lock);
+   }
/* final block may be  DES_BLOCK_SIZE, copy only nbytes */
if (nbytes) {
out = walk-dst.virt.addr;
in = walk-src.virt.addr;
ret = crypt_s390_kmctr(func, ctx-key, buf, in,
-  DES_BLOCK_SIZE, ctrblk);
+  DES_BLOCK_SIZE, ctrbuf);
if (ret  0 || ret != DES_BLOCK_SIZE)
return -EIO;
memcpy(out, buf, nbytes);
-   crypto_inc(ctrblk, DES_BLOCK_SIZE

[PATCH] s390/crypto: fix aes,des ctr mode concurrency finding.

[Harald Freudenberger]

An additional testcase found an issue with the last
series of patches applied: the fallback solution may
not save the iv value after operation. This very small
fix just makes sure the iv is copied back to the
walk/desc struct.

Signed-off-by: Harald Freudenberger fre...@linux.vnet.ibm.com
---
 arch/s390/crypto/aes_s390.c |3 +++
 arch/s390/crypto/des_s390.c |3 +++
 2 files changed, 6 insertions(+)

diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
index cf3c008..23223cd 100644
--- a/arch/s390/crypto/aes_s390.c
+++ b/arch/s390/crypto/aes_s390.c
@@ -820,6 +820,9 @@ static int ctr_aes_crypt(struct blkcipher_desc *desc, long 
func,
else
memcpy(walk-iv, ctrptr, AES_BLOCK_SIZE);
spin_unlock(ctrblk_lock);
+   } else {
+   if (!nbytes)
+   memcpy(walk-iv, ctrptr, AES_BLOCK_SIZE);
}
/*
 * final block may be  AES_BLOCK_SIZE, copy only nbytes
diff --git a/arch/s390/crypto/des_s390.c b/arch/s390/crypto/des_s390.c
index 0a5aac8..7acb77f 100644
--- a/arch/s390/crypto/des_s390.c
+++ b/arch/s390/crypto/des_s390.c
@@ -429,6 +429,9 @@ static int ctr_desall_crypt(struct blkcipher_desc *desc, 
long func,
else
memcpy(walk-iv, ctrptr, DES_BLOCK_SIZE);
spin_unlock(ctrblk_lock);
+   } else {
+   if (!nbytes)
+   memcpy(walk-iv, ctrptr, DES_BLOCK_SIZE);
}
/* final block may be  DES_BLOCK_SIZE, copy only nbytes */
if (nbytes) {
-- 
1.7.9.5


--
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] s390/crypto: Fix incorrect ghash icv buffer handling.

[Harald Freudenberger]

Multitheaded tests showed that the icv buffer in the current ghash
implementation is not handled correctly. A move of this working ghash
buffer value to the descriptor context fixed this. Code is tested and
verified with an multithreaded application via af_alg interface.

Signed-off-by: Harald Freudenberger fre...@linux.vnet.ibm.com
Signed-off-by: Gerald Schaefer geral...@linux.vnet.ibm.com
Reported-by: Herbert Xu herb...@gondor.apana.org.au
---
 arch/s390/crypto/ghash_s390.c |   25 +
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/arch/s390/crypto/ghash_s390.c b/arch/s390/crypto/ghash_s390.c
index 7940dc9..b258110 100644
--- a/arch/s390/crypto/ghash_s390.c
+++ b/arch/s390/crypto/ghash_s390.c
@@ -16,11 +16,12 @@
 #define GHASH_DIGEST_SIZE  16
 
 struct ghash_ctx {
-   u8 icv[16];
-   u8 key[16];
+   u8 key[GHASH_BLOCK_SIZE];
 };
 
 struct ghash_desc_ctx {
+   u8 icv[GHASH_BLOCK_SIZE];
+   u8 key[GHASH_BLOCK_SIZE];
u8 buffer[GHASH_BLOCK_SIZE];
u32 bytes;
 };
@@ -28,8 +29,10 @@ struct ghash_desc_ctx {
 static int ghash_init(struct shash_desc *desc)
 {
struct ghash_desc_ctx *dctx = shash_desc_ctx(desc);
+   struct ghash_ctx *ctx = crypto_shash_ctx(desc-tfm);
 
memset(dctx, 0, sizeof(*dctx));
+   memcpy(dctx-key, ctx-key, GHASH_BLOCK_SIZE);
 
return 0;
 }
@@ -45,7 +48,6 @@ static int ghash_setkey(struct crypto_shash *tfm,
}
 
memcpy(ctx-key, key, GHASH_BLOCK_SIZE);
-   memset(ctx-icv, 0, GHASH_BLOCK_SIZE);
 
return 0;
 }
@@ -54,7 +56,6 @@ static int ghash_update(struct shash_desc *desc,
 const u8 *src, unsigned int srclen)
 {
struct ghash_desc_ctx *dctx = shash_desc_ctx(desc);
-   struct ghash_ctx *ctx = crypto_shash_ctx(desc-tfm);
unsigned int n;
u8 *buf = dctx-buffer;
int ret;
@@ -70,7 +71,7 @@ static int ghash_update(struct shash_desc *desc,
src += n;
 
if (!dctx-bytes) {
-   ret = crypt_s390_kimd(KIMD_GHASH, ctx, buf,
+   ret = crypt_s390_kimd(KIMD_GHASH, dctx, buf,
  GHASH_BLOCK_SIZE);
if (ret != GHASH_BLOCK_SIZE)
return -EIO;
@@ -79,7 +80,7 @@ static int ghash_update(struct shash_desc *desc,
 
n = srclen  ~(GHASH_BLOCK_SIZE - 1);
if (n) {
-   ret = crypt_s390_kimd(KIMD_GHASH, ctx, src, n);
+   ret = crypt_s390_kimd(KIMD_GHASH, dctx, src, n);
if (ret != n)
return -EIO;
src += n;
@@ -94,7 +95,7 @@ static int ghash_update(struct shash_desc *desc,
return 0;
 }
 
-static int ghash_flush(struct ghash_ctx *ctx, struct ghash_desc_ctx *dctx)
+static int ghash_flush(struct ghash_desc_ctx *dctx)
 {
u8 *buf = dctx-buffer;
int ret;
@@ -104,24 +105,24 @@ static int ghash_flush(struct ghash_ctx *ctx, struct 
ghash_desc_ctx *dctx)
 
memset(pos, 0, dctx-bytes);
 
-   ret = crypt_s390_kimd(KIMD_GHASH, ctx, buf, GHASH_BLOCK_SIZE);
+   ret = crypt_s390_kimd(KIMD_GHASH, dctx, buf, GHASH_BLOCK_SIZE);
if (ret != GHASH_BLOCK_SIZE)
return -EIO;
+
+   dctx-bytes = 0;
}
 
-   dctx-bytes = 0;
return 0;
 }
 
 static int ghash_final(struct shash_desc *desc, u8 *dst)
 {
struct ghash_desc_ctx *dctx = shash_desc_ctx(desc);
-   struct ghash_ctx *ctx = crypto_shash_ctx(desc-tfm);
int ret;
 
-   ret = ghash_flush(ctx, dctx);
+   ret = ghash_flush(dctx);
if (!ret)
-   memcpy(dst, ctx-icv, GHASH_BLOCK_SIZE);
+   memcpy(dst, dctx-icv, GHASH_BLOCK_SIZE);
return ret;
 }
 
-- 
1.7.9.5


--
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] Added one larger ghash testvector (400 bytes) to the testmgr.

[Harald Freudenberger]

Signed-off-by: Harald Freudenberger fre...@linux.vnet.ibm.com
---
 crypto/testmgr.h |   59 +-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/crypto/testmgr.h b/crypto/testmgr.h
index 90f43b9..6003143 100644
--- a/crypto/testmgr.h
+++ b/crypto/testmgr.h
@@ -1822,7 +1822,7 @@ static struct hash_testvec tgr128_tv_template[] = {
},
 };
 
-#define GHASH_TEST_VECTORS 5
+#define GHASH_TEST_VECTORS 6
 
 static struct hash_testvec ghash_tv_template[] =
 {
@@ -1875,6 +1875,63 @@ static struct hash_testvec ghash_tv_template[] =
.psize  = 20,
.digest = \xf8\x94\x87\x2a\x4b\x63\x99\x28
  \x23\xf7\x93\xf7\x19\xf5\x96\xd9,
+   }, {
+   .key= \x0a\x1b\x2c\x3d\x4e\x5f\x64\x71
+   \x82\x93\xa4\xb5\xc6\xd7\xe8\xf9,
+   .ksize  = 16,
+   .plaintext = \x56\x6f\x72\x20\x6c\x61\x75\x74
+   \x65\x72\x20\x4c\x61\x75\x73\x63
+   \x68\x65\x6e\x20\x75\x6e\x64\x20
+   \x53\x74\x61\x75\x6e\x65\x6e\x20
+   \x73\x65\x69\x20\x73\x74\x69\x6c
+   \x6c\x2c\x0a\x64\x75\x20\x6d\x65
+   \x69\x6e\x20\x74\x69\x65\x66\x74
+   \x69\x65\x66\x65\x73\x20\x4c\x65
+   \x62\x65\x6e\x3b\x0a\x64\x61\x73
+   \x73\x20\x64\x75\x20\x77\x65\x69
+   \xc3\x9f\x74\x20\x77\x61\x73\x20
+   \x64\x65\x72\x20\x57\x69\x6e\x64
+   \x20\x64\x69\x72\x20\x77\x69\x6c
+   \x6c\x2c\x0a\x65\x68\x20\x6e\x6f
+   \x63\x68\x20\x64\x69\x65\x20\x42
+   \x69\x72\x6b\x65\x6e\x20\x62\x65
+   \x62\x65\x6e\x2e\x0a\x0a\x55\x6e
+   \x64\x20\x77\x65\x6e\x6e\x20\x64
+   \x69\x72\x20\x65\x69\x6e\x6d\x61
+   \x6c\x20\x64\x61\x73\x20\x53\x63
+   \x68\x77\x65\x69\x67\x65\x6e\x20
+   \x73\x70\x72\x61\x63\x68\x2c\x0a
+   \x6c\x61\x73\x73\x20\x64\x65\x69
+   \x6e\x65\x20\x53\x69\x6e\x6e\x65
+   \x20\x62\x65\x73\x69\x65\x67\x65
+   \x6e\x2e\x0a\x4a\x65\x64\x65\x6d
+   \x20\x48\x61\x75\x63\x68\x65\x20
+   \x67\x69\x62\x74\x20\x64\x69\x63
+   \x68\x2c\x20\x67\x69\x62\x20\x6e
+   \x61\x63\x68\x2c\x0a\x65\x72\x20
+   \x77\x69\x72\x64\x20\x64\x69\x63
+   \x68\x20\x6c\x69\x65\x62\x65\x6e
+   \x20\x75\x6e\x64\x20\x77\x69\x65
+   \x67\x65\x6e\x2e\x0a\x0a\x55\x6e
+   \x64\x20\x64\x61\x6e\x6e\x20\x6d
+   \x65\x69\x6e\x65\x20\x53\x65\x65
+   \x6c\x65\x20\x73\x65\x69\x74\x20
+   \x77\x65\x69\x74\x2c\x20\x73\x65
+   \x69\x20\x77\x65\x69\x74\x2c\x0a
+   \x64\x61\x73\x73\x20\x64\x69\x72
+   \x20\x64\x61\x73\x20\x4c\x65\x62
+   \x65\x6e\x20\x67\x65\x6c\x69\x6e
+   \x67\x65\x2c\x0a\x62\x72\x65\x69
+   \x74\x65\x20\x64\x69\x63\x68\x20
+   \x77\x69\x65\x20\x65\x69\x6e\x20
+   \x46\x65\x69\x65\x72\x6b\x6c\x65
+   \x69\x64\x0a\xc3\xbc\x62\x65\x72
+   \x20\x64\x69\x65\x20\x73\x69\x6e
+   \x6e\x65\x6e\x64\x65\x6e\x20\x44
+   \x69\x6e\x67\x65\x2e\x2e\x2e\x0a,
+   .psize  = 400,
+   .digest = \xad\xb1\xc1\xe9\x56\x70\x31\x1d
+   \xbb\x5b\xdf\x5e\x70\x72\x1a\x57,
},
 };
 
-- 
1.7.9.5


--
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

crypto: hang in crypto_larval_lookup

[Harald Freudenberger]

Hello all

I am currently following a hang at modprobe aes_s390 where
crypto_register_alg() does not come back for the xts(aes) algorithm.

The registration is waiting forever in algapi.c crypto_wait_for_test() but
the completion never occurs. The cryptomgr is triggering a test via
kthread_run to invoce cryptomgr_probe and this thread is calling the
create() function of the xts template (file xts.c). Following this thread
it comes down to api.c crypto_larval_lookup(name="aes") which is first
requesting the module "crypto-aes" via request_module() successful and then
blocking forever in requesting the module "crypto-aes-all".

The xts(aes) has at registration CRYPTO_ALG_NEED_FALLBACK flag on.

This problem is seen since about 6 weeks now, first only on the linux next
kernel. Now it appers on the 4.10-rc kernels as well. And I still have no
idea on how this could be fixed or what's wrong with just the xts
registration (ecb, cbc, ctr work fine).

Any ideas or hints?
Thank's in advance.

regards
Harald Freudenberger

Re: crypto: hang in crypto_larval_lookup

[Harald Freudenberger]

On 02/24/2017 11:32 AM, Harald Freudenberger wrote:
> On 02/24/2017 09:42 AM, Harald Freudenberger wrote:
...
>> ...
>> Feb 24 09:28:10 r35lp49 kernel: 
>> ->crypto_larval_lookup(name=aes,type=0x0405,mask=0x248c)
>> Feb 24 09:28:10 r35lp49 kernel:   crypto_larval_lookup calling 
>> crypto_alg_lookup(aes,0x0405,0x248c)
>> Feb 24 09:28:10 r35lp49 kernel: ->__crypto_alg_lookup(name=aes)
>> Feb 24 09:28:10 r35lp49 kernel: <-__crypto_alg_lookup(name=aes) alg= 
>>  (null)
>> Feb 24 09:28:10 r35lp49 kernel:   crypto_larval_lookup calling 
>> request_module(crypto-aes)
>> Feb 24 09:28:10 r35lp49 kernel:   crypto_larval_lookup calling 
>> request_module(crypto-aes-all)
>>
>> type=0x0405 = CRYPTO_ALG_TESTED, CRYPTO_ALG_TYPE_SKCIPHER
>> mask=0x248c = CRYPTO_ALG_INTERNAL, CRYPTO_ALG_TESTED, CRYPTO_ALG_ASYNC, 
>> CRYPTO_ALG_TYPE_BLKCIPHER_MASK
>>
> I catched it: Thanks Herbert for your hint. The aes algorith registers with:
>   .cra_flags = CRYPTO_ALG_TYPE_CIPHER |CRYPTO_ALG_NEED_FALLBACK
> so later at __crypto_alg_lookup(aes, 0x0405, 0x248c)
> this alg is not choosen because the check
>   if ((q->cra_flags ^ type) & mask)
> is true q->cra_flags = 0x0501 ^ 0x0405 & 0x248x => 0x0004
>
> @Martin, I'll commit a patch asap.
>
> Thanks for your help :-)
>
> regards, H.Freudenberger
>
rollback. Changing the cra_flag CRYPTO_ALG_TYPE_SKCIPHER in the crypto_alg
registration struct is not the right way.
continuing evaluation ...

Re: crypto: hang in crypto_larval_lookup

[Harald Freudenberger]

On 02/23/2017 12:39 PM, Herbert Xu wrote:
> On Thu, Feb 23, 2017 at 07:19:57PM +0800, Herbert Xu wrote:
>> Harald Freudenberger <fre...@linux.vnet.ibm.com> wrote:
>>> Hello all
>>>
>>> I am currently following a hang at modprobe aes_s390 where
>>> crypto_register_alg() does not come back for the xts(aes) algorithm.
>>>
>>> The registration is waiting forever in algapi.c crypto_wait_for_test() but
>>> the completion never occurs. The cryptomgr is triggering a test via
>>> kthread_run to invoce cryptomgr_probe and this thread is calling the
>>> create() function of the xts template (file xts.c). Following this thread
>>> it comes down to api.c crypto_larval_lookup(name="aes") which is first
>>> requesting the module "crypto-aes" via request_module() successful and then
>>> blocking forever in requesting the module "crypto-aes-all".
>>>
>>> The xts(aes) has at registration CRYPTO_ALG_NEED_FALLBACK flag on.
>>>
>>> This problem is seen since about 6 weeks now, first only on the linux next
>>> kernel. Now it appers on the 4.10-rc kernels as well. And I still have no
>>> idea on how this could be fixed or what's wrong with just the xts
>>> registration (ecb, cbc, ctr work fine).
>>>
>>> Any ideas or hints?
>> Sorry, my fault.  I should've converted all the fallback users of
>> the old blkcipher interface over to skcipher before converting the
>> core algorithms to skcipher.
>>
>> I'll send a patch.
> Hmm, actually looks like I did convert this one :)
>
> Do you have ECB enabled in your configuration? XTS doesn't work
> without it.  Currently the Kconfig is missing a select on ECB so
> it could stop the generic XTS from loading.
>
> However, you seem to be stuck on straight AES which quite strange.
> The reason is that s390 crypto registers AES as the first thing so
> it should already be available.
>
> The fact that it hangs is expected because it's trying to find
> an acceptable AES implementation and in doing so it's loading
> s390-aes again.
>
> So we need to get to the bottom of why there is no acceptable
> "aes" registered.  Can you check /proc/crypto to see if the simple
> aes cipher is correctly registered (passing the selftest) after
> it hangs?
>
> You could also print out the type/mask in crypto_larval_lookup
> to see if perhaps the caller is asking for something unreasonable.
>
> Thanks,
Thanks for your help, here is cat /proc/crypto:
name : xts(aes)
driver   :
module   : kernel
priority : -1
refcnt   : 3
selftest : passed
internal : no
type : larval
flags: 0x415

name : xts(aes)
driver   : xts-aes-s390
module   : kernel
priority : 400
refcnt   : 1
selftest : passed
internal : no
type : larval
flags: 0x514

name : xts(aes)
driver   : xts-aes-s390
module   : aes_s390
priority : 400
refcnt   : 4
selftest : unknown
internal : no
type : blkcipher
blocksize: 16
min keysize  : 32
max keysize  : 64
ivsize   : 16
geniv: 

...

name : ecb(aes)
driver   : ecb(aes-s390)
module   : kernel
priority : 300
refcnt   : 1
selftest : passed
internal : no
type : blkcipher
blocksize: 16
min keysize  : 16
max keysize  : 32
ivsize   : 0
geniv: 

name : ecb(aes)
driver   : ecb-aes-s390
module   : aes_s390
priority : 400
refcnt   : 1
selftest : passed
internal : no
type : blkcipher
blocksize: 16
min keysize  : 16
max keysize  : 32
ivsize   : 0
geniv: 

name : aes
driver   : aes-s390
module   : aes_s390
priority : 300
refcnt   : 1
selftest : passed
internal : no
type : cipher
blocksize: 16
min keysize  : 16
max keysize  : 32
...

at the time where the modprobe hangs.

And here is some syslog output (I added some printks for debugging) maybe
it may give you some hints about whats going on during the modprobe:
Feb 23 16:52:08 r35lp49 kernel:   crypto_larval_lookup calling 
crypto_alg_lookup()
Feb 23 16:52:08 r35lp49 kernel: ->__crypto_alg_lookup(name=aes)
Feb 23 16:52:08 r35lp49 kernel:   crypto_alg_get(alg.cra_name=aes) 
alg=00f2dd68 alg.cra_refcnt=2
Feb 23 16:52:08 r35lp49 kernel:   crypto_mod_get(alg.cra_name=aes 
alg=00f2dd68 alg.cra_driver_name=aes-generic alg.cra_module.name=(null))
Feb 23 16:52:08 r35lp49 kernel: <-__crypto_alg_lookup(name=aes) 
alg=00f2dd68
Feb 23 16:52:08 r35lp49 kernel: <-crypto_larval_lookup(name=aes) 
alg=00f2dd68
Feb 23 16:52:08 r35lp49 kernel:   crypto_mod_put(alg.cra_name=aes 
alg=00f2dd68 alg.cra_driv

Re: crypto: hang in crypto_larval_lookup

[Harald Freudenberger]

On 02/26/2017 05:22 AM, Herbert Xu wrote:
> On Sat, Feb 25, 2017 at 04:20:22PM -0300, Marcelo Cerri wrote:
>> Yeah, I agree. This should work as long as the module aliases are
>> correct, which is enough.
>>
>> Other templates will not trigger the same error since they don't have to
>> try more than one underlying algorithm. But I think this is still
>> desirable for the remaining templates to avoid a long chain of unused
>> fallbacks as in the example I gave in my previous email.
>>
>> Probably a helper function to return the correct mask might be useful
>> for readability and to avoid duplicate code.
> You're right.  Here is a patch to add a helper for this.
> Thanks!
>
> ---8<---
> Subject: crypto: api - Add crypto_requires_off helper
>
> This patch adds crypto_requires_off which is an extension of
> crypto_requires_sync for similar bits such as NEED_FALLBACK.
>
> Suggested-by: Marcelo Cerri <marcelo.ce...@canonical.com>
> Signed-off-by: Herbert Xu <herb...@gondor.apana.org.au>
>
> diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
> index ebe4ded..436c4c2 100644
> --- a/include/crypto/algapi.h
> +++ b/include/crypto/algapi.h
> @@ -360,13 +360,18 @@ static inline struct crypto_alg 
> *crypto_get_attr_alg(struct rtattr **tb,
>   return crypto_attr_alg(tb[1], type, mask);
>  }
>
> +static inline int crypto_requires_off(u32 type, u32 mask, u32 off)
> +{
> + return (type ^ off) & mask & off;
> +}
> +
>  /*
>   * Returns CRYPTO_ALG_ASYNC if type/mask requires the use of sync algorithms.
>   * Otherwise returns zero.
>   */
>  static inline int crypto_requires_sync(u32 type, u32 mask)
>  {
> - return (type ^ CRYPTO_ALG_ASYNC) & mask & CRYPTO_ALG_ASYNC;
> + return crypto_requires_off(type, mask, CRYPTO_ALG_ASYNC);
>  }
>
>  noinline unsigned long __crypto_memneq(const void *a, const void *b, size_t 
> size);
applied the xts.c create patch v2 and the helper patch, built and installed.
Now the aes_s390 module loads perfect without any hang, no complains
in syslog and /proc/crypto shows that all selftests for the algs in the module
passed successful.

Thanks all for your help :-)

regards, Harald Freudenberger

Re: Question - seeding the hw pseudo random number generator

[Harald Freudenberger]

On 03/20/2017 02:39 PM, Stephan Müller wrote:
> Am Montag, 20. März 2017, 14:28:58 CET schrieb Herbert Xu:
>
> Hi Herbert,
>
>> On Mon, Mar 20, 2017 at 12:19:32PM +0530, PrasannaKumar Muralidharan wrote:
>>> AF_ALG interface for rng does have seeding support. I think hw_random
>>> does not provide seeding support intentionally as I understand that
>>> True RNG need not require seeding (please correct me if I am wrong).
>> Yes.  We should be converting PRNGs in hwrng over to algif_rng.
> IMHO this not only applies to the PRNGs in drivers/crypto (which should 
> simply 
> register with crypto_register_rngs) but also to ~/hacking/sources/linux/arch/
> s390/crypto/prng.c which exports a /dev/prandom file.
>
> For the seeding, it may make sense to follow the example given with crypto/
> drbg.c using the add_random_ready_callback function.
>
> Ciao
> Stephan
>

I'll have a look on it. Currently the s390/crypto/prng seeds itself with
an algorithm based on the jitter of the very fine granular hardware
clock of a s390 machine. There were some thoughts and measurements
by an mathematician which let to this algorithm. However, long-term
the s390 platform will provide some kind of true hardware random number
generator and the idea is to use this for seeding the prng.

regards
Harald Freudenberger

[PATCH 1/2] crypto: hwrng use rng source with best quality

[Harald Freudenberger]

This patch rewoks the hwrng to always use the
rng source with best entropy quality.

On registation and unregistration the hwrng now
tries to choose the best (= highest quality value)
rng source. The handling of the internal list
of registered rng sources is now always sorted
by quality and the top most rng chosen.

Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 drivers/char/hw_random/core.c | 25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 503a41d..e9dda16 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -29,6 +29,7 @@
 
 static struct hwrng *current_rng;
 static struct task_struct *hwrng_fill;
+/* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
 /* Protects rng_list and current_rng */
 static DEFINE_MUTEX(rng_mutex);
@@ -417,6 +418,7 @@ int hwrng_register(struct hwrng *rng)
 {
int err = -EINVAL;
struct hwrng *old_rng, *tmp;
+   struct list_head *rng_list_ptr;
 
if (!rng->name || (!rng->data_read && !rng->read))
goto out;
@@ -432,14 +434,25 @@ int hwrng_register(struct hwrng *rng)
init_completion(>cleanup_done);
complete(>cleanup_done);
 
+   /* rng_list is sorted by decreasing quality */
+   list_for_each(rng_list_ptr, _list) {
+   tmp = list_entry(rng_list_ptr, struct hwrng, list);
+   if (tmp->quality < rng->quality)
+   break;
+   }
+   list_add_tail(>list, rng_list_ptr);
+
old_rng = current_rng;
err = 0;
-   if (!old_rng) {
+   if (!old_rng || (rng->quality > old_rng->quality)) {
+   /*
+* Set new rng as current as the new rng source
+* provides better entropy quality.
+*/
err = set_current_rng(rng);
if (err)
goto out_unlock;
}
-   list_add_tail(>list, _list);
 
if (old_rng && !rng->init) {
/*
@@ -466,12 +479,12 @@ void hwrng_unregister(struct hwrng *rng)
list_del(>list);
if (current_rng == rng) {
drop_current_rng();
+   /* rng_list is sorted by quality, use the best (=first) one */
if (!list_empty(_list)) {
-   struct hwrng *tail;
-
-   tail = list_entry(rng_list.prev, struct hwrng, list);
+   struct hwrng *new_rng;
 
-   set_current_rng(tail);
+   new_rng = list_entry(rng_list.next, struct hwrng, list);
+   set_current_rng(new_rng);
}
}
 
-- 
2.7.4

[PATCH 2/2] crypto: hwrng remember rng chosen by user

[Harald Freudenberger]

When a user chooses a rng source via sysfs attribute
this rng should be sticky, even when other sources
with better quality to register. This patch introduces
a simple way to remember the user's choice. This is
reflected by a new sysfs attribute file 'rng_selected'
which shows if the current rng has been chosen by
userspace. The new attribute file shows '1' for user
selected rng and '0' otherwise.

Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 drivers/char/hw_random/core.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index e9dda16..9701ac7 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -28,6 +28,8 @@
 #define RNG_MODULE_NAME"hw_random"
 
 static struct hwrng *current_rng;
+/* the current rng has been explicitly chosen by user via sysfs */
+static int cur_rng_set_by_user;
 static struct task_struct *hwrng_fill;
 /* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
@@ -304,6 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
list_for_each_entry(rng, _list, list) {
if (sysfs_streq(rng->name, buf)) {
err = 0;
+   cur_rng_set_by_user = 1;
if (rng != current_rng)
err = set_current_rng(rng);
break;
@@ -352,16 +355,27 @@ static ssize_t hwrng_attr_available_show(struct device 
*dev,
return strlen(buf);
 }
 
+static ssize_t hwrng_attr_selected_show(struct device *dev,
+   struct device_attribute *attr,
+   char *buf)
+{
+   return snprintf(buf, PAGE_SIZE, "%d\n", cur_rng_set_by_user);
+}
+
 static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
   hwrng_attr_current_show,
   hwrng_attr_current_store);
 static DEVICE_ATTR(rng_available, S_IRUGO,
   hwrng_attr_available_show,
   NULL);
+static DEVICE_ATTR(rng_selected, S_IRUGO,
+  hwrng_attr_selected_show,
+  NULL);
 
 static struct attribute *rng_dev_attrs[] = {
_attr_rng_current.attr,
_attr_rng_available.attr,
+   _attr_rng_selected.attr,
NULL
 };
 
@@ -444,10 +458,12 @@ int hwrng_register(struct hwrng *rng)
 
old_rng = current_rng;
err = 0;
-   if (!old_rng || (rng->quality > old_rng->quality)) {
+   if (!old_rng ||
+   (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
/*
 * Set new rng as current as the new rng source
-* provides better entropy quality.
+* provides better entropy quality and was not
+* chosen by userspace.
 */
err = set_current_rng(rng);
if (err)
@@ -479,6 +495,7 @@ void hwrng_unregister(struct hwrng *rng)
list_del(>list);
if (current_rng == rng) {
drop_current_rng();
+   cur_rng_set_by_user = 0;
/* rng_list is sorted by quality, use the best (=first) one */
if (!list_empty(_list)) {
struct hwrng *new_rng;
-- 
2.7.4

Re: [PATCH] crypto: change hwrng device default permissions to 0444

[Harald Freudenberger]

On 07/12/2017 12:13 PM, Herbert Xu wrote:
> On Mon, Jul 03, 2017 at 12:37:59PM +0200, Harald Freudenberger wrote:
>> Currently /dev/hwrng uses default device node permissions
>> which is 0600. So by default the device node is not accessible
>> by an ordinary user. Some distros do rewrite the device node
>> permissions via udev rule, others don't. This patch provides
>> 0444 as the new mode value and so makes the device node
>> accessible for all users without the need to have udev rules
>> rewriting the access rights.
>>
>> Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
> Hmm, one usage scenario for /dev/hwrng is to feed rngd which then
> feeds into /dev/random.  In that case it may not be desirable to
> allow arbitrary access to hwrgn since it may cause the rate of
> entropy going into /dev/random to go down.
>
> In any case, as you noted userspace can change this anyway so I
> don't see why we need to make this policy change in the kernel.
>
> Cheers,
It was worth a try to get rid of complains from customers.
However, your argument about the possible weakness in
the entropy pool for /dev/random with pumping hwrng dry
does not really fit: This can be easier done by just pulling
random directly from /dev/random, as every distro I could
get a hand on uses crw-rw-rw- permissions on /dev/random
and /dev/urandom.

Thanks
Harald Freudenberger

Re: [PATCH v1] crypto: caam - set hwrng quality level

[Harald Freudenberger]

On 07/19/2017 08:13 PM, Oleksij Rempel wrote:
> On Wed, Jul 19, 2017 at 04:53:21PM +, Horia Geantă wrote:
>> On 7/19/2017 7:32 PM, Oleksij Rempel wrote:
>>> On Wed, Jul 19, 2017 at 12:49:47PM +, Horia Geantă wrote:
>>>> On 7/19/2017 10:45 AM, Oleksij Rempel wrote:
>>>>> According documentation, it is NIST certified TRNG.
>>>>> So, set high quality to let the HWRNG framework automatically use it.
>>>>>
>>>>> Signed-off-by: Oleksij Rempel <o.rem...@pengutronix.de>
>>>>> ---
>>>>>  drivers/crypto/caam/caamrng.c | 6 ++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/crypto/caam/caamrng.c b/drivers/crypto/caam/caamrng.c
>>>>> index 41398da3edf4..684c0bc88dfd 100644
>>>>> --- a/drivers/crypto/caam/caamrng.c
>>>>> +++ b/drivers/crypto/caam/caamrng.c
>>>>> @@ -292,10 +292,16 @@ static int caam_init_rng(struct caam_rng_ctx *ctx, 
>>>>> struct device *jrdev)
>>>>>   return 0;
>>>>>  }
>>>>>  
>>>>> +/*
>>>>> + * hwrng register struct
>>>>> + * The trng is suppost to have 100% entropy, and thus
>>>>> + * we register with a very high quality value.
>>>>> + */
>>>>>  static struct hwrng caam_rng = {
>>>>>   .name   = "rng-caam",
>>>>>   .cleanup= caam_cleanup,
>>>>>   .read   = caam_read,
>>>>> + .quality= 999,
>>>> Why not 1024, i.e. where is 999 coming from?
>>> It comes from s390-trng.c driver.
>>> Should I use 1024 instead?
>>>
>> AFAICT the range for quality is from 0 to 1024 (no entropy -> perfect
>> entropy).
>>
>> 1024 should be used since I'd expect a HW TRNG to provide perfect
>> entropy unless otherwise stated.
> I assume 1024 can be given only on verified HW with accessible verilog
> files and compared with resulting chip :)
> May be this would be a good example https://www.sifive.com/
>
Well, the header file says:
...
/**
 * struct hwrng - Hardware Random Number Generator driver
 * @name:   Unique RNG name.
 * @init:   Initialization callback (can be NULL).
 * @cleanup:Cleanup callback (can be NULL).
 * @data_present:   Callback to determine if data is available
 *  on the RNG. If NULL, it is assumed that
 *  there is always data available.  *OBSOLETE*
 * @data_read:  Read data from the RNG device.
 *  Returns the number of lower random bytes in "data".
 *  Must not be NULL.*OBSOLETE*
 * @read:   New API. drivers can fill up to max bytes of data
 *  into the buffer. The buffer is aligned for any type
 *  and max is a multiple of 4 and >= 32 bytes.
 * @priv:   Private data, for use by the RNG driver.
 * @quality:Estimation of true entropy in RNG's bitstream
 *  (per mill).
 */
...
quality = estimation of true entropy per mill.
I understand this as quality=1000 meaning 100% entropy.
However, the core code currently does not really check this value.
When more than one hwrng sources do register, simple the one with
the higher quality value wins :-) The value is not even checked
to be in a given range.

I searched through some device drivers which do register at
the hwrng and it looks like most of the drivers do not even
set this value. My feeling is, you should use 999 when your
hardware provides 'perfect' random. So there is a chance for
an even 'more perfect' hardware coming up later to overrule
your 'perfect' hardware.

regards
Harald Freudenberger

Re: [PATCH 1/2] crypto: Make hwrng choose rng source by quality.

[Harald Freudenberger]

On 06/30/2017 07:27 AM, PrasannaKumar Muralidharan wrote:
> Hi Harald,
>
> Can you split this patch into two? One patch to choose rng based on
> the quality and another for not overriding user decided rng.
>
> Some more minor comments below.
>
> On 29 June 2017 at 15:33, Harald Freudenberger
> <fre...@linux.vnet.ibm.com> wrote:
>> The hwrng core implementation currently doesn't consider the
>> quality field of the struct hwrng. So the first registered rng
>> is the winner and further rng sources even with much better
>> quality are ignored.
>>
>> The behavior should be that always the best rng with the highest
>> quality rate should be used as current rng source. Only if the
>> user explicitly chooses a rng source (via writing a rng name
>> to /sys/class/misc/hw_random) the decision for the best quality
>> should be suppressed.
>>
>> This patch makes hwrng always hold a list of registered rng
>> sources sorted decreasing by quality. On registration of a new
>> hwrng source the list is updated and if the current rng source
>> was not chosen by user and the new rng provides better quality
>> set as new current rng source. Similar on unregistration of an
>> rng, if it was the current used rng source the one with the
>> next highest quality is used. If a rng source has been set via
>> sysfs from userland as long as this one doesn't unregister
>> it is kept as current rng regardless of registration of 'better'
>> rng sources.
> Nice to see the patch. This is indeed required.
>
>> Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
>> ---
>>  drivers/char/hw_random/core.c | 31 +--
>>  1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
>> index 503a41d..7fe47f8 100644
>> --- a/drivers/char/hw_random/core.c
>> +++ b/drivers/char/hw_random/core.c
>> @@ -28,7 +28,10 @@
>>  #define RNG_MODULE_NAME"hw_random"
>>
>>  static struct hwrng *current_rng;
>> +/* the current rng has been explicitly chosen by user via sysfs */
>> +static int cur_rng_set_by_user;
> Letting the user know that the current rng was selected based on user
> input would be a good option I guess. Any thoughts on this?
>
>>  static struct task_struct *hwrng_fill;
>> +/* list of registered rngs, sorted decending by quality */
>>  static LIST_HEAD(rng_list);
>>  /* Protects rng_list and current_rng */
>>  static DEFINE_MUTEX(rng_mutex);
>> @@ -308,6 +311,8 @@ static ssize_t hwrng_attr_current_store(struct device 
>> *dev,
>> break;
>> }
>> }
>> +   if (!err)
>> +   cur_rng_set_by_user = 1;
> This can be put inside the loop. The if condition will go away in that case.
>
>> mutex_unlock(_mutex);
>>
>> return err ? : len;
>> @@ -417,6 +422,7 @@ int hwrng_register(struct hwrng *rng)
>>  {
>> int err = -EINVAL;
>> struct hwrng *old_rng, *tmp;
>> +   struct list_head *ptr;
> Any better name instead of ptr?
>
>> if (!rng->name || (!rng->data_read && !rng->read))
>> goto out;
>> @@ -432,14 +438,26 @@ int hwrng_register(struct hwrng *rng)
>> init_completion(>cleanup_done);
>> complete(>cleanup_done);
>>
>> +   /* rng_list is sorted by decreasing quality */
>> +   list_for_each(ptr, _list) {
>> +   tmp = list_entry(ptr, struct hwrng, list);
>> +   if (tmp->quality < rng->quality)
>> +   break;
>> +   }
>> +   list_add_tail(>list, ptr);
>> +
>> old_rng = current_rng;
>> err = 0;
>> -   if (!old_rng) {
>> +   if (!old_rng ||
>> +   (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
>> +   /*
>> +* Set new rng as current if no current rng or rng was
>> +* not chosen by user and the new one has better quality.
>> +*/
>> err = set_current_rng(rng);
>> if (err)
>> goto out_unlock;
>> }
>> -   list_add_tail(>list, _list);
>>
>> if (old_rng && !rng->init) {
>> /*
>> @@ -466,12 +484,13 @@ void hwrng_unregister(struct hwrng *rng)
>> list_del(>list);
>> if (current_r

[PATCH 1/3] crypto: hwrng use rng source with best quality

[Harald Freudenberger]

This patch rewoks the hwrng to always use the
rng source with best entropy quality.

On registation and unregistration the hwrng now
tries to choose the best (= highest quality value)
rng source. The handling of the internal list
of registered rng sources is now always sorted
by quality and the top most rng chosen.

Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
---
 drivers/char/hw_random/core.c | 25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 503a41d..e9dda16 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -29,6 +29,7 @@
 
 static struct hwrng *current_rng;
 static struct task_struct *hwrng_fill;
+/* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
 /* Protects rng_list and current_rng */
 static DEFINE_MUTEX(rng_mutex);
@@ -417,6 +418,7 @@ int hwrng_register(struct hwrng *rng)
 {
int err = -EINVAL;
struct hwrng *old_rng, *tmp;
+   struct list_head *rng_list_ptr;
 
if (!rng->name || (!rng->data_read && !rng->read))
goto out;
@@ -432,14 +434,25 @@ int hwrng_register(struct hwrng *rng)
init_completion(>cleanup_done);
complete(>cleanup_done);
 
+   /* rng_list is sorted by decreasing quality */
+   list_for_each(rng_list_ptr, _list) {
+   tmp = list_entry(rng_list_ptr, struct hwrng, list);
+   if (tmp->quality < rng->quality)
+   break;
+   }
+   list_add_tail(>list, rng_list_ptr);
+
old_rng = current_rng;
err = 0;
-   if (!old_rng) {
+   if (!old_rng || (rng->quality > old_rng->quality)) {
+   /*
+* Set new rng as current as the new rng source
+* provides better entropy quality.
+*/
err = set_current_rng(rng);
if (err)
goto out_unlock;
}
-   list_add_tail(>list, _list);
 
if (old_rng && !rng->init) {
/*
@@ -466,12 +479,12 @@ void hwrng_unregister(struct hwrng *rng)
list_del(>list);
if (current_rng == rng) {
drop_current_rng();
+   /* rng_list is sorted by quality, use the best (=first) one */
if (!list_empty(_list)) {
-   struct hwrng *tail;
-
-   tail = list_entry(rng_list.prev, struct hwrng, list);
+   struct hwrng *new_rng;
 
-   set_current_rng(tail);
+   new_rng = list_entry(rng_list.next, struct hwrng, list);
+   set_current_rng(new_rng);
}
}
 
-- 
2.7.4

[PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected rng

[Harald Freudenberger]

This patch introduces a new sysfs attribute file 'rng_selected'
which shows the the rng chosen by userspace.

If a rng source is chosen by user via echo some valid string
to rng_current there should be a way to signal this choice to
userspace. The new attribute file 'rng_selected' shows either
the name of the rng chosen or 'none'.

Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
---
 drivers/char/hw_random/core.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index ffd4e36..6a6276a 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -28,8 +28,8 @@
 #define RNG_MODULE_NAME"hw_random"
 
 static struct hwrng *current_rng;
-/* the current rng has been explicitly chosen by user via sysfs */
-static int cur_rng_set_by_user;
+/* the rng explicitly selected by user via sysfs */
+static struct hwrng *selected_rng;
 static struct task_struct *hwrng_fill;
 /* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
@@ -306,7 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
list_for_each_entry(rng, _list, list) {
if (sysfs_streq(rng->name, buf)) {
err = 0;
-   cur_rng_set_by_user = 1;
+   selected_rng = rng;
if (rng != current_rng)
err = set_current_rng(rng);
break;
@@ -355,16 +355,28 @@ static ssize_t hwrng_attr_available_show(struct device 
*dev,
return strlen(buf);
 }
 
+static ssize_t hwrng_attr_selected_show(struct device *dev,
+   struct device_attribute *attr,
+   char *buf)
+{
+   return snprintf(buf, PAGE_SIZE, "%s\n",
+   selected_rng ? selected_rng->name : "none");
+}
+
 static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
   hwrng_attr_current_show,
   hwrng_attr_current_store);
 static DEVICE_ATTR(rng_available, S_IRUGO,
   hwrng_attr_available_show,
   NULL);
+static DEVICE_ATTR(rng_selected, S_IRUGO,
+  hwrng_attr_selected_show,
+  NULL);
 
 static struct attribute *rng_dev_attrs[] = {
_attr_rng_current.attr,
_attr_rng_available.attr,
+   _attr_rng_selected.attr,
NULL
 };
 
@@ -448,7 +460,7 @@ int hwrng_register(struct hwrng *rng)
old_rng = current_rng;
err = 0;
if (!old_rng ||
-   (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
+   (!selected_rng && rng->quality > old_rng->quality)) {
/*
 * Set new rng as current as the new rng source
 * provides better entropy quality and was not
@@ -484,7 +496,7 @@ void hwrng_unregister(struct hwrng *rng)
list_del(>list);
if (current_rng == rng) {
drop_current_rng();
-   cur_rng_set_by_user = 0;
+   selected_rng = NULL;
/* rng_list is sorted by quality, use the best (=first) one */
if (!list_empty(_list)) {
struct hwrng *new_rng;
-- 
2.7.4

[PATCH 2/3] crypto: hwrng remember rng chosen by user

[Harald Freudenberger]

When a user chooses a rng source via sysfs
attribute this rng should be sticky, even
when other sources with better quality to
register. This patch introduces a simple way
to remember the user's choise.

Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
---
 drivers/char/hw_random/core.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index e9dda16..ffd4e36 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -28,6 +28,8 @@
 #define RNG_MODULE_NAME"hw_random"
 
 static struct hwrng *current_rng;
+/* the current rng has been explicitly chosen by user via sysfs */
+static int cur_rng_set_by_user;
 static struct task_struct *hwrng_fill;
 /* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
@@ -304,6 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
list_for_each_entry(rng, _list, list) {
if (sysfs_streq(rng->name, buf)) {
err = 0;
+   cur_rng_set_by_user = 1;
if (rng != current_rng)
err = set_current_rng(rng);
break;
@@ -444,10 +447,12 @@ int hwrng_register(struct hwrng *rng)
 
old_rng = current_rng;
err = 0;
-   if (!old_rng || (rng->quality > old_rng->quality)) {
+   if (!old_rng ||
+   (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
/*
 * Set new rng as current as the new rng source
-* provides better entropy quality.
+* provides better entropy quality and was not
+* chosen by userspace.
 */
err = set_current_rng(rng);
if (err)
@@ -479,6 +484,7 @@ void hwrng_unregister(struct hwrng *rng)
list_del(>list);
if (current_rng == rng) {
drop_current_rng();
+   cur_rng_set_by_user = 0;
/* rng_list is sorted by quality, use the best (=first) one */
if (!list_empty(_list)) {
struct hwrng *new_rng;
-- 
2.7.4

[PATCH 0/3] crypto hwrng consider quality value, remember user choice

[Harald Freudenberger]

The hwrng core implementation currently doesn't consider the
quality field of the struct hwrng. So the first registered rng
is the winner and further rng sources even with much better
quality are ignored.

The behavior should be that always the best rng with the highest
quality rate should be used as current rng source. Only if the
user explicitly chooses a rng source (via writing a rng name
to /sys/class/misc/hw_random/rng_current) the decision for the
best quality should be suppressed.

This set of patches makes hwrng always hold a list of registered
rng sources sorted decreasing by quality. On registration of a new
hwrng source the list is updated and if the current rng source was
not chosen by user and the new rng provides better quality set as
new current rng source. Similar on unregistration of an rng, if it
was the current used rng source the one with the next highest quality
is used. If a rng source has been set via sysfs from userland as
long as this one doesn't unregister it is kept as current rng
regardless of registration of 'better' rng sources.

Patch 1 introduces the sorted list of registered rngs and the
always use the best quality behavior.

Patch 2 makes hwrng remember that the user has selected an
rng via echo to /sys/class/misc/hw_random/rng_current.

Patch 3 adds a new sysfs attribute file 'rng_selected' to the
rng core. This file shows the chosen rng name if a selection
from userspace took place otherwise 'none'.

Patch 3 is just a simple implementation of an possible improvement
and may act as a starting point for further discussions. For example,
the implementation could be reworked to accept also currently not
known rng sources and upon appearing instantly select this user
chosen rng. However, this would require to hold an string buffer
and this would introduce some string length limit on the rng name.
Another idea is that there should be a possibility to unselect
the user's choice. An echo 'none' to rng_current may be a way to
remove the selection and the hwrng may act by using the quality best
rng.

Harald Freudenberger (3):
  crypto: hwrng use rng source with best quality
  crypto: hwrng remember rng chosen by user
  crypto: hwrng add sysfs attribute to show user selected rng

 drivers/char/hw_random/core.c | 43 +--
 1 file changed, 37 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH] crypto: change hwrng device default permissions to 0444

[Harald Freudenberger]

Currently /dev/hwrng uses default device node permissions
which is 0600. So by default the device node is not accessible
by an ordinary user. Some distros do rewrite the device node
permissions via udev rule, others don't. This patch provides
0444 as the new mode value and so makes the device node
accessible for all users without the need to have udev rules
rewriting the access rights.

Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
---
 drivers/char/hw_random/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 503a41d..4093db5 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -285,6 +285,7 @@ static struct miscdevice rng_miscdev = {
.minor  = HWRNG_MINOR,
.name   = RNG_MODULE_NAME,
.nodename   = "hwrng",
+   .mode   = 0444,
.fops   = _chrdev_ops,
.groups = rng_dev_groups,
 };
-- 
2.7.4

Re: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected rng

[Harald Freudenberger]

On 07/04/2017 03:15 PM, PrasannaKumar Muralidharan wrote:
> On 3 July 2017 at 15:33, Harald Freudenberger <fre...@linux.vnet.ibm.com> 
> wrote:
>> This patch introduces a new sysfs attribute file 'rng_selected'
>> which shows the the rng chosen by userspace.
>>
>> If a rng source is chosen by user via echo some valid string
>> to rng_current there should be a way to signal this choice to
>> userspace. The new attribute file 'rng_selected' shows either
>> the name of the rng chosen or 'none'.
>>
>> Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
>> ---
>>  drivers/char/hw_random/core.c | 22 +-
>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
>> index ffd4e36..6a6276a 100644
>> --- a/drivers/char/hw_random/core.c
>> +++ b/drivers/char/hw_random/core.c
>> @@ -28,8 +28,8 @@
>>  #define RNG_MODULE_NAME"hw_random"
>>
>>  static struct hwrng *current_rng;
>> -/* the current rng has been explicitly chosen by user via sysfs */
>> -static int cur_rng_set_by_user;
>> +/* the rng explicitly selected by user via sysfs */
>> +static struct hwrng *selected_rng;
>>  static struct task_struct *hwrng_fill;
>>  /* list of registered rngs, sorted decending by quality */
>>  static LIST_HEAD(rng_list);
>> @@ -306,7 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device 
>> *dev,
>> list_for_each_entry(rng, _list, list) {
>> if (sysfs_streq(rng->name, buf)) {
>> err = 0;
>> -   cur_rng_set_by_user = 1;
>> +   selected_rng = rng;
>> if (rng != current_rng)
>> err = set_current_rng(rng);
>> break;
>> @@ -355,16 +355,28 @@ static ssize_t hwrng_attr_available_show(struct device 
>> *dev,
>> return strlen(buf);
>>  }
>>
>> +static ssize_t hwrng_attr_selected_show(struct device *dev,
>> +   struct device_attribute *attr,
>> +   char *buf)
>> +{
>> +   return snprintf(buf, PAGE_SIZE, "%s\n",
>> +   selected_rng ? selected_rng->name : "none");
>> +}
>> +
>>  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
>>hwrng_attr_current_show,
>>hwrng_attr_current_store);
>>  static DEVICE_ATTR(rng_available, S_IRUGO,
>>hwrng_attr_available_show,
>>NULL);
>> +static DEVICE_ATTR(rng_selected, S_IRUGO,
>> +  hwrng_attr_selected_show,
>> +  NULL);
>>
>>  static struct attribute *rng_dev_attrs[] = {
>> _attr_rng_current.attr,
>> _attr_rng_available.attr,
>> +   _attr_rng_selected.attr,
>> NULL
>>  };
>>
>> @@ -448,7 +460,7 @@ int hwrng_register(struct hwrng *rng)
>> old_rng = current_rng;
>> err = 0;
>> if (!old_rng ||
>> -   (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
>> +   (!selected_rng && rng->quality > old_rng->quality)) {
>> /*
>>  * Set new rng as current as the new rng source
>>  * provides better entropy quality and was not
>> @@ -484,7 +496,7 @@ void hwrng_unregister(struct hwrng *rng)
>> list_del(>list);
>> if (current_rng == rng) {
>> drop_current_rng();
>> -   cur_rng_set_by_user = 0;
>> +   selected_rng = NULL;
>> /* rng_list is sorted by quality, use the best (=first) one 
>> */
>> if (!list_empty(_list)) {
>> struct hwrng *new_rng;
>> --
>> 2.7.4
>>
> The current_rng sysfs attribute shows currently selected rng. So this
> new attribute can contain 1 to indicate user's choice, 0 otherwise.
>
> Regards,
> PrasannaKumar
>
Here is an updated version with just showing 0 or 1 in the new sysfs
attribute file:
== cut ==
From: Harald Freudenberger <fre...@linux.vnet.ibm.com>
Date: Mon, 3 Jul 2017 10:19:22 +0200
Subject: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected
 rng

This patch introduces a new sysfs attribute file 'rng_selected'
which shows if the current rng has been chosen by userspace.

[PATCH 1/2] crypto: Make hwrng choose rng source by quality.

[Harald Freudenberger]

The hwrng core implementation currently doesn't consider the
quality field of the struct hwrng. So the first registered rng
is the winner and further rng sources even with much better
quality are ignored.

The behavior should be that always the best rng with the highest
quality rate should be used as current rng source. Only if the
user explicitly chooses a rng source (via writing a rng name
to /sys/class/misc/hw_random) the decision for the best quality
should be suppressed.

This patch makes hwrng always hold a list of registered rng
sources sorted decreasing by quality. On registration of a new
hwrng source the list is updated and if the current rng source
was not chosen by user and the new rng provides better quality
set as new current rng source. Similar on unregistration of an
rng, if it was the current used rng source the one with the
next highest quality is used. If a rng source has been set via
sysfs from userland as long as this one doesn't unregister
it is kept as current rng regardless of registration of 'better'
rng sources.

Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
---
 drivers/char/hw_random/core.c | 31 +--
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 503a41d..7fe47f8 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -28,7 +28,10 @@
 #define RNG_MODULE_NAME"hw_random"
 
 static struct hwrng *current_rng;
+/* the current rng has been explicitly chosen by user via sysfs */
+static int cur_rng_set_by_user;
 static struct task_struct *hwrng_fill;
+/* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
 /* Protects rng_list and current_rng */
 static DEFINE_MUTEX(rng_mutex);
@@ -308,6 +311,8 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
break;
}
}
+   if (!err)
+   cur_rng_set_by_user = 1;
mutex_unlock(_mutex);
 
return err ? : len;
@@ -417,6 +422,7 @@ int hwrng_register(struct hwrng *rng)
 {
int err = -EINVAL;
struct hwrng *old_rng, *tmp;
+   struct list_head *ptr;
 
if (!rng->name || (!rng->data_read && !rng->read))
goto out;
@@ -432,14 +438,26 @@ int hwrng_register(struct hwrng *rng)
init_completion(>cleanup_done);
complete(>cleanup_done);
 
+   /* rng_list is sorted by decreasing quality */
+   list_for_each(ptr, _list) {
+   tmp = list_entry(ptr, struct hwrng, list);
+   if (tmp->quality < rng->quality)
+   break;
+   }
+   list_add_tail(>list, ptr);
+
old_rng = current_rng;
err = 0;
-   if (!old_rng) {
+   if (!old_rng ||
+   (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
+   /*
+* Set new rng as current if no current rng or rng was
+* not chosen by user and the new one has better quality.
+*/
err = set_current_rng(rng);
if (err)
goto out_unlock;
}
-   list_add_tail(>list, _list);
 
if (old_rng && !rng->init) {
/*
@@ -466,12 +484,13 @@ void hwrng_unregister(struct hwrng *rng)
list_del(>list);
if (current_rng == rng) {
drop_current_rng();
+   cur_rng_set_by_user = 0;
+   /* rng_list is sorted by quality, use the best (=first) one */
if (!list_empty(_list)) {
-   struct hwrng *tail;
-
-   tail = list_entry(rng_list.prev, struct hwrng, list);
+   struct hwrng *new_rng;
 
-   set_current_rng(tail);
+   new_rng = list_entry(rng_list.next, struct hwrng, list);
+   set_current_rng(new_rng);
}
}
 
-- 
2.7.4

[PATCH] hwrng: Make hwrng choose rng source by quality.

[Harald Freudenberger]

The hwrng core implementation currently doesn't consider the
quality field of the struct hwrng. So the first registered rng
is the winner and further rng sources even with much better
quality are ignored.

The behavior should be that always the best rng with the highest
quality rate should be used as current rng source. Only if the
user explicitly chooses a rng source (via writing a rng name
to /sys/class/misc/hw_random) the decision for the best quality
should be suppressed.

This patch makes hwrng always hold a list of registered rng
sources sorted decreasing by quality. On registration of a new
hwrng source the list is updated and if the current rng source
was not chosen by user and the new rng provides better quality
set as new current rng source. Similar on unregistration of an
rng, if it was the current used rng source the one with the
next highest quality is used. If a rng source has been set via
sysfs from userland as long as this one doesn't unregister
it is kept as current rng regardless of registration of 'better'
rng sources.

Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
---
 drivers/char/hw_random/core.c | 31 +--
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 503a41d..7fe47f8 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -28,7 +28,10 @@
 #define RNG_MODULE_NAME"hw_random"
 
 static struct hwrng *current_rng;
+/* the current rng has been explicitly chosen by user via sysfs */
+static int cur_rng_set_by_user;
 static struct task_struct *hwrng_fill;
+/* list of registered rngs, sorted decending by quality */
 static LIST_HEAD(rng_list);
 /* Protects rng_list and current_rng */
 static DEFINE_MUTEX(rng_mutex);
@@ -308,6 +311,8 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
break;
}
}
+   if (!err)
+   cur_rng_set_by_user = 1;
mutex_unlock(_mutex);
 
return err ? : len;
@@ -417,6 +422,7 @@ int hwrng_register(struct hwrng *rng)
 {
int err = -EINVAL;
struct hwrng *old_rng, *tmp;
+   struct list_head *ptr;
 
if (!rng->name || (!rng->data_read && !rng->read))
goto out;
@@ -432,14 +438,26 @@ int hwrng_register(struct hwrng *rng)
init_completion(>cleanup_done);
complete(>cleanup_done);
 
+   /* rng_list is sorted by decreasing quality */
+   list_for_each(ptr, _list) {
+   tmp = list_entry(ptr, struct hwrng, list);
+   if (tmp->quality < rng->quality)
+   break;
+   }
+   list_add_tail(>list, ptr);
+
old_rng = current_rng;
err = 0;
-   if (!old_rng) {
+   if (!old_rng ||
+   (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
+   /*
+* Set new rng as current if no current rng or rng was
+* not chosen by user and the new one has better quality.
+*/
err = set_current_rng(rng);
if (err)
goto out_unlock;
}
-   list_add_tail(>list, _list);
 
if (old_rng && !rng->init) {
/*
@@ -466,12 +484,13 @@ void hwrng_unregister(struct hwrng *rng)
list_del(>list);
if (current_rng == rng) {
drop_current_rng();
+   cur_rng_set_by_user = 0;
+   /* rng_list is sorted by quality, use the best (=first) one */
if (!list_empty(_list)) {
-   struct hwrng *tail;
-
-   tail = list_entry(rng_list.prev, struct hwrng, list);
+   struct hwrng *new_rng;
 
-   set_current_rng(tail);
+   new_rng = list_entry(rng_list.next, struct hwrng, list);
+   set_current_rng(new_rng);
}
}
 
-- 
2.7.4

Re: [PATCH] hw_random: core: Reset user selected rng by writing "" to rng_current

[Harald Freudenberger]

On 10/27/2017 07:04 PM, PrasannaKumar Muralidharan wrote:
> User is able to select a chosen rng by writing its name to rng_current
> but there is no way to reset it without unbinding the rng. Let user
> write "" to rng_current and delesect the chosen rng.
>
> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
> ---
>  drivers/char/hw_random/core.c | 51 
> +++
>  1 file changed, 32 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 9701ac7..be03024 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -292,26 +292,48 @@ static struct miscdevice rng_miscdev = {
>   .groups = rng_dev_groups,
>  };
>
> +static int enable_best_rng(void)
> +{
> + int ret = -ENODEV;
> +
> + BUG_ON(!mutex_is_locked(_mutex));
> +
> + /* rng_list is sorted by quality, use the best (=first) one */
> + if (!list_empty(_list)) {
> + struct hwrng *new_rng;
> +
> + new_rng = list_entry(rng_list.next, struct hwrng, list);
> + ret = ((new_rng == current_rng) ? 0 : set_current_rng(new_rng));
> + if (!ret)
> + cur_rng_set_by_user = 0;
> + }
> +
> + return ret;
> +}
> +
>  static ssize_t hwrng_attr_current_store(struct device *dev,
>   struct device_attribute *attr,
>   const char *buf, size_t len)
>  {
> - int err;
> + int err = -ENODEV;
Two lines later err is assigned the return value of mutex_lock_interruptible().
So the -ENODEV value assignment does nothing.
>   struct hwrng *rng;
>
>   err = mutex_lock_interruptible(_mutex);
>   if (err)
>   return -ERESTARTSYS;
> - err = -ENODEV;
> - list_for_each_entry(rng, _list, list) {
> - if (sysfs_streq(rng->name, buf)) {
> - err = 0;
> - cur_rng_set_by_user = 1;
> - if (rng != current_rng)
> +
> + if (sysfs_streq(buf, "")) {
> + err = enable_best_rng();
> + } else {
> + list_for_each_entry(rng, _list, list) {
> + if (sysfs_streq(rng->name, buf)) {
> + cur_rng_set_by_user = 1;
>   err = set_current_rng(rng);
> - break;
> + break;
> + }
>   }
>   }
> +
>   mutex_unlock(_mutex);
>
>   return err ? : len;
> @@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng)
>   mutex_lock(_mutex);
>
>   list_del(>list);
> - if (current_rng == rng) {
> - drop_current_rng();
> - cur_rng_set_by_user = 0;
> - /* rng_list is sorted by quality, use the best (=first) one */
> - if (!list_empty(_list)) {
> - struct hwrng *new_rng;
> -
> - new_rng = list_entry(rng_list.next, struct hwrng, list);
> - set_current_rng(new_rng);
> - }
> - }
> + if (current_rng == rng)
> + enable_best_rng();
>
>   if (list_empty(_list)) {
>   mutex_unlock(_mutex);
looks like for me.
reviewed-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>

Re: [PATCH] hw_random: core: Reset user selected rng by writing "" to rng_current

[Harald Freudenberger]

On 10/27/2017 07:04 PM, PrasannaKumar Muralidharan wrote:
> User is able to select a chosen rng by writing its name to rng_current
> but there is no way to reset it without unbinding the rng. Let user
> write "" to rng_current and delesect the chosen rng.
>
> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
> ---
>  drivers/char/hw_random/core.c | 51 
> +++
>  1 file changed, 32 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 9701ac7..be03024 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -292,26 +292,48 @@ static struct miscdevice rng_miscdev = {
>   .groups = rng_dev_groups,
>  };
>
> +static int enable_best_rng(void)
> +{
> + int ret = -ENODEV;
> +
> + BUG_ON(!mutex_is_locked(_mutex));
> +
> + /* rng_list is sorted by quality, use the best (=first) one */
> + if (!list_empty(_list)) {
> + struct hwrng *new_rng;
> +
> + new_rng = list_entry(rng_list.next, struct hwrng, list);
> + ret = ((new_rng == current_rng) ? 0 : set_current_rng(new_rng));
> + if (!ret)
> + cur_rng_set_by_user = 0;
> + }
> +
> + return ret;
> +}
> +
>  static ssize_t hwrng_attr_current_store(struct device *dev,
>   struct device_attribute *attr,
>   const char *buf, size_t len)
>  {
> - int err;
> + int err = -ENODEV;
>   struct hwrng *rng;
>
>   err = mutex_lock_interruptible(_mutex);
>   if (err)
>   return -ERESTARTSYS;
> - err = -ENODEV;
> - list_for_each_entry(rng, _list, list) {
> - if (sysfs_streq(rng->name, buf)) {
> - err = 0;
> - cur_rng_set_by_user = 1;
> - if (rng != current_rng)
> +
> + if (sysfs_streq(buf, "")) {
> + err = enable_best_rng();
> + } else {
> + list_for_each_entry(rng, _list, list) {
> + if (sysfs_streq(rng->name, buf)) {
> + cur_rng_set_by_user = 1;
>   err = set_current_rng(rng);
> - break;
> + break;
> + }
>   }
>   }
> +
>   mutex_unlock(_mutex);
>
>   return err ? : len;
> @@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng)
>   mutex_lock(_mutex);
>
>   list_del(>list);
> - if (current_rng == rng) {
> - drop_current_rng();
> - cur_rng_set_by_user = 0;
> - /* rng_list is sorted by quality, use the best (=first) one */
> - if (!list_empty(_list)) {
> - struct hwrng *new_rng;
> -
> - new_rng = list_entry(rng_list.next, struct hwrng, list);
> -     set_current_rng(new_rng);
> - }
> - }
> + if (current_rng == rng)
> + enable_best_rng();
>
>   if (list_empty(_list)) {
>   mutex_unlock(_mutex);
That's a really good idea. I also thought about something like that.
regards
Harald Freudenberger

Q for a new API for the random device driver

[Harald Freudenberger]

Hello Theodore, hi Linux Community

my patch for the s390 arch_get_random_seed* implementation is about to
be integrated with the current merge window for kernel 4.18.

So I'd like to start a discussion about a new API for the random.c
device driver. The current s390 hardware comes with a true hardware
random generator. This great random source provides high quality
entropy (100% according to our hardware guys) but is relatively slow.

I want to provide this entropy source to the random pool of the
random device driver. The only possibility is the implementation of
the arch_get_random* and arch_get_random_seed* API. So my first attempt
was to implement the arch_get_random* functions and directly call the
TRNG. This code made it into the 4.12 kernel and slowed down the
/dev/urandom performance remarkable. So the first rework removed the
s390 arch_get_random* implementation and instead provided the
arch_*_seed functions. The idea was that your good but slow TRNG is
ideal for providing entropy as seed for pseudo random
generators. However, as arch_get_random_seed_long() is called very
frequently in interrupt context and this limits the amount of irqs
per cpu remarkable (e.g. on heavy network load). As a result my latest
fix introduces some kind of buffer concept in combination with filling
the buffer asynchronous with a cascaded TRNG/PRNG.

I am still searching for a way to provide our good hardware entropy
source to the random pool in the random device driver. So I'd like to
have a new arch interface which is called when the random pool finds
out that it is running out of entropy. My feeling is that it should
not be the only source but should still be mixed with other sources
of entropy. It should not be able to dominate or occupy the random
pool but contribute to a significant part.

As nowadays true random generators provide conditioned data usually
with some kind of hashing algorithm a granularity of 4 or 8 bytes is
waste of random entropy. The s390 TRNG uses SHA512 and can provide
64 bytes entropy with each invocation. Other TRNGs may use sha1 or
sha256 and so provide 20 or 32 bytes of random. However, the API
could be something like:

  int arch_get_entropy(void *buf, int bufsize);

and the function returns:

  < 0 - error, negative errno value
  0...bufsize - amount of entropy bytes written into the buffer
        may be 0 if there is (currently) no random entropy
        available. No need to fill the buffer, any data in the
        range of 0 up to bufsize is welcome.

random.c could call this api with a buffer of 64 bytes when the
computed entropy within the pool drops below a threshold limit.

The call should not be done within a userspace process context or
'foreign' kernel context (like irqs) but with an own kernel thread
or workqueue or similar. The data returned should be mixed into
the pool and counted as entropy.

I think, other architectures could also benefit from such a new
interface. Power and even x86 have or will have true random entropy
source hardware and may also like to contribute to the linux random
device driver.

Does this sound reasonable? If yes, I'll start some investigation and
try to develop something for random.c. Though this may take some time.

regards and thanks for your time
Harald Freudenberger

Re: Q for a new API for the random device driver

[Harald Freudenberger]

On 06.06.2018 16:26, PrasannaKumar Muralidharan wrote:
> Hi Herald,
>
> On 6 June 2018 at 18:18, Harald Freudenberger  wrote:
>> Hello Theodore, hi Linux Community
>>
>> my patch for the s390 arch_get_random_seed* implementation is about to
>> be integrated with the current merge window for kernel 4.18.
>>
>> So I'd like to start a discussion about a new API for the random.c
>> device driver. The current s390 hardware comes with a true hardware
>> random generator. This great random source provides high quality
>> entropy (100% according to our hardware guys) but is relatively slow.
>>
>> I want to provide this entropy source to the random pool of the
>> random device driver. The only possibility is the implementation of
>> the arch_get_random* and arch_get_random_seed* API. So my first attempt
>> was to implement the arch_get_random* functions and directly call the
>> TRNG. This code made it into the 4.12 kernel and slowed down the
>> /dev/urandom performance remarkable. So the first rework removed the
>> s390 arch_get_random* implementation and instead provided the
>> arch_*_seed functions. The idea was that your good but slow TRNG is
>> ideal for providing entropy as seed for pseudo random
>> generators. However, as arch_get_random_seed_long() is called very
>> frequently in interrupt context and this limits the amount of irqs
>> per cpu remarkable (e.g. on heavy network load). As a result my latest
>> fix introduces some kind of buffer concept in combination with filling
>> the buffer asynchronous with a cascaded TRNG/PRNG.
>>
>> I am still searching for a way to provide our good hardware entropy
>> source to the random pool in the random device driver. So I'd like to
>> have a new arch interface which is called when the random pool finds
>> out that it is running out of entropy. My feeling is that it should
>> not be the only source but should still be mixed with other sources
>> of entropy. It should not be able to dominate or occupy the random
>> pool but contribute to a significant part.
>>
>> As nowadays true random generators provide conditioned data usually
>> with some kind of hashing algorithm a granularity of 4 or 8 bytes is
>> waste of random entropy. The s390 TRNG uses SHA512 and can provide
>> 64 bytes entropy with each invocation. Other TRNGs may use sha1 or
>> sha256 and so provide 20 or 32 bytes of random. However, the API
>> could be something like:
>>
>>   int arch_get_entropy(void *buf, int bufsize);
>>
>> and the function returns:
>>
>>   < 0 - error, negative errno value
>>   0...bufsize - amount of entropy bytes written into the buffer
>> may be 0 if there is (currently) no random entropy
>> available. No need to fill the buffer, any data in the
>> range of 0 up to bufsize is welcome.
>>
>> random.c could call this api with a buffer of 64 bytes when the
>> computed entropy within the pool drops below a threshold limit.
>>
>> The call should not be done within a userspace process context or
>> 'foreign' kernel context (like irqs) but with an own kernel thread
>> or workqueue or similar. The data returned should be mixed into
>> the pool and counted as entropy.
>>
>> I think, other architectures could also benefit from such a new
>> interface. Power and even x86 have or will have true random entropy
>> source hardware and may also like to contribute to the linux random
>> device driver.
>>
>> Does this sound reasonable? If yes, I'll start some investigation and
>> try to develop something for random.c. Though this may take some time.
>>
>> regards and thanks for your time
>> Harald Freudenberger
>>
> hw_rng framework provides entropy to the kernel's entropy pool
> already. I am wondering why hw_random interface can't be used for
> this.
>
> Regards,
> PrasannaKumar
You are right. I was not aware of this API. Thanks
I'll give it a try.

regards
H.Freudenberger

Re: Q for a new API for the random device driver

[Harald Freudenberger]

On 06.06.2018 15:11, Stephan Mueller wrote:
> Am Mittwoch, 6. Juni 2018, 14:48:33 CEST schrieb Harald Freudenberger:
>
> Hi Harald,
>> I am still searching for a way to provide our good hardware entropy
>> source to the random pool in the random device driver. So I'd like to
>> have a new arch interface which is called when the random pool finds
>> out that it is running out of entropy. My feeling is that it should
>> not be the only source but should still be mixed with other sources
>> of entropy. It should not be able to dominate or occupy the random
>> pool but contribute to a significant part.
>>
>> As nowadays true random generators provide conditioned data usually
>> with some kind of hashing algorithm a granularity of 4 or 8 bytes is
>> waste of random entropy. The s390 TRNG uses SHA512 and can provide
>> 64 bytes entropy with each invocation. Other TRNGs may use sha1 or
>> sha256 and so provide 20 or 32 bytes of random. However, the API
>> could be something like:
>>
>>   int arch_get_entropy(void *buf, int bufsize);
>
> Why not using the add_hwgenerator_randomness with a kernel thread that is 
> controlled/spawned from the noise source?
>
> Ciao
> Stephan
Had a short glimpse to the mentioned add_hwgenerator_randomness()
and this looks in fact like the API I am looking for :-)
Thanks Stephan, I'll write some code and check this out.

regards H.Freudenberger
>

Re: [PATCH 2/2] crypto: ccree: enable support for hardware keys

[Harald Freudenberger]

On 04/03/2018 12:19 PM, Herbert Xu wrote:
> On Sat, Mar 31, 2018 at 08:30:46PM +0300, Gilad Ben-Yossef wrote:
>> However, as it uses the exact same mechanism of the regular xts-aes-ccree
>> but takes the key from another source, I've marked it with a test of
>> alg_test_null() on the premise that if the xts-aes-ccree works, so must this.
> Well it appears to be stubbed out as cc_is_hw_key always returns
> false.
>
>>> Are there other crypto drivers doing this?
>> I thought the exact same thing until I ran into a presentation about the s390
>> secure keys implementation. I basically imitated their use (or abuse?)
>> of the Crypto API
>> assuming it is the way to go.
>>
>> Take a look at arch/s390/crypto/paes_s390.c
> It's always nice to discover code that was never reviewed.
>
> The general approach seems sane though.
>
>> Anyway, if this is not the way to go I'd be more than happy to do whatever
>> work is needed to create the right interface.
>>
>> PS. I'd be out of the office and away from email access to the coming week, 
>> so
>> kindly excuse any delay in response.
> I think it's fine.  However, I don't really like the idea of everyone
> coming up with their own "paes", i.e., your patch's use of "haes".
> It should be OK to just use paes for everyone, no?
>
> As to your patch specifically, there is one issue where you're
> directly dereferencing the key as a struct.  This is a no-no because
> the key may have come from user-space.  You must treat it as a
> binary blob.  The s390 code seems to do this correctly.
>
> Cheers,
I would be happy to have a generic kernel interface for some kind
of key token as a binary blob. I am also open to use the kernel key
ring for future extensions. But please understand we needed a way
to support our hardware keys and I think the chosen design isn't
so bad.

regards Harald Freudenberger
using the kernel key ring in future extensions.

Question on random.c add_interrupt_randomness function

[Harald Freudenberger]

Hello Theodore,
I am currently investigating a better implementation of
the arch_get_random_long_seed() implementation for s390.
And so I stumbled over the function add_interrupt_randomness()
in random.c and have one question regarding this code:

void add_interrupt_randomness(int irq, int irq_flags)
{
    ...
    fast_mix(fast_pool);
    ...
    if ((fast_pool->count < 64) &&
        !time_after(now, fast_pool->last + HZ))
        return;
    ...
}

The condition is true and terminates the function
when the count value of the cpu fast pool is below 64
AND the time since last mix of the pool is lower than
HZ (so lower than 1s).
This means the code following this condition is run
when the count value is > 64 or the last mix is more
than 1s old.
As the fast_mix() function does a fast_pool->count++
effectively every 64 invocations this condition is
false and the rest of the function is executed.

Is this the intention? Shouldn't the condition
terminate the function either when there are fewer
than 64 mixes in the pool OR time since last
invocation is < 1 s ?

regards
Harald Freudenberger