Re: Computing GHASH for GCM when IV > 12 Bytes

[Stephan Müller]

Am Donnerstag, 16. August 2018, 09:14:59 CEST schrieb Jitendra Lulla:

Hi Jitendra,

> Hi Stephen,
> 
> I could not spot in the kernel where we are computing GHASH when the
> IV is bigger than 12 Bytes for GCM encryption.
> 
> libkcapi and kernel appears to ignore the bytes beyond 12th byte in the IV.
> SO the o/p is same with iv=12 bytes or iv=128 bytes as can be seen below:
> 
> jlulla@ubuntu:~/libkcapi-1.0.3/bin$ ./kcapi -x 2 -e -c "gcm(aes)" -k
> 88768354df414ce4097f4d357837116685beee0d93aab343576b893088256260 -i
> f649d375e4e896397a8a96cbb847fbf45cb54132c76baf814f4e35e9f7737f16d5cd710370f1
> 43612b46724bbdded2a26264b90a91f5ed425d08d317f49a56828fcfeb9ebe1bc53117bb4156
> c2e99d70b238dd9166cc05906719818022c75957d25ad9c36c93ce2626248c783e0207c35db7
> 4996f47d096c3cafe701a38154ce -a "" -p "" -l 16
> 
> output (with 128 Byte IV): cb35642763e3a112857acc7aeab15720
> 
> jlulla@ubuntu:~/libkcapi-1.0.3/bin$ ./kcapi -x 2 -e -c "gcm(aes)" -k
> 88768354df414ce4097f4d357837116685beee0d93aab343576b893088256260 -i
> f649d375e4e896397a8a96cb -a "" -p "" -l 16
> 
> output (with 12 byte IV): cb35642763e3a112857acc7aeab15720
> 
> 
> The standard says something different as can be seen here Algorithm
> 4's step 2 [page 15]
> 
> https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pd
> f
> 
> 
> Freebsd's aes_gcm_prepare_j0() seems to be doing what is expected
> https://github.com/lattera/freebsd/blob/master/contrib/wpa/src/crypto/aes-gc
> m.c
> 
> Does linux have any corresponding function?

GCM implemented in the kernel only supports IVs with 12 bytes as defined by 
GCM_AES_IV_SIZE. There is no GHASH operation of the IV if it is larger. Most 
implementations simply copy only 12 bytes from the IV irrespective whether the 
IV is larger.

Libkcapi will only consume the IV size specified by the kernel:

uint32_t iv_msg_size = handle->cipher.iv ?
  CMSG_SPACE(sizeof(*alg_iv) + tfm->info.ivsize) :
  0;

Ciao
Stephan

random: ensure use of aligned buffers with ChaCha20

[Stephan Müller]

The function extract_crng invokes the ChaCha20 block operation directly
on the user-provided buffer. The block operation operates on u32 words.
Thus the extract_crng function expects the buffer to be aligned to u32
as it is visible with the parameter type of extract_crng. However,
get_random_bytes uses a void pointer which may or may not be aligned.
Thus, an alignment check is necessary and the temporary buffer must be
used if the alignment to u32 is not ensured.

Cc:  # v4.16+
Cc: Ted Tso 
Signed-off-by: Stephan Mueller 
---
 drivers/char/random.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index bd449ad52442..23f336872426 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1617,8 +1617,14 @@ static void _get_random_bytes(void *buf, int nbytes)
trace_get_random_bytes(nbytes, _RET_IP_);
 
while (nbytes >= CHACHA20_BLOCK_SIZE) {
-   extract_crng(buf);
-   buf += CHACHA20_BLOCK_SIZE;
+   if (likely((unsigned long)buf & (sizeof(tmp[0]) - 1))) {
+   extract_crng(buf);
+   buf += CHACHA20_BLOCK_SIZE;
+   } else {
+   extract_crng(tmp);
+   memcpy(buf, tmp, CHACHA20_BLOCK_SIZE);
+   }
+
nbytes -= CHACHA20_BLOCK_SIZE;
}
 
-- 
2.17.1

Re: [PATCH 2/3] crypto: hisilicon SEC security accelerator driver

[Stephan Müller]

Am Montag, 16. Juli 2018, 12:43:41 CEST schrieb Jonathan Cameron:

Hi Jonathan,

> +static int sec_alg_skcipher_setkey_aes_xts(struct crypto_skcipher *tfm,
> +  const u8 *key, unsigned int
> keylen) +{
> +   enum sec_cipher_alg alg;
> +
> +   switch (keylen) {
> +   case AES_KEYSIZE_128 * 2:
> +   alg = SEC_C_AES_XTS_128;
> +   break;
> +   case AES_KEYSIZE_256 * 2:
> +   alg = SEC_C_AES_XTS_256;
> +   break;
> +   default:
> +   return -EINVAL;
> +   }
> +
> +   return sec_alg_skcipher_setkey(tfm, key, keylen, alg);
> +}

Can you please call the function xts_check_key or xts_verify_key before 
setting the key?

Ciao
Stephan

[PATCH] crypto: CTR DRBG - in-place cipher operation

[Stephan Müller]

The cipher implementations of the kernel crypto API favor in-place
cipher operations. Thus, switch the CTR cipher operation in the DRBG to
perform in-place operations. This is implemented by using the output
buffer as input buffer and zeroizing it before the cipher operation to
implement a CTR encryption of a NULL buffer.

The speed improvement is quite visibile with the following comparison
using the LRNG implementation.

Without the patch set:

  16 bytes|   12.267661 MB/s|61338304 bytes |  500213 ns
  32 bytes|   23.603770 MB/s|   118018848 bytes |  500073 ns
  64 bytes|   46.732262 MB/s|   233661312 bytes |  500241 ns
 128 bytes|   90.038042 MB/s|   450190208 bytes |  500244 ns
 256 bytes|  160.399616 MB/s|   801998080 bytes |  500393 ns
 512 bytes|  259.878400 MB/s|  1299392000 bytes |  501675 ns
1024 bytes|  386.050662 MB/s|  1930253312 bytes |  501661 ns
2048 bytes|  493.641728 MB/s|  2468208640 bytes |  501598 ns
4096 bytes|  581.835981 MB/s|  2909179904 bytes |  503426 ns

With the patch set:

  16 bytes | 17.051142 MB/s | 85255712 bytes |  500854 ns
  32 bytes | 32.695898 MB/s |163479488 bytes |  500544 ns
  64 bytes | 64.490739 MB/s |322453696 bytes |  500954 ns
 128 bytes |123.285043 MB/s |616425216 bytes |  500201 ns
 256 bytes |233.434573 MB/s |   1167172864 bytes |  500573 ns
 512 bytes |384.405197 MB/s |   1922025984 bytes |  500671 ns
1024 bytes |566.313370 MB/s |   2831566848 bytes |  501080 ns
2048 bytes |744.518042 MB/s |   3722590208 bytes |  500926 ns
4096 bytes |867.501670 MB/s |   4337508352 bytes |  502181 ns

Signed-off-by: Stephan Mueller 
---
 crypto/drbg.c | 34 ++
 include/crypto/drbg.h |  2 --
 2 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index ee302fd229ad..bc52d9562611 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -261,8 +261,7 @@ static int drbg_fini_sym_kernel(struct drbg_state *drbg);
 static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
  u8 *inbuf, u32 inbuflen,
  u8 *outbuf, u32 outlen);
-#define DRBG_CTR_NULL_LEN 128
-#define DRBG_OUTSCRATCHLEN DRBG_CTR_NULL_LEN
+#define DRBG_OUTSCRATCHLEN 256
 
 /* BCC function for CTR DRBG as defined in 10.4.3 */
 static int drbg_ctr_bcc(struct drbg_state *drbg,
@@ -555,8 +554,7 @@ static int drbg_ctr_generate(struct drbg_state *drbg,
}
 
/* 10.2.1.5.2 step 4.1 */
-   ret = drbg_kcapi_sym_ctr(drbg, drbg->ctr_null_value, DRBG_CTR_NULL_LEN,
-buf, len);
+   ret = drbg_kcapi_sym_ctr(drbg, NULL, 0, buf, len);
if (ret)
return ret;
 
@@ -1644,9 +1642,6 @@ static int drbg_fini_sym_kernel(struct drbg_state *drbg)
skcipher_request_free(drbg->ctr_req);
drbg->ctr_req = NULL;
 
-   kfree(drbg->ctr_null_value_buf);
-   drbg->ctr_null_value = NULL;
-
kfree(drbg->outscratchpadbuf);
drbg->outscratchpadbuf = NULL;
 
@@ -1697,15 +1692,6 @@ static int drbg_init_sym_kernel(struct drbg_state *drbg)
crypto_req_done, >ctr_wait);
 
alignmask = crypto_skcipher_alignmask(sk_tfm);
-   drbg->ctr_null_value_buf = kzalloc(DRBG_CTR_NULL_LEN + alignmask,
-  GFP_KERNEL);
-   if (!drbg->ctr_null_value_buf) {
-   drbg_fini_sym_kernel(drbg);
-   return -ENOMEM;
-   }
-   drbg->ctr_null_value = (u8 *)PTR_ALIGN(drbg->ctr_null_value_buf,
-  alignmask + 1);
-
drbg->outscratchpadbuf = kmalloc(DRBG_OUTSCRATCHLEN + alignmask,
 GFP_KERNEL);
if (!drbg->outscratchpadbuf) {
@@ -1716,7 +1702,7 @@ static int drbg_init_sym_kernel(struct drbg_state *drbg)
  alignmask + 1);
 
sg_init_table(>sg_in, 1);
-   sg_init_table(>sg_out, 1);
+   sg_init_one(>sg_out, drbg->outscratchpad, DRBG_OUTSCRATCHLEN);
 
return alignmask;
 }
@@ -1747,10 +1733,18 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
  u8 *outbuf, u32 outlen)
 {
struct scatterlist *sg_in = >sg_in, *sg_out = >sg_out;
+   u32 scratchpad_use = min_t(u32, outlen, DRBG_OUTSCRATCHLEN);
int ret;
 
-   sg_set_buf(sg_in, inbuf, inlen);
-   sg_set_buf(sg_out, drbg->outscratchpad, DRBG_OUTSCRATCHLEN);
+   if (inbuf) {
+   /* Use caller-provided input buffer */
+   sg_set_buf(sg_in, inbuf, inlen);
+   } else {
+   /* Use scratchpad for in-place operation */
+ 

Re: [PATCH 2/2] crypto: DRBG - use caller buffer if suitable

[Stephan Müller]

Am Donnerstag, 19. Juli 2018, 11:34:33 CEST schrieb Herbert Xu:

Hi Herbert,

> I think this is an abuse of virt_addr_valid.  It's meant to catch
> bogus uses of SG lists, it's not meant to be a guarantee that an
> address can be used on an SG list.

Thanks for your insights.
> 
> A better solution would be either an SG-list interface for rng,
> or alternatively a virtual address interface for sync skcipher.

My goal is to implement an in-place cipher operation drbg_kcapi_sym_ctr 
operating directly on the output buffer if possible. Without knowing whether 
the output buffer is valid for SGLs, I would need to use the scratchpad buffer 
as input/output buffer. That means that the data must be memcpy'ed from the 
scratchpad buffer to the output buffer.

To provide the fastest DRBG implementation, we should eliminate the memcpy in 
drbg_kcapi_sym_ctr and with it the restriction of generating 
DRBG_OUTSCRATCHLEN with one cipher invocation.

I.e. the ultimate goal is to operate directly on the output buffer if at all 
possible.

So far, all RNGs generate data for one buffer only. Thus, I would consider 
converting the RNG interface to use SGLs as overkill. Furthermore, if the RNG 
uses the SHASH ciphers (like the DRBG use case), the input data is expected to 
be a straight buffer.

Also, even the caller of the RNG may not know whether the destination buffer 
is valid for an SGL in case it is an intermediate layer.

Therefore, I am not sure that either having an SGL interface for the RNG API 
or a virtual address interface for the sync skcipher would be helpful.

Is there no way of identifying whether a buffer is valid for SGL operation?

PS: I experimented with the in-place cipher operation in drbg_kcapi_sym_ctr 
while operating directly on the output buffer. The result was a more than 3-
times increase in performance.

Current implementation:

  16 bytes|   12.267661 MB/s|61338304 bytes |  500213 ns
  32 bytes|   23.603770 MB/s|   118018848 bytes |  500073 ns
  64 bytes|   46.732262 MB/s|   233661312 bytes |  500241 ns
 128 bytes|   90.038042 MB/s|   450190208 bytes |  500244 ns
 256 bytes|  160.399616 MB/s|   801998080 bytes |  500393 ns
 512 bytes|  259.878400 MB/s|  1299392000 bytes |  501675 ns
1024 bytes|  386.050662 MB/s|  1930253312 bytes |  501661 ns
2048 bytes|  493.641728 MB/s|  2468208640 bytes |  501598 ns
4096 bytes|  581.835981 MB/s|  2909179904 bytes |  503426 ns

In-place cipher operation directly on output buffer:

  16 bytes|   16.017830 MB/s|80089152 bytes |  500752 ns
  32 bytes|   30.775686 MB/s|   153878432 bytes |  500701 ns
  64 bytes|   58.299430 MB/s|   291497152 bytes |  500466 ns
 128 bytes|  114.847462 MB/s|   574237312 bytes |  500385 ns
 256 bytes|  218.359859 MB/s|  1091799296 bytes |  500476 ns
 512 bytes|  416.003379 MB/s|  2080016896 bytes |  501105 ns
1024 bytes|  714.792346 MB/s|  3573961728 bytes |  5000718637 ns
2048 bytes|1.143480 GB/s|  5717401600 bytes |  500094 ns
4096 bytes|1.609953 GB/s|  8049766400 bytes |  501687 ns

Ciao
Stephan

[PATCH 1/2] crypto: DH - update test for public key verification

[Stephan Müller]

By adding a zero byte-length for the DH parameter Q value, the public
key verification test is disabled for the given test.

Reported-by: Eric Biggers 
Signed-off-by: Stephan Mueller 
---
 crypto/testmgr.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/crypto/testmgr.h b/crypto/testmgr.h
index b6362169771a..759462d65f41 100644
--- a/crypto/testmgr.h
+++ b/crypto/testmgr.h
@@ -644,12 +644,14 @@ static const struct kpp_testvec dh_tv_template[] = {
"\x11\x02" /* len */
"\x00\x01\x00\x00" /* key_size */
"\x00\x01\x00\x00" /* p_size */
+   "\x00\x00\x00\x00" /* q_size */
"\x01\x00\x00\x00" /* g_size */
 #else
"\x00\x01" /* type */
"\x02\x11" /* len */
"\x00\x00\x01\x00" /* key_size */
"\x00\x00\x01\x00" /* p_size */
+   "\x00\x00\x00\x00" /* q_size */
"\x00\x00\x00\x01" /* g_size */
 #endif
/* xa */
@@ -751,12 +753,14 @@ static const struct kpp_testvec dh_tv_template[] = {
"\x11\x02" /* len */
"\x00\x01\x00\x00" /* key_size */
"\x00\x01\x00\x00" /* p_size */
+   "\x00\x00\x00\x00" /* q_size */
"\x01\x00\x00\x00" /* g_size */
 #else
"\x00\x01" /* type */
"\x02\x11" /* len */
"\x00\x00\x01\x00" /* key_size */
"\x00\x00\x01\x00" /* p_size */
+   "\x00\x00\x00\x00" /* q_size */
"\x00\x00\x00\x01" /* g_size */
 #endif
/* xa */
-- 
2.17.1

Re: [PATCH v2] crypto: DH - add public key verification test

[Stephan Müller]

Am Mittwoch, 11. Juli 2018, 06:10:59 CEST schrieb Eric Biggers:

Hi Eric,

> You forgot to update the self-tests in the kernel, so they're failing now,
> as you *did* change the interface (the "key" is encoded differently now).

I was actually looking for failing self tests. But accidentally I disabled the 
self tests in my test bed which I just saw now. :-(

I will provide a fix shortly.

Ciao
Stephan

Re: [PATCH] crypto: dh - fix calculating encoded key size

[Stephan Müller]

Am Mittwoch, 11. Juli 2018, 05:59:05 CEST schrieb Eric Biggers:

Hi Eric,

> From: Eric Biggers 
> 
> It was forgotten to increase DH_KPP_SECRET_MIN_SIZE to include 'q_size',
> causing an out-of-bounds write of 4 bytes in crypto_dh_encode_key(), and
> an out-of-bounds read of 4 bytes in crypto_dh_decode_key().  Fix it.
> Also add a BUG_ON() if crypto_dh_encode_key() doesn't exactly fill the
> buffer, as that would have found this bug without resorting to KASAN.
> 
> Reported-by: syzbot+6d38d558c25b53b8f...@syzkaller.appspotmail.com
> Fixes: e3fe0ae12962 ("crypto: dh - add public key verification test")
> Signed-off-by: Eric Biggers 

Thanks.

Reviewed-by: Stephan Müller 

Ciao
Stephan

[PATCH 2/2] crypto: DRBG - use caller buffer if suitable

[Stephan Müller]

The SGL can directly operate caller-provided memory with the exception
of stack memory. The DRBG detects whether the caller provided
non-suitable memory and uses the scratchpad only on those circumstances.

This patch increases the speed of the CTR DRBG by 1 to 3 percent
depending on the buffer size of the output buffer.

Signed-off-by: Stephan Mueller 
---
 crypto/drbg.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index ee302fd229ad..193354e9d207 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1748,14 +1748,20 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
 {
struct scatterlist *sg_in = >sg_in, *sg_out = >sg_out;
int ret;
+   bool virt_addr_valid = virt_addr_valid(outbuf);
 
sg_set_buf(sg_in, inbuf, inlen);
-   sg_set_buf(sg_out, drbg->outscratchpad, DRBG_OUTSCRATCHLEN);
 
while (outlen) {
-   u32 cryptlen = min3(inlen, outlen, (u32)DRBG_OUTSCRATCHLEN);
+   u32 cryptlen = min_t(u32, inlen, outlen);
 
/* Output buffer may not be valid for SGL, use scratchpad */
+   if (virt_addr_valid) {
+   sg_set_buf(sg_out, outbuf, cryptlen);
+   } else {
+   cryptlen = min_t(u32, cryptlen, DRBG_OUTSCRATCHLEN);
+   sg_set_buf(sg_out, drbg->outscratchpad, cryptlen);
+   }
skcipher_request_set_crypt(drbg->ctr_req, sg_in, sg_out,
   cryptlen, drbg->V);
ret = crypto_wait_req(crypto_skcipher_encrypt(drbg->ctr_req),
@@ -1765,7 +1771,8 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
 
crypto_init_wait(>ctr_wait);
 
-   memcpy(outbuf, drbg->outscratchpad, cryptlen);
+   if (!virt_addr_valid)
+   memcpy(outbuf, drbg->outscratchpad, cryptlen);
 
outlen -= cryptlen;
outbuf += cryptlen;
@@ -1773,7 +1780,8 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
ret = 0;
 
 out:
-   memzero_explicit(drbg->outscratchpad, DRBG_OUTSCRATCHLEN);
+   if (!virt_addr_valid)
+   memzero_explicit(drbg->outscratchpad, DRBG_OUTSCRATCHLEN);
return ret;
 }
 #endif /* CONFIG_CRYPTO_DRBG_CTR */
-- 
2.17.1

[PATCH 0/2] crypto: DRBG - performance improvements for CTR DRBG

[Stephan Müller]

Hi Herbert,

Please find CTR DRBG performance improvements with the patches attached.

In the following, there is an example log taken with my LRNG implementation
using the getrandom(2) system call demonstrating the difference.

Without the patch set:

  16 bytes|   12.267661 MB/s|61338304 bytes |  500213 ns
  32 bytes|   23.603770 MB/s|   118018848 bytes |  500073 ns
  64 bytes|   46.732262 MB/s|   233661312 bytes |  500241 ns
 128 bytes|   90.038042 MB/s|   450190208 bytes |  500244 ns
 256 bytes|  160.399616 MB/s|   801998080 bytes |  500393 ns
 512 bytes|  259.878400 MB/s|  1299392000 bytes |  501675 ns
1024 bytes|  386.050662 MB/s|  1930253312 bytes |  501661 ns
2048 bytes|  493.641728 MB/s|  2468208640 bytes |  501598 ns
4096 bytes|  581.835981 MB/s|  2909179904 bytes |  503426 ns

With the patch set:

  16 bytes|   12.593974 MB/s|62969872 bytes |  500969 ns
  32 bytes|   24.112653 MB/s|   120563264 bytes |  500179 ns
  64 bytes|   48.216115 MB/s|   241080576 bytes |  500401 ns
 128 bytes|   94.260454 MB/s|   471302272 bytes |  500817 ns
 256 bytes|  164.752947 MB/s|   823764736 bytes |  500088 ns
 512 bytes|  270.364672 MB/s|  1351823360 bytes |  501695 ns
1024 bytes|  397.194035 MB/s|  1985970176 bytes |  500682 ns
2048 bytes|  517.212570 MB/s|  2586062848 bytes |  501324 ns
4096 bytes|  603.600486 MB/s|  3018002432 bytes |  503170 ns

Please note that you questioned the 2nd patch before in a different context.
However, I would like to ask whether it is appropriate considering the
performance improvements.

Thanks

Stephan Mueller (2):
  crypto: DRBG - eliminate constant reinitialization of SGL
  crypto: DRBG - use caller buffer if suitable

 crypto/drbg.c | 25 ++---
 include/crypto/drbg.h |  1 +
 2 files changed, 19 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH 1/2] crypto: DRBG - eliminate constant reinitialization of SGL

[Stephan Müller]

The CTR DRBG requires two SGLs pointing to input/output buffers for the
CTR AES operation. The used SGLs always have only one entry. Thus, the
SGL can be initialized during allocation time, preventing a
re-initialization of the SGLs during each call.

The performance is increased by about 1 to 3 percent depending on the
size of the requested buffer size.

Signed-off-by: Stephan Mueller 
---
 crypto/drbg.c | 11 +++
 include/crypto/drbg.h |  1 +
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 466a112a4446..ee302fd229ad 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1715,6 +1715,9 @@ static int drbg_init_sym_kernel(struct drbg_state *drbg)
drbg->outscratchpad = (u8 *)PTR_ALIGN(drbg->outscratchpadbuf,
  alignmask + 1);
 
+   sg_init_table(>sg_in, 1);
+   sg_init_table(>sg_out, 1);
+
return alignmask;
 }
 
@@ -1743,17 +1746,17 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
  u8 *inbuf, u32 inlen,
  u8 *outbuf, u32 outlen)
 {
-   struct scatterlist sg_in, sg_out;
+   struct scatterlist *sg_in = >sg_in, *sg_out = >sg_out;
int ret;
 
-   sg_init_one(_in, inbuf, inlen);
-   sg_init_one(_out, drbg->outscratchpad, DRBG_OUTSCRATCHLEN);
+   sg_set_buf(sg_in, inbuf, inlen);
+   sg_set_buf(sg_out, drbg->outscratchpad, DRBG_OUTSCRATCHLEN);
 
while (outlen) {
u32 cryptlen = min3(inlen, outlen, (u32)DRBG_OUTSCRATCHLEN);
 
/* Output buffer may not be valid for SGL, use scratchpad */
-   skcipher_request_set_crypt(drbg->ctr_req, _in, _out,
+   skcipher_request_set_crypt(drbg->ctr_req, sg_in, sg_out,
   cryptlen, drbg->V);
ret = crypto_wait_req(crypto_skcipher_encrypt(drbg->ctr_req),
>ctr_wait);
diff --git a/include/crypto/drbg.h b/include/crypto/drbg.h
index 8f941102af36..54b9f5d375f5 100644
--- a/include/crypto/drbg.h
+++ b/include/crypto/drbg.h
@@ -127,6 +127,7 @@ struct drbg_state {
__u8 *outscratchpadbuf; /* CTR mode output scratchpad */
 __u8 *outscratchpad;   /* CTR mode aligned outbuf */
struct crypto_wait ctr_wait;/* CTR mode async wait obj */
+   struct scatterlist sg_in, sg_out;   /* CTR mode SGLs */
 
bool seeded;/* DRBG fully seeded? */
bool pr;/* Prediction resistance enabled? */
-- 
2.17.1

[PATCH v2] crypto: DH - add public key verification test

[Stephan Müller]

Hi,

Changes v2:
* addition of a check that mpi_alloc succeeds.

---8<---

According to SP800-56A section 5.6.2.1, the public key to be processed
for the DH operation shall be checked for appropriateness. The check
shall covers the full verification test in case the domain parameter Q
is provided as defined in SP800-56A section 5.6.2.3.1. If Q is not
provided, the partial check according to SP800-56A section 5.6.2.3.2 is
performed.

The full verification test requires the presence of the domain parameter
Q. Thus, the patch adds the support to handle Q. It is permissible to
not provide the Q value as part of the domain parameters. This implies
that the interface is still backwards-compatible where so far only P and
G are to be provided. However, if Q is provided, it is imported.

Without the test, the NIST ACVP testing fails. After adding this check,
the NIST ACVP testing passes. Testing without providing the Q domain
parameter has been performed to verify the interface has not changed.

Signed-off-by: Stephan Mueller 
---
 crypto/dh.c | 66 ++---
 crypto/dh_helper.c  | 15 ---
 include/crypto/dh.h |  4 +++
 3 files changed, 79 insertions(+), 6 deletions(-)

diff --git a/crypto/dh.c b/crypto/dh.c
index 5659fe7f446d..8f79269db2b7 100644
--- a/crypto/dh.c
+++ b/crypto/dh.c
@@ -16,14 +16,16 @@
 #include 
 
 struct dh_ctx {
-   MPI p;
-   MPI g;
-   MPI xa;
+   MPI p;  /* Value is guaranteed to be set. */
+   MPI q;  /* Value is optional. */
+   MPI g;  /* Value is guaranteed to be set. */
+   MPI xa; /* Value is guaranteed to be set. */
 };
 
 static void dh_clear_ctx(struct dh_ctx *ctx)
 {
mpi_free(ctx->p);
+   mpi_free(ctx->q);
mpi_free(ctx->g);
mpi_free(ctx->xa);
memset(ctx, 0, sizeof(*ctx));
@@ -60,6 +62,12 @@ static int dh_set_params(struct dh_ctx *ctx, struct dh 
*params)
if (!ctx->p)
return -EINVAL;
 
+   if (params->q && params->q_size) {
+   ctx->q = mpi_read_raw_data(params->q, params->q_size);
+   if (!ctx->q)
+   return -EINVAL;
+   }
+
ctx->g = mpi_read_raw_data(params->g, params->g_size);
if (!ctx->g)
return -EINVAL;
@@ -93,6 +101,55 @@ static int dh_set_secret(struct crypto_kpp *tfm, const void 
*buf,
return -EINVAL;
 }
 
+/*
+ * SP800-56A public key verification:
+ *
+ * * If Q is provided as part of the domain paramenters, a full validation
+ *   according to SP800-56A section 5.6.2.3.1 is performed.
+ *
+ * * If Q is not provided, a partial validation according to SP800-56A section
+ *   5.6.2.3.2 is performed.
+ */
+static int dh_is_pubkey_valid(struct dh_ctx *ctx, MPI y)
+{
+   if (unlikely(!ctx->p))
+   return -EINVAL;
+
+   /*
+* Step 1: Verify that 2 <= y <= p - 2.
+*
+* The upper limit check is actually y < p instead of y < p - 1
+* as the mpi_sub_ui function is yet missing.
+*/
+   if (mpi_cmp_ui(y, 1) < 1 || mpi_cmp(y, ctx->p) >= 0)
+   return -EINVAL;
+
+   /* Step 2: Verify that 1 = y^q mod p */
+   if (ctx->q) {
+   MPI val = mpi_alloc(0);
+   int ret;
+
+   if (!val)
+   return -ENOMEM;
+
+   ret = mpi_powm(val, y, ctx->q, ctx->p);
+
+   if (ret) {
+   mpi_free(val);
+   return ret;
+   }
+
+   ret = mpi_cmp_ui(val, 1);
+
+   mpi_free(val);
+
+   if (ret != 0)
+   return -EINVAL;
+   }
+
+   return 0;
+}
+
 static int dh_compute_value(struct kpp_request *req)
 {
struct crypto_kpp *tfm = crypto_kpp_reqtfm(req);
@@ -115,6 +172,9 @@ static int dh_compute_value(struct kpp_request *req)
ret = -EINVAL;
goto err_free_val;
}
+   ret = dh_is_pubkey_valid(ctx, base);
+   if (ret)
+   goto err_free_val;
} else {
base = ctx->g;
}
diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c
index 24fdb2ecaa85..a7de3d9ce5ac 100644
--- a/crypto/dh_helper.c
+++ b/crypto/dh_helper.c
@@ -30,7 +30,7 @@ static inline const u8 *dh_unpack_data(void *dst, const void 
*src, size_t size)
 
 static inline unsigned int dh_data_size(const struct dh *p)
 {
-   return p->key_size + p->p_size + p->g_size;
+   return p->key_size + p->p_size + p->q_size + p->g_size;
 }
 
 unsigned int crypto_dh_key_len(const struct dh *p)
@@ -56,9 +56,11 @@ int crypto_dh_encode_key(char *buf, unsigned int len, const 
struct dh *params)
ptr = dh_pack_data(ptr, , sizeof(secret));
ptr = dh_pack_data(ptr, >key_size, sizeof(params->key_size));
ptr = dh_pack_data(ptr, >p_size, sizeof(params->p_size));
+   ptr = dh_pack_data(ptr, >q_size, 

[RFC PATCH] crypto: DH - add public key verification test

[Stephan Müller]

Hi,

This patch is an RFC to discuss the following: step 1 in
dh_is_pubkey_valid requires the public key to be in the range of
1 < y < p - 1. The currently implemented check is 1 < y < p since the
calculation of p - 1 requires the presence of mpi_sub or mpi_sub_ui.
Both functions are currently not implemented and I am not sure we
really need to add them for just this check.

I personally think that the check of 1 < y < p would suffice for now
considering that the verification of step 2 using the modular
exponentiation is implemented as well.

In case mpi_sub is added for another reason, we can update the check
code to be accurate.

The NIST ACVP testing passes with the code provided below.

---8<---

According to SP800-56A section 5.6.2.1, the public key to be processed
for the DH operation shall be checked for appropriateness. The check
shall covers the full verification test in case the domain parameter Q
is provided as defined in SP800-56A section 5.6.2.3.1. If Q is not
provided, the partial check according to SP800-56A section 5.6.2.3.2 is
performed.

The full verification test requires the presence of the domain parameter
Q. Thus, the patch adds the support to handle Q. It is permissible to
not provide the Q value as part of the domain parameters. This implies
that the interface is still backwards-compatible where so far only P and
G are to be provided. However, if Q is provided, it is imported.

Without the test, the NIST ACVP testing fails. After adding this check,
the NIST ACVP testing passes. Testing without providing the Q domain
parameter has been performed to verify the interface has not changed.

Signed-off-by: Stephan Mueller 
---
 crypto/dh.c | 61 ++---
 crypto/dh_helper.c  | 15 ---
 include/crypto/dh.h |  4 +++
 3 files changed, 74 insertions(+), 6 deletions(-)

diff --git a/crypto/dh.c b/crypto/dh.c
index 5659fe7f446d..5d6d397c20ee 100644
--- a/crypto/dh.c
+++ b/crypto/dh.c
@@ -16,14 +16,16 @@
 #include 
 
 struct dh_ctx {
-   MPI p;
-   MPI g;
-   MPI xa;
+   MPI p;  /* Value is guaranteed to be set. */
+   MPI q;  /* Value is optional. */
+   MPI g;  /* Value is guaranteed to be set. */
+   MPI xa; /* Value is guaranteed to be set. */
 };
 
 static void dh_clear_ctx(struct dh_ctx *ctx)
 {
mpi_free(ctx->p);
+   mpi_free(ctx->q);
mpi_free(ctx->g);
mpi_free(ctx->xa);
memset(ctx, 0, sizeof(*ctx));
@@ -60,6 +62,12 @@ static int dh_set_params(struct dh_ctx *ctx, struct dh 
*params)
if (!ctx->p)
return -EINVAL;
 
+   if (params->q && params->q_size) {
+   ctx->q = mpi_read_raw_data(params->q, params->q_size);
+   if (!ctx->q)
+   return -EINVAL;
+   }
+
ctx->g = mpi_read_raw_data(params->g, params->g_size);
if (!ctx->g)
return -EINVAL;
@@ -93,6 +101,50 @@ static int dh_set_secret(struct crypto_kpp *tfm, const void 
*buf,
return -EINVAL;
 }
 
+/*
+ * SP800-56A public key verification:
+ *
+ * * If Q is provided as part of the domain paramenters, a full validation
+ *   according to SP800-56A section 5.6.2.3.1 is performed.
+ *
+ * * If Q is not provided, a partial validation according to SP800-56A section
+ *   5.6.2.3.2 is performed.
+ */
+static int dh_is_pubkey_valid(struct dh_ctx *ctx, MPI y)
+{
+   if (unlikely(!ctx->p))
+   return -EINVAL;
+
+   /*
+* Step 1: Verify that 2 <= y <= p - 2.
+*
+* The upper limit check is actually y < p instead of y < p - 1
+* as the mpi_sub_ui function is yet missing.
+*/
+   if (mpi_cmp_ui(y, 1) < 1 || mpi_cmp(y, ctx->p) >= 0)
+   return -EINVAL;
+
+   /* Step 2: Verify that 1 = y^q mod p */
+   if (ctx->q) {
+   MPI val = mpi_alloc(0);
+   int ret = mpi_powm(val, y, ctx->q, ctx->p);
+
+   if (ret) {
+   mpi_free(val);
+   return ret;
+   }
+
+   ret = mpi_cmp_ui(val, 1);
+
+   mpi_free(val);
+
+   if (ret != 0)
+   return -EINVAL;
+   }
+
+   return 0;
+}
+
 static int dh_compute_value(struct kpp_request *req)
 {
struct crypto_kpp *tfm = crypto_kpp_reqtfm(req);
@@ -115,6 +167,9 @@ static int dh_compute_value(struct kpp_request *req)
ret = -EINVAL;
goto err_free_val;
}
+   ret = dh_is_pubkey_valid(ctx, base);
+   if (ret)
+   goto err_free_val;
} else {
base = ctx->g;
}
diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c
index 24fdb2ecaa85..a7de3d9ce5ac 100644
--- a/crypto/dh_helper.c
+++ b/crypto/dh_helper.c
@@ -30,7 +30,7 @@ static inline const u8 *dh_unpack_data(void *dst, const void 
*src, size_t size)
 
 static inline unsigned int 

[PATCH] crypto: ECDH - add public key verification test

[Stephan Müller]

According to SP800-56A section 5.6.2.1, the public key to be processed
for the ECDH operation shall be checked for appropriateness. When the
public key is considered to be an ephemeral key, the partial validation
test as defined in SP800-56A section 5.6.2.3.4 can be applied.

The partial verification test requires the presence of the field
elements of a and b. For the implemented NIST curves, b is defined in
FIPS 186-4 appendix D.1.2. The element a is implicitly given with the
Weierstrass equation given in D.1.2 where a = p - 3.

Without the test, the NIST ACVP testing fails. After adding this check,
the NIST ACVP testing passes.

Signed-off-by: Stephan Mueller 
---
 crypto/ecc.c| 42 +
 crypto/ecc_curve_defs.h | 22 +
 2 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 815541309a95..8facafd67802 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -1019,6 +1019,36 @@ int ecc_make_pub_key(unsigned int curve_id, unsigned int 
ndigits,
return ret;
 }
 
+/* SP800-56A section 5.6.2.3.4 partial verification: ephemeral keys only */
+static int ecc_is_pubkey_valid_partial(const struct ecc_curve *curve,
+  struct ecc_point *pk)
+{
+   u64 yy[ECC_MAX_DIGITS], xxx[ECC_MAX_DIGITS], w[ECC_MAX_DIGITS];
+
+   /* Check 1: Verify key is not the zero point. */
+   if (ecc_point_is_zero(pk))
+   return -EINVAL;
+
+   /* Check 2: Verify key is in the range [1, p-1]. */
+   if (vli_cmp(curve->p, pk->x, pk->ndigits) != 1)
+   return -EINVAL;
+   if (vli_cmp(curve->p, pk->y, pk->ndigits) != 1)
+   return -EINVAL;
+
+   /* Check 3: Verify that y^2 == (x^3 + a·x + b) mod p */
+   vli_mod_square_fast(yy, pk->y, curve->p, pk->ndigits); /* y^2 */
+   vli_mod_square_fast(xxx, pk->x, curve->p, pk->ndigits); /* x^2 */
+   vli_mod_mult_fast(xxx, xxx, pk->x, curve->p, pk->ndigits); /* x^3 */
+   vli_mod_mult_fast(w, curve->a, pk->x, curve->p, pk->ndigits); /* a·x */
+   vli_mod_add(w, w, curve->b, curve->p, pk->ndigits); /* a·x + b */
+   vli_mod_add(w, w, xxx, curve->p, pk->ndigits); /* x^3 + a·x + b */
+   if (vli_cmp(yy, w, pk->ndigits) != 0) /* Equation */
+   return -EINVAL;
+
+   return 0;
+
+}
+
 int crypto_ecdh_shared_secret(unsigned int curve_id, unsigned int ndigits,
  const u64 *private_key, const u64 *public_key,
  u64 *secret)
@@ -1046,16 +1076,20 @@ int crypto_ecdh_shared_secret(unsigned int curve_id, 
unsigned int ndigits,
goto out;
}
 
+   ecc_swap_digits(public_key, pk->x, ndigits);
+   ecc_swap_digits(_key[ndigits], pk->y, ndigits);
+   ret = ecc_is_pubkey_valid_partial(curve, pk);
+   if (ret)
+   goto err_alloc_product;
+
+   ecc_swap_digits(private_key, priv, ndigits);
+
product = ecc_alloc_point(ndigits);
if (!product) {
ret = -ENOMEM;
goto err_alloc_product;
}
 
-   ecc_swap_digits(public_key, pk->x, ndigits);
-   ecc_swap_digits(_key[ndigits], pk->y, ndigits);
-   ecc_swap_digits(private_key, priv, ndigits);
-
ecc_point_mult(product, pk, priv, rand_z, curve->p, ndigits);
 
ecc_swap_digits(product->x, secret, ndigits);
diff --git a/crypto/ecc_curve_defs.h b/crypto/ecc_curve_defs.h
index b80f45da829c..94e883a9403f 100644
--- a/crypto/ecc_curve_defs.h
+++ b/crypto/ecc_curve_defs.h
@@ -13,9 +13,11 @@ struct ecc_curve {
struct ecc_point g;
u64 *p;
u64 *n;
+   u64 *a;
+   u64 *b;
 };
 
-/* NIST P-192 */
+/* NIST P-192: a = p - 3 */
 static u64 nist_p192_g_x[] = { 0xF4FF0AFD82FF1012ull, 0x7CBF20EB43A18800ull,
0x188DA80EB03090F6ull };
 static u64 nist_p192_g_y[] = { 0x73F977A11E794811ull, 0x631011ED6B24CDD5ull,
@@ -24,6 +26,10 @@ static u64 nist_p192_p[] = { 0xull, 
0xFFFEull,
0xull };
 static u64 nist_p192_n[] = { 0x146BC9B1B4D22831ull, 0x99DEF836ull,
0xull };
+static u64 nist_p192_a[] = { 0xFFFCull, 0xFFFEull,
+   0xFFFEull };
+static u64 nist_p192_b[] = { 0xFEB8DEECC146B9B1ull, 0x0FA7E9AB72243049ull,
+   0x64210519E59C80E7ull };
 static struct ecc_curve nist_p192 = {
.name = "nist_192",
.g = {
@@ -32,10 +38,12 @@ static struct ecc_curve nist_p192 = {
.ndigits = 3,
},
.p = nist_p192_p,
-   .n = nist_p192_n
+   .n = nist_p192_n,
+   .a = nist_p192_a,
+   .b = nist_p192_b
 };
 
-/* NIST P-256 */
+/* NIST P-256: a = p - 3 */
 static u64 nist_p256_g_x[] = { 0xF4A13945D898C296ull, 0x77037D812DEB33A0ull,
 

Re: cryptomgr_test / drbg_ctr: BUG: sleeping function called from invalid context

[Stephan Müller]

Am Freitag, 18. Mai 2018, 10:36:04 CEST schrieb Geert Uytterhoeven:

Hi Geert,
> 
> I tried following the code path, but couldn't find where it went wrong.
> 
> mutex_lock(>drbg_mutex) is called from drbg_instantiate(), which is
> inlined by the compiler into drbg_kcapi_seed().
> 
> Do you have a clue?

It is the first time I hear from such an issue. Yes, the DRBG should not be 
called in atomic context. But I do not see where we have an atomic context 
(either a spin_lock or in an interrupt handler) when we are executing the test 
manager.

I will keep looking.

Ciao
Stephan

4.16: /dev/random - a new approach

[Stephan Müller]

Hi,

The patch set available at [1] provides a different approach to /dev/random 
which I call Linux Random Number Generator (LRNG) to collect entropy within 
the Linux kernel. The main improvements compared to the legacy /dev/random is 
to provide sufficient entropy during boot time as well as in virtual 
environments and when using SSDs. A secondary design goal is to limit the 
impact of the entropy collection on massive parallel systems and also allow 
the use accelerated cryptographic primitives. Also, all steps of the entropic 
data processing are testable.

The design and implementation is driven by a set of goals described in [1]
that the LRNG completely implements. Furthermore, [1] includes a
comparison with RNG design suggestions such as SP800-90B, SP800-90C, and
AIS20/31.

The LRNG provides a complete separation of the noise source maintenance
and the collection of entropy into an entropy pool from the post-processing
using a pseudo-random number generator. Different PRNGs are supported,
including:

* Built-in ChaCha20 PRNG which has no dependency to other kernel
  frameworks.

* SP800-90A DRBG using the kernel crypto API including its accelerated
  raw cipher implementations.

* Arbitrary PRNGs registered with the kernel crypto API

Booting the patch with the kernel command line option
"dyndbg=file drivers/char/lrng* +p" generates logs indicating the operation
of the LRNG. Each log is pre-pended with "lrng:".

The LRNG has a flexible design by allowing an easy replacement of the
deterministic random number generator component.

[1] http://www.chronox.de/lrng.html

Changes (compared to the previous patch set for 4.15):

 * Addition of SPOX copyright identifier
 * Use the updated poll infrastructure
 * Add the kernel crypto API PRNG support
-- 
2.14.3

libkcapi v1.1.0 released

[Stephan Müller]

Hi,

The Linux kernel exports a network interface of type AF_ALG to allow user 
space to utilize the kernel crypto API. libkcapi uses this network interface 
and exports an easy to use API so that a developer does not need to consider 
the low-level network interface handling. 

The library does not implement any low level cipher algorithms. All consumer 
requests are sent to the kernel for processing. Results from the kernel crypto 
API are returned to the consumer via the library API.

The kernel interface and therefore this library can be used by unprivileged 
processes. By using the convenience API functions, one API call performs one 
complete cipher operation.

The library code archive also provides a drop-in replacement for the command 
line tools of sha*sum, fipscheck/fipshmac and sha512hmac. It also contains 
command line tools to use the hashing, symmetric ciphers and random number 
generation on the command line.

The source code and the documentation is available at [1].

[1] http://www.chronox.de/libkcapi.html

Changes 1.1.0
 * API Enhancement: Addition of kcapi_handle_reinit
 * Fix: simplify code by removing the internal *_fd functions from
   kcapi-kernel-if.c
 * Fix: add a loop around the read system call to always obtain all generated
   data
 * Fix: use host compiler for compiling docproc (reported by Christophe LEROY,
   fixed by Björn Esser)
 * Fix: make error handling of hashing applications consistent with coreutils
   applications (reported by Christophe LEROY)
 * Fix: support for zero length files (patched by Ondrej Mosnáček)
 * Fix: support for zero message hashes on kernels <= 4.9 (patched by
Ondrej Mosnáček)
 * Fix: Add Travis CI test system provided by Ondrej Mosnáček
 * Fix: Add several fixes to kcapi-hasher by Ondrej Mosnáček
 * Fix: Add additional tests for kcapi-hasher by Ondrej Mosnáček
 * Fix: Apply unpadding only to last block of data by Ondrej Mosnáček
 * Fix: Fix resource leaks in error code paths suggested by Ondrej Mosnáček
 * Enhancement: achieve hmaccalc CLI equivalence by Ondrej Mosnáček

Ciao
Stephan

[PATCH] crypto: drbg - set freed buffers to NULL

[Stephan Müller]

Add the Fixes, CC stable tags.

---8<---

During freeing of the internal buffers used by the DRBG, set the pointer
to NULL. It is possible that the context with the freed buffers is
reused. In case of an error during initialization where the pointers
do not yet point to allocated memory, the NULL value prevents a double
free.

Cc: sta...@vger.kernel.org
Fixes: 3cfc3b9721123 ("crypto: drbg - use aligned buffers")
Signed-off-by: Stephan Mueller 
Reported-by: syzbot+75397ee3df5c70164...@syzkaller.appspotmail.com
---
 crypto/drbg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 4faa2781c964..466a112a4446 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1134,8 +1134,10 @@ static inline void drbg_dealloc_state(struct drbg_state 
*drbg)
if (!drbg)
return;
kzfree(drbg->Vbuf);
+   drbg->Vbuf = NULL;
drbg->V = NULL;
kzfree(drbg->Cbuf);
+   drbg->Cbuf = NULL;
drbg->C = NULL;
kzfree(drbg->scratchpadbuf);
drbg->scratchpadbuf = NULL;
-- 
2.14.3

[PATCH] crypto: drbg - set freed buffers to NULL

[Stephan Müller]

Sorry, this time with the proper subject line.

---8<---

During freeing of the internal buffers used by the DRBG, set the pointer
to NULL. It is possible that the context with the freed buffers is
reused. In case of an error during initialization where the pointers
do not yet point to allocated memory, the NULL value prevents a double
free.

Signed-off-by: Stephan Mueller 
Reported-by: syzbot+75397ee3df5c70164...@syzkaller.appspotmail.com
---
 crypto/drbg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 4faa2781c964..466a112a4446 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1134,8 +1134,10 @@ static inline void drbg_dealloc_state(struct drbg_state 
*drbg)
if (!drbg)
return;
kzfree(drbg->Vbuf);
+   drbg->Vbuf = NULL;
drbg->V = NULL;
kzfree(drbg->Cbuf);
+   drbg->Cbuf = NULL;
drbg->C = NULL;
kzfree(drbg->scratchpadbuf);
drbg->scratchpadbuf = NULL;
-- 
2.14.3

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Stephan Müller]

Hi Dimitry,

This fix prevents the kernel from crashing when injecting the fault. 
Stack traces are yet shown but I guess that is expected every time
a fault is injected.

As to why KASAN did not notice this one, I am not sure. Maybe it is
because I use two buffer pointers to point to (almost) the same memory
(one that is aligned and one pointing to the complete buffer)?

---8<---

During freeing of the internal buffers used by the DRBG, set the pointer
to NULL. It is possible that the context with the freed buffers is
reused. In case of an error during initialization where the pointers
do not yet point to allocated memory, the NULL value prevents a double
free.

Signed-off-by: Stephan Mueller 
Reported-by: syzbot+75397ee3df5c70164...@syzkaller.appspotmail.com
---
 crypto/drbg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 4faa2781c964..466a112a4446 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1134,8 +1134,10 @@ static inline void drbg_dealloc_state(struct drbg_state 
*drbg)
if (!drbg)
return;
kzfree(drbg->Vbuf);
+   drbg->Vbuf = NULL;
drbg->V = NULL;
kzfree(drbg->Cbuf);
+   drbg->Cbuf = NULL;
drbg->C = NULL;
kzfree(drbg->scratchpadbuf);
drbg->scratchpadbuf = NULL;
-- 
2.14.3

[PATCH] crypto: DRBG - guard uninstantion by lock

[Stephan Müller]

Am Sonntag, 8. April 2018, 17:41:17 CEST schrieb Dmitry Vyukov:

Hi Dmitry,
> 
> Hi,
> 
> Here is config and kernel commit:
> https://groups.google.com/d/msg/syzkaller-bugs/PINYyzoaG1s/ntZPOZdcCAAJ
> You can also find compiler and image here if necessary:
> https://github.com/google/syzkaller/blob/master/docs/syzbot.md
> 
> And note that the program needs to be compiled with -m32. The bugs is
> probably not-compat specific, but the program injects fault into a
> particular malloc invocation and maybe malloc numbering is affected by
> compat path.

I am unable to reproduce the issue. But since you mention that you induce 
errors, I could see that the unlocking of the DRBG context is too soon.

Can you please check whether the attached patch fixes the issue?

Thanks

---8<---

In the error code path, the uninstantiation must be guarded by a lock to
ensure that the modification of the context is fully atomic.

Signed-off-by: Stephan Mueller 
Reported-by: syzkaller
---
 crypto/drbg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 4faa2781c964..68c1949a253f 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1510,8 +1510,8 @@ static int drbg_instantiate(struct drbg_state *drbg, 
struct drbg_string *pers,
return ret;
 
 free_everything:
-   mutex_unlock(>drbg_mutex);
drbg_uninstantiate(drbg);
+   mutex_unlock(>drbg_mutex);
return ret;
 }
 
-- 
2.14.3

[PATCH] AF_ALG: register completely initialized request in list

[Stephan Müller]

Hi,

May I ask to check whether this patch fixes the issue? I cannot re-create
the issue with the reproducter. Yet, as far as I understand, you try to
induce errors which shall validate whether the error code paths are correct.

The fix below should ensure this now.

Thanks a lot.

---8<---

>From 8f083e7b0684a9f91c186d7b46eec34e439689c3 Mon Sep 17 00:00:00 2001
From: Stephan Mueller 
Date: Sun, 8 Apr 2018 19:53:59 +0200
Subject: [PATCH] AF_ALG: Initialize sg_num_bytes in error code path

The RX SGL in processing is already registered with the RX SGL tracking
list to support proper cleanup. The cleanup code path uses the
sg_num_bytes variable which must therefore be always initialized, even
in the error code path.

Signed-off-by: Stephan Mueller 
Reported-by: syzbot+9c251bdd09f83b92b...@syzkaller.appspotmail.com
---
 crypto/af_alg.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index c49766b03165..0d555c072669 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1156,8 +1156,10 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, 
int flags,
 
/* make one iovec available as scatterlist */
err = af_alg_make_sg(>sgl, >msg_iter, seglen);
-   if (err < 0)
+   if (err < 0) {
+   rsgl->sg_num_bytes = 0;
return err;
+   }
 
/* chain the new scatterlist with previous one */
if (areq->last_rsgl)
-- 
2.14.3

Re: WARNING in kmem_cache_free

[Stephan Müller]

Am Sonntag, 8. April 2018, 13:18:06 CEST schrieb Dmitry Vyukov:

Hi Dmitry,

> 
> Running syz-repro utility on this log, I think I've found the guilty guy:
> https://gist.githubusercontent.com/dvyukov/1dd75d55efd238e7207af1cc38478b3a/
> raw/403859b56b161a6fbb158e8953fac5bb6e73b1a1/gistfile1.txt
> 

I am unable to reproduce it with the code. I am using the current 
cryptodev-2.6 tree with kazan enabled. Could you please give me your kernel 
config or a pointer of the used tree?

> It crashes as:
> BUG: KASAN: use-after-free in drbg_kcapi_seed+0x1178/0x12e0
> and:
> BUG: unable to handle kernel paging request at ebe00020
> and with other indications of badly corrupted heap.
> 
> This points to crypto/drbg.c, so +crypto maintainers.


Ciao
Stephan

Re: error in libkcapi 1.0.3 for aead aio

[Stephan Müller]

Am Sonntag, 25. Februar 2018, 06:25:06 CET schrieb Harsh Jain:

Hi Harsh,

> Hi Stephan,
> 
> 1 of the test mentioned in test.sh is failing for AEAD AIO operation even
> thought driver is returning EBADMSG(as expected) to af_alg with latest
> cryptodev tree.
> 
> Debug log and strace attached.
> 
> Command :
> 
> strace -o strace.log ../bin/kcapi   -x 10   -c "gcm(aes)" -i
> 7815d4b06ae50c9c56e87bd7 -k ea38ac0c9b9998c80e28fb496a2b88d9 -a
> "853f98a750098bec1aa7497e979e78098155c877879556bb51ddeb6374cbaefc" -t
> "c4ce58985b7203094be1d134c1b8ab0b" -q "b03692f86d1b8b39baf2abb255197c98"

I am not seeing that the driver reports -EBADMSG in the strace log. If you see 
the following line, you will spot the issue:

io_getevents(140715452821504, 1, 1, {{(nil), 0x24a41c0, 4294967222, 0}}, NULL) 
= 1

The 4th parameter is struct io_event that is returned from the kernel. It 
contains as 3rd value "4294967222". This value ought to be the return code of 
the crypto operation. If the value is positive, it returns the number of 
processed bytes. If it is negative, it holds the error code.

I am not sure you expect a value close to 2^32 bytes as return code -- from 
the io_submit syscall where you send 48 bytes and from the command above I 
assume that this return value is wrong.
> 
> Thanks & Regards
> 
> Harsh Jain


Ciao
Stephan

Re: [PATCH v3 0/4] crypto: AF_ALG AIO improvements

[Stephan Müller]

Am Freitag, 23. Februar 2018, 13:00:26 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Feb 23, 2018 at 09:33:33AM +0100, Stephan Müller wrote:
> > A simple copy operation, however, will imply that in one AIO recvmsg
> > request, only *one* IOCB can be set and processed.
> 
> Sure, but the recvmsg will return as soon as the crypto API encrypt
> or decrypt function returns.  It's still fully async.  It's just
> that the setup part needs to be done with sendmsg/recvmsg.

Wouldn't a copy of the ctx->iv into a per-request buffer change the behavoir 
of the AF_ALG interface significantly?

Today, if multiple IOCBs are submitted, most cipher implementations would 
serialize the requests (e.g. all implementations that behave synchronous in 
nature like all software implementations).

Thus, when copying the ctx->iv into a separate per-request buffer, suddenly 
all block-chained cipher operations are not block chained any more.
> 
> Even if we wanted to do what you stated, just inlining the IV isn't
> enough.  You'd also need to inline the assoclen, and probably the
> optype in case you want to mix encrypt/decrypt too.

Maybe that is what we have to do.
> 
> However, I must say that I don't see the point of going all the way
> to support such a bulk submission interface (e.g., lio_listio).

IMHO, the point is that AF_ALG is the only interface to allow userspace to 
utilize hardware crypto implementations. For example, on a small chip with 
hardware crypto support, your user space code can offload crypto to that 
hardware to free CPU time.

How else would somebody access its crypto accelerators?
> 
> Remember, the algif interface due to its inherent overhead is meant
> for bulk data.  That is, the processing time for each request is
> dominated by the actual processing, not the submission process.

I see that. And for smaller chips with crypto support, this would be the case 
IMHO. Especially if we streamline the AF_ALG overhead such that we reduce the 
number of syscalls and user/kernel space roundtrips.
> 
> If you're instead processing lots of tiny requests, do NOT use
> algif because it's not designed for that.

The only issue in this case is that it makes the operation slower. 
> 
> Therefore spending too much time to optimise the submission overhead
> seems pointless to me.
> 
> Cheers,


Ciao
Stephan

Re: [PATCH v3 0/4] crypto: AF_ALG AIO improvements

[Stephan Müller]

Am Donnerstag, 22. Februar 2018, 14:06:00 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Feb 09, 2018 at 11:02:27PM +0100, Stephan Müller wrote:
> > Hi,
> > 
> > Herbert, the patch 1 is meant for stable. However, this patch as is
> > only applies to the new AF_ALG interface implementation. Though,
> > the issue goes back to the first implementation of AIO support.
> > Shall I try prepare a patch for the old AF_ALG implementation
> > as well?
> 
> I think this is overcomplicated.  We simply need to duplicate
> the IV for async operations.  That is, if you're doing an async
> recvmsg then the IV must be duplicated and stored in the request
> instead of the global context.

A simple copy operation, however, will imply that in one AIO recvmsg request, 
only *one* IOCB can be set and processed.

If multiple IOCBs are processed, then each IOCB would get the same IV with the 
same key, just different plain/ciphertext. With this approach, I think we 
neither support a fully parallel execution of independent cipher operations 
(as suppgested with the inline IV patch that requires the caller to provide a 
separate IV for each recvmsg call) nor a serialized operation of multiple 
IOCBs where the IV-based block chaining links the different dependent IOBCs.

Therefore, I would think that between each recvmsg call with one IOCB another 
sendmsg must be made to set the IV in order to support either of the 
aforementioned scenarios (parallel and serialized). IMHO this seems to be a 
waste of resources.
> 
> Remember, you must not start the sendmsg for the next request
> until the recvmsg system call for the previous request has completed.

I understand that, but one AIO recvmsg can process multiple cipher operations 
all at once (one IOCB defines one operation). Otherwise we do not utilize the 
true potential of the AIO support. We would use the AIO support to mimic 
synchronous behavior.
> 
> Cheers,


Ciao
Stephan

[PATCH v3 3/4] crypto: AF_ALG - allow driver to serialize IV access

[Stephan Müller]

The mutex in AF_ALG to serialize access to the IV ensures full
serialization of requests sent to the crypto driver.

However, the hardware may implement serialization to the IV such that
preparation work without touching the IV can already happen while the IV
is processed by another operation. This may speed up the AIO processing.

The following ASCII art demonstrates this.

AF_ALG mutex handling implements the following logic:

[REQUEST 1 from userspace]   [REQUEST 2 from userspace]
 ||
 [AF_ALG/SOCKET]   [AF_ALG/SOCKET]
 ||
NOTHING BLOCKING (lock mutex) |
 | Queued on Mutex
 [BUILD / MAP HW DESCS]   |
 ||
   [Place in HW Queue]|
 ||
 [Process]|
 ||
[Interrupt]   |
 ||
 [Respond and update IV]  |
 ||
[unlock mutex]   Nothing Blocking (lock mutex)
 ||
Don't care beyond here.   [BUILD / MAP HW DESCS]
  |
  [Place in HW Queue]
  |
  [Process]
  |
 [Interrupt]
  |
[Respond and update IV]
  |
Don't care beyond here.

A crypto driver may implement the following serialization:

[REQUEST 1 from userspace]   [REQUEST 2 from userspace]
 ||
  [AF_ALG/SOCKET]  [AF_ALG/SOCKET]
 ||
  [BUILD / MAP HW DESCS] [BUILD / MAP HW DESCS]
 ||
NOTHING BLOCKING IV in flight (enqueue sw queue)
 ||
   [Place in HW Queue]|
 ||
 [Process]|
 ||
[Interrupt]   |
 ||
 [Respond and update IV] Nothing Blocking (dequeue sw queue)
 ||
  Don't care beyond here.  [Place in HW Queue]
  |
  [Process]
  |
 [Interrupt]
  |
[Respond and update IV]
  |
Don't care beyond here.

If the driver implements its own serialization (i.e. AF_ALG does not
serialize the access to the IV), the crypto implementation must set the
flag CRYPTO_ALG_SERIALIZES_IV_ACCESS.

Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 13 -
 crypto/algif_aead.c |  1 +
 crypto/algif_skcipher.c |  1 +
 include/crypto/if_alg.h | 13 +
 include/linux/crypto.h  | 15 +++
 5 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 7f80dcfc12a6..56b4da65025a 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1247,8 +1247,10 @@ int af_alg_get_iv(struct sock *sk, struct 
af_alg_async_req *areq)
return 0;
 
/* No inline IV, use ctx IV buffer and lock it */
-   if (ctx->iiv == ALG_IV_SERIAL_PROCESSING)
-   return mutex_lock_interruptible(>ivlock);
+   if (ctx->iiv == ALG_IV_SERIAL_PROCESSING) {
+   return (ctx->lock_iv) ?
+   mutex_lock_interruptible(>ivlock) : 0;
+   }
 
/* Inline IV handling: There must be the IV data present. */
if (ctx->used < ctx->ivlen || list_empty(>tsgl_list))
@@ -1280,12 +1282,13 @@ void af_alg_put_iv(struct sock *sk)
struct alg_sock *ask = alg_sk(sk);
struct af_alg_ctx *ctx = ask->private;
 
-   /* To resemble af_alg_get_iv, do not merge the two branches. */
if (!ctx->ivlen)
return;
 
-   if (ctx->iiv == ALG_IV_SERIAL_PROCESSING)
-   mutex_unlock(>ivlock);
+   if (ctx->iiv == ALG_IV_SERIAL_PROCESSING) {
+   if (ctx->lock_iv)
+   mutex_unlock(>ivlock);
+   }
 }
 EXPORT_SYMBOL_GPL(af_alg_put_iv);
 
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 4c425effc5a5..619147792cc9 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -565,6 

[PATCH v3 0/4] crypto: AF_ALG AIO improvements

[Stephan Müller]

Hi,

Herbert, the patch 1 is meant for stable. However, this patch as is
only applies to the new AF_ALG interface implementation. Though,
the issue goes back to the first implementation of AIO support.
Shall I try prepare a patch for the old AF_ALG implementation
as well?

Changes from v2:

* rename ALG_IIV flags into ALG_IV_...

* rename CRYPTO_TFM_REQ_PARALLEL into CRYPTO_TFM_REQ_IV_SERIALIZE

* fix branch in patch 4 to add CRYPTO_TFM_REQ_IV_SERIALIZE flag when
  ctx->iiv == ALG_IV_SERIAL_PROCESSING

* fix patch description of patch 4

Stephan Mueller (4):
  crypto: AF_ALG AIO - lock context IV
  crypto: AF_ALG - inline IV support
  crypto: AF_ALG - allow driver to serialize IV access
  crypto: add CRYPTO_TFM_REQ_IV_SERIALIZE flag

 crypto/af_alg.c | 119 +++-
 crypto/algif_aead.c |  86 +---
 crypto/algif_skcipher.c |  38 ++
 include/crypto/if_alg.h |  37 ++
 include/linux/crypto.h  |  16 ++
 include/uapi/linux/if_alg.h |   6 ++-
 6 files changed, 249 insertions(+), 53 deletions(-)

-- 
2.14.3

[PATCH v3 1/4] crypto: AF_ALG AIO - lock context IV

[Stephan Müller]

The kernel crypto API requires the caller to set an IV in the request data
structure. That request data structure shall define one particular cipher
operation. During the cipher operation, the IV is read by the cipher
implementation and eventually the potentially updated IV (e.g. in case of
CBC) is written back to the memory location the request data structure
points to.

AF_ALG allows setting the IV with a sendmsg request, where the IV is stored
in the AF_ALG context that is unique to one particular AF_ALG socket. Note
the analogy: an AF_ALG socket is like a TFM where one recvmsg operation
uses one request with the TFM from the socket.

AF_ALG these days supports AIO operations with multiple IOCBs. I.e. with
one recvmsg call, multiple IOVECs can be specified. Each individual IOCB
(derived from one IOVEC) implies that one request data structure is
created with the data to be processed by the cipher implementation. The
IV that was set with the sendmsg call is registered with the request data
structure before the cipher operation.

In case of an AIO operation, the cipher operation invocation returns
immediately, queuing the request to the hardware. While the AIO request is
processed by the hardware, recvmsg processes the next IOVEC for which
another request is created. Again, the IV buffer from the AF_ALG socket
context is registered with the new request and the cipher operation is
invoked.

You may now see that there is a potential race condition regarding the IV
handling, because there is *no* separate IV buffer for the different
requests. This is nicely demonstrated with libkcapi using the following
command which creates an AIO request with two IOCBs each encrypting one
AES block in CBC mode:

kcapi  -d 2 -x 9  -e -c "cbc(aes)" -k
8d7dd9b0170ce0b5f2f8e1aa768e01e91da8bfc67fd486d081b28254c99eb423 -i
7fbc02ebf5b93322329df9bfccb635af -p 48981da18e4bb9ef7e2e3162d16b1910

When the first AIO request finishes before the 2nd AIO request is
processed, the returned value is:

8b19050f66582cb7f7e4b6c873819b7108afa0eaa7de29bac7d903576b674c32

I.e. two blocks where the IV output from the first request is the IV input
to the 2nd block.

In case the first AIO request is not completed before the 2nd request
commences, the result is two identical AES blocks (i.e. both use the same
IV):

8b19050f66582cb7f7e4b6c873819b718b19050f66582cb7f7e4b6c873819b71

This inconsistent result may even lead to the conclusion that there can be
a memory corruption in the IV buffer if both AIO requests write to the IV
buffer at the same time.

As the AF_ALG interface is used by user space, a mutex provides the
serialization which puts the caller to sleep in case a previous IOCB
processing is not yet finished.

If multiple IOCBs arrive that all are blocked, the mutex' FIFO operation
of processing arriving requests ensures that the blocked IOCBs are
unblocked in the right order.

CC:  #4.14
Fixes: e870456d8e7c8 ("crypto: algif_skcipher - overhaul memory management")
Fixes: d887c52d6ae43 ("crypto: algif_aead - overhaul memory management")
Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 31 +++
 crypto/algif_aead.c | 20 +---
 crypto/algif_skcipher.c | 12 +---
 include/crypto/if_alg.h |  5 +
 4 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 5231f421ad00..e7887621aa44 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1051,6 +1051,8 @@ void af_alg_async_cb(struct crypto_async_request *_req, 
int err)
struct kiocb *iocb = areq->iocb;
unsigned int resultlen;
 
+   af_alg_put_iv(sk);
+
/* Buffer size written by crypto operation. */
resultlen = areq->outlen;
 
@@ -1175,6 +1177,35 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, 
int flags,
 }
 EXPORT_SYMBOL_GPL(af_alg_get_rsgl);
 
+/**
+ * af_alg_get_iv
+ *
+ * @sk [in] AF_ALG socket
+ * @return 0 on success, < 0 on error
+ */
+int af_alg_get_iv(struct sock *sk)
+{
+   struct alg_sock *ask = alg_sk(sk);
+   struct af_alg_ctx *ctx = ask->private;
+
+   return mutex_lock_interruptible(>ivlock);
+}
+EXPORT_SYMBOL_GPL(af_alg_get_iv);
+
+/**
+ * af_alg_put_iv - release lock on IV in case CTX IV is used
+ *
+ * @sk [in] AF_ALG socket
+ */
+void af_alg_put_iv(struct sock *sk)
+{
+   struct alg_sock *ask = alg_sk(sk);
+   struct af_alg_ctx *ctx = ask->private;
+
+   mutex_unlock(>ivlock);
+}
+EXPORT_SYMBOL_GPL(af_alg_put_iv);
+
 static int __init af_alg_init(void)
 {
int err = proto_register(_proto, 0);
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 4b07edd5a9ff..402de50d4a58 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -159,10 +159,14 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
if (IS_ERR(areq))
return PTR_ERR(areq);
 
+   err = af_alg_get_iv(sk);
+   if (err)
+

[PATCH v3 2/4] crypto: AF_ALG - inline IV support

[Stephan Müller]

The kernel crypto API requires the caller to set an IV in the request
data structure. That request data structure shall define one particular
cipher operation. During the cipher operation, the IV is read by the
cipher implementation and eventually the potentially updated IV (e.g.
in case of CBC) is written back to the memory location the request data
structure points to.

AF_ALG allows setting the IV with a sendmsg request, where the IV is
stored in the AF_ALG context that is unique to one particular AF_ALG
socket. Note the analogy: an AF_ALG socket is like a TFM where one
recvmsg operation uses one request with the TFM from the socket.

AF_ALG these days supports AIO operations with multiple IOCBs. I.e.
with one recvmsg call, multiple IOVECs can be specified. Each
individual IOCB (derived from one IOVEC) implies that one request data
structure is created with the data to be processed by the cipher
implementation. The IV that was set with the sendmsg call is registered
with the request data structure before the cipher operation.

As of now, the individual IOCBs are serialized with respect to the IV
handling. This implies that the kernel does not perform a truly parallel
invocation of the cipher implementations. However, if the user wants to
perform cryptographic operations on multiple IOCBs where each IOCB is
truly independent from the other, parallel invocations are possible.
This would require that each IOCB provides its own IV to ensure true
separation of the IOCBs.

The solution is to allow providing the IV data supplied as part of the
plaintext/ciphertext. To do so, the AF_ALG interface treats the
ALG_SET_OP flag usable with sendmsg as a bit-array allowing to set the
cipher operation together with the flag whether the operation should
enable support for inline IV handling.

If inline IV handling is enabled, the IV is expected to be the first
part of the input plaintext/ciphertext. This IV is only used for one
cipher operation and will not retained in the kernel for subsequent
cipher operations.

The inline IV handling support is only allowed to be enabled during
the first sendmsg call for a context. Any subsequent sendmsg calls are
not allowed to change the setting of the inline IV handling (either
enable or disable it) as this would open up a race condition with the
locking and unlocking of the ctx->ivlock mutex.

The AEAD support required a slight re-arragning of the code, because
obtaining the IV implies that ctx->used is updated. Thus, the ctx->used
access in _aead_recvmsg must be moved below the IV gathering.

The AEAD code to find the first SG with data in the TX SGL is moved to a
common function as it is required by the IV gathering function as well.

This patch does not change the existing interface where user space is
allowed to provide an IV via sendmsg. It only extends the interface by
giving the user the choice to provide the IV either via sendmsg (the
current approach) or as part of the data (the additional approach).

Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 93 ++---
 crypto/algif_aead.c | 58 
 crypto/algif_skcipher.c | 12 +++---
 include/crypto/if_alg.h | 21 +-
 include/uapi/linux/if_alg.h |  6 ++-
 5 files changed, 143 insertions(+), 47 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index e7887621aa44..7f80dcfc12a6 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -14,6 +14,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -834,6 +835,7 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, 
size_t size,
struct af_alg_control con = {};
long copied = 0;
bool enc = 0;
+   int iiv = ALG_IV_SERIAL_PROCESSING;
bool init = 0;
int err = 0;
 
@@ -843,7 +845,7 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, 
size_t size,
return err;
 
init = 1;
-   switch (con.op) {
+   switch (con.op & ALG_OP_CIPHER_MASK) {
case ALG_OP_ENCRYPT:
enc = 1;
break;
@@ -854,6 +856,9 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, 
size_t size,
return -EINVAL;
}
 
+   if (con.op & ALG_OP_INLINE_IV)
+   iiv = ALG_IV_PARALLEL_PROCESSING;
+
if (con.iv && con.iv->ivlen != ivsize)
return -EINVAL;
}
@@ -866,6 +871,19 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr 
*msg, size_t size,
 
if (init) {
ctx->enc = enc;
+
+   /*
+* IIV can only be enabled once with the first sendmsg call.
+* This prevents a race in locking and unlocking the
+* ctx->ivlock mutex.
+*/
+   if (ctx->iiv == 

[PATCH v3 4/4] crypto: add CRYPTO_TFM_REQ_IV_SERIALIZE flag

[Stephan Müller]

Crypto drivers may implement a streamlined serialization support for AIO
requests that is reported by the CRYPTO_ALG_SERIALIZES_IV_ACCESS flag to
the crypto user. When the user decides that he wants to send multiple
AIO requests concurrently and wants the crypto driver to handle the
serialization, the caller has to set CRYPTO_TFM_REQ_IV_SERIALIZE to notify
the crypto driver.

Only when this flag is enabled, the crypto driver shall apply its
serialization logic for handling IV updates between requests. If this
flag is not provided, the serialization logic shall not be applied by
the driver as the caller decides that it does not need it (because no
parallel AIO requests are sent) or that it performs its own
serialization.

Signed-off-by: Stephan Mueller 
---
 crypto/algif_aead.c | 15 ---
 crypto/algif_skcipher.c | 15 ---
 include/linux/crypto.h  |  1 +
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 619147792cc9..5ec4dec6e6a1 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -66,13 +66,22 @@ static int aead_sendmsg(struct socket *sock, struct msghdr 
*msg, size_t size)
 {
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
+   struct af_alg_ctx *ctx = ask->private;
struct sock *psk = ask->parent;
struct alg_sock *pask = alg_sk(psk);
struct aead_tfm *aeadc = pask->private;
-   struct crypto_aead *tfm = aeadc->aead;
-   unsigned int ivsize = crypto_aead_ivsize(tfm);
+   struct crypto_aead *aead = aeadc->aead;
+   struct crypto_tfm *tfm = crypto_aead_tfm(aead);
+   unsigned int ivsize = crypto_aead_ivsize(aead);
+   int ret = af_alg_sendmsg(sock, msg, size, ivsize);
+
+   if (ret < 0)
+   return ret;
 
-   return af_alg_sendmsg(sock, msg, size, ivsize);
+   if (ctx->iiv == ALG_IV_SERIAL_PROCESSING)
+   tfm->crt_flags |= CRYPTO_TFM_REQ_IV_SERIALIZE;
+
+   return ret;
 }
 
 static int crypto_aead_copy_sgl(struct crypto_skcipher *null_tfm,
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index cf27dda6a181..fd2a0ba32feb 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -43,12 +43,21 @@ static int skcipher_sendmsg(struct socket *sock, struct 
msghdr *msg,
 {
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
+   struct af_alg_ctx *ctx = ask->private;
struct sock *psk = ask->parent;
struct alg_sock *pask = alg_sk(psk);
-   struct crypto_skcipher *tfm = pask->private;
-   unsigned ivsize = crypto_skcipher_ivsize(tfm);
+   struct crypto_skcipher *skc = pask->private;
+   struct crypto_tfm *tfm = crypto_skcipher_tfm(skc);
+   unsigned int ivsize = crypto_skcipher_ivsize(skc);
+   int ret = af_alg_sendmsg(sock, msg, size, ivsize);
+
+   if (ret < 0)
+   return ret;
 
-   return af_alg_sendmsg(sock, msg, size, ivsize);
+   if (ctx->iiv == ALG_IV_SERIAL_PROCESSING)
+   tfm->crt_flags |= CRYPTO_TFM_REQ_IV_SERIALIZE;
+
+   return ret;
 }
 
 static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 4860aa2c9be4..4d54f2b30692 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -133,6 +133,7 @@
 #define CRYPTO_TFM_REQ_WEAK_KEY0x0100
 #define CRYPTO_TFM_REQ_MAY_SLEEP   0x0200
 #define CRYPTO_TFM_REQ_MAY_BACKLOG 0x0400
+#define CRYPTO_TFM_REQ_IV_SERIALIZE0x0800
 #define CRYPTO_TFM_RES_WEAK_KEY0x0010
 #define CRYPTO_TFM_RES_BAD_KEY_LEN 0x0020
 #define CRYPTO_TFM_RES_BAD_KEY_SCHED   0x0040
-- 
2.14.3

Re: [PATCH v2 2/4] crypto: AF_ALG - inline IV support

[Stephan Müller]

Am Mittwoch, 7. Februar 2018, 14:54:08 CET schrieb Jonathan Cameron:

Hi Jonathan,

> 
> It's not a 'bug' as such, but this does currently break the crypto-perf
> test for aio and iiv in the libkcapi branch.  We can perhaps make it
> more forgiving...

I just pushed an update for the IIV branch of libkcapi that handles it 
appropriately.
> 
> I suggest we let noop cases where we set IIV on when it is already on
> to not result in an error but to just be ignored.

I do not like the kernel to behave differently on one and the same flag. 
Though, the check could be more forgiving indeed: if the flag from user space 
is set again and identical to what we already have, we would not report an 
error. Only if it is different from what was already set, we report the error.

> > 
> > +
> > +   /*
> > +* IIV can only be enabled once with the first sendmsg call.
> > +* This prevents a race in locking and unlocking the
> > +* ctx->ivlock mutex.
> > +*/
> > +   if (ctx->iiv == ALG_IIV_UNSET) {
> > +   ctx->iiv = iiv;
> > +   } else if (iiv == ALG_IIV_USE) {
> 
> So the issue in kcapi is that we are running a tight loop around a test case
> that does a certain number of aio messages.  At the start of each run of
> that performance loop we reinitialize.
> 
> There is an optmisation in there though which avoids an accept call (which
> would have given a new context) if we have one available already.
> 
> The result is that we are reusing the context which already has this
> set.
> 
> So, can we be a little less restrictive and let things through
> if and only iff iiv = ctx->iiv - i.e. the set is a noop?

Precisely, will do in the next version of the patch set.

Ciao
Stephan

[PATCH v2 3/4] crypto: AF_ALG - allow driver to serialize IV access

[Stephan Müller]

The mutex in AF_ALG to serialize access to the IV ensures full
serialization of requests sent to the crypto driver.

However, the hardware may implement serialization to the IV such that
preparation work without touching the IV can already happen while the IV
is processed by another operation. This may speed up the AIO processing.

The following ASCII art demonstrates this.

AF_ALG mutex handling implements the following logic:

[REQUEST 1 from userspace]   [REQUEST 2 from userspace]
 ||
 [AF_ALG/SOCKET]   [AF_ALG/SOCKET]
 ||
NOTHING BLOCKING (lock mutex) |
 | Queued on Mutex
 [BUILD / MAP HW DESCS]   |
 ||
   [Place in HW Queue]|
 ||
 [Process]|
 ||
[Interrupt]   |
 ||
 [Respond and update IV]  |
 ||
[unlock mutex]   Nothing Blocking (lock mutex)
 ||
Don't care beyond here.   [BUILD / MAP HW DESCS]
  |
  [Place in HW Queue]
  |
  [Process]
  |
 [Interrupt]
  |
[Respond and update IV]
  |
Don't care beyond here.

A crypto driver may implement the following serialization:

[REQUEST 1 from userspace]   [REQUEST 2 from userspace]
 ||
  [AF_ALG/SOCKET]  [AF_ALG/SOCKET]
 ||
  [BUILD / MAP HW DESCS] [BUILD / MAP HW DESCS]
 ||
NOTHING BLOCKING IV in flight (enqueue sw queue)
 ||
   [Place in HW Queue]|
 ||
 [Process]|
 ||
[Interrupt]   |
 ||
 [Respond and update IV] Nothing Blocking (dequeue sw queue)
 ||
  Don't care beyond here.  [Place in HW Queue]
  |
  [Process]
  |
 [Interrupt]
  |
[Respond and update IV]
  |
Don't care beyond here.

If the driver implements its own serialization (i.e. AF_ALG does not
serialize the access to the IV), the crypto implementation must set the
flag CRYPTO_ALG_SERIALIZES_IV_ACCESS.

Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 13 -
 crypto/algif_aead.c |  1 +
 crypto/algif_skcipher.c |  1 +
 include/crypto/if_alg.h | 13 +
 include/linux/crypto.h  | 15 +++
 5 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 973233000d18..8e634163b9ba 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1247,8 +1247,10 @@ int af_alg_get_iv(struct sock *sk, struct 
af_alg_async_req *areq)
return 0;
 
/* No inline IV, use ctx IV buffer and lock it */
-   if (ctx->iiv == ALG_IIV_DISABLE)
-   return mutex_lock_interruptible(>ivlock);
+   if (ctx->iiv == ALG_IIV_DISABLE) {
+   return (ctx->lock_iv) ?
+   mutex_lock_interruptible(>ivlock) : 0;
+   }
 
/* Inline IV handling: There must be the IV data present. */
if (ctx->used < ctx->ivlen || list_empty(>tsgl_list))
@@ -1280,12 +1282,13 @@ void af_alg_put_iv(struct sock *sk)
struct alg_sock *ask = alg_sk(sk);
struct af_alg_ctx *ctx = ask->private;
 
-   /* To resemble af_alg_get_iv, do not merge the two branches. */
if (!ctx->ivlen)
return;
 
-   if (ctx->iiv == ALG_IIV_DISABLE)
-   mutex_unlock(>ivlock);
+   if (ctx->iiv == ALG_IIV_DISABLE) {
+   if (ctx->lock_iv)
+   mutex_unlock(>ivlock);
+   }
 }
 EXPORT_SYMBOL_GPL(af_alg_put_iv);
 
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 623a0fc2b535..3970ad7f6fd0 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -565,6 +565,7 @@ static int 

[PATCH v2 0/4] crypto: AF_ALG AIO improvements

[Stephan Müller]

Hi Herbert,

Herbert, the patch 1 is meant for stable. However, this patch as is
only applies to the new AF_ALG interface implementation. Though,
the issue goes back to the first implementation of AIO support.
Shall I try prepare a patch for the old AF_ALG implementation
as well?

Changes from v1:

* integrate the inline IV and locking patch into one patch set

* reverse the order of lock context IV patch and inline IV patch --
  the reason is to allow the first patch to be back-ported to stable

* mark the first patch (locking of the context IV) as applicable to
  stable as there is an existing inconsistency which was demonstrated
  by Harsh with the Chelsio driver vs the AES-NI driver

* modify the inline IV patch to have proper unlocking of the mutex
  in case of errors

* prevent locking if no IV is defined by cipher

* add a patch to allow crypto drivers to report whether they support
  serialization -- in this case the locking in AF_ALG shall be
  disabled

* add a patch to inform the crypto drivers that their serialization
  support should actually be enabled and used because AF_ALG does not
  serialize the interdependent parallel AIO requests

* streamline the code in patch 1 and 2 slightly

I would like to ask the folks with real AIO hardware (Harsh, Jonathan)
to test the patches. Especially, is the locking patch should be tested
by Harsh as you have seen the issue with your hardware.

Thanks.

Stephan Mueller (4):
  crypto: AF_ALG AIO - lock context IV
  crypto: AF_ALG - inline IV support
  crypto: AF_ALG - allow driver to serialize IV access
  crypto: add CRYPTO_TFM_REQ_PARALLEL flag

 crypto/af_alg.c | 119 +++-
 crypto/algif_aead.c |  86 +---
 crypto/algif_skcipher.c |  38 ++
 include/crypto/if_alg.h |  37 ++
 include/linux/crypto.h  |  16 ++
 include/uapi/linux/if_alg.h |   6 ++-
 6 files changed, 249 insertions(+), 53 deletions(-)

-- 
2.14.3

[PATCH v2 4/4] crypto: add CRYPTO_TFM_REQ_PARALLEL flag

[Stephan Müller]

Crypto drivers may implement a streamlined serialization support for AIO
requests that is reported by the CRYPTO_TFM_REQ_PARALLEL flag to the
crypto user. When the user decides that he wants to send multiple AIO
requests concurrently and wants the crypto driver to handle the
serialization, the caller has to set CRYPTO_TFM_REQ_PARALLEL to notify
the crypto driver.

Only when this flag is enabled, the crypto driver shall apply its
serialization logic for handling IV updates between requests. If this
flag is not provided, the serialization logic shall not be applied by
the driver as the caller decides that it does not need it (because no
parallel AIO requests are sent) or that it performs its own
serialization.

Signed-off-by: Stephan Mueller 
---
 crypto/algif_aead.c | 15 ---
 crypto/algif_skcipher.c | 15 ---
 include/linux/crypto.h  |  1 +
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 3970ad7f6fd0..da010405eea0 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -66,13 +66,22 @@ static int aead_sendmsg(struct socket *sock, struct msghdr 
*msg, size_t size)
 {
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
+   struct af_alg_ctx *ctx = ask->private;
struct sock *psk = ask->parent;
struct alg_sock *pask = alg_sk(psk);
struct aead_tfm *aeadc = pask->private;
-   struct crypto_aead *tfm = aeadc->aead;
-   unsigned int ivsize = crypto_aead_ivsize(tfm);
+   struct crypto_aead *aead = aeadc->aead;
+   struct crypto_tfm *tfm = crypto_aead_tfm(aead);
+   unsigned int ivsize = crypto_aead_ivsize(aead);
+   int ret = af_alg_sendmsg(sock, msg, size, ivsize);
+
+   if (ret < 0)
+   return ret;
 
-   return af_alg_sendmsg(sock, msg, size, ivsize);
+   if (ctx->iiv == ALG_IIV_USE)
+   tfm->crt_flags |= CRYPTO_TFM_REQ_PARALLEL;
+
+   return ret;
 }
 
 static int crypto_aead_copy_sgl(struct crypto_skcipher *null_tfm,
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index aee602a3ec24..22554cf7 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -43,12 +43,21 @@ static int skcipher_sendmsg(struct socket *sock, struct 
msghdr *msg,
 {
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
+   struct af_alg_ctx *ctx = ask->private;
struct sock *psk = ask->parent;
struct alg_sock *pask = alg_sk(psk);
-   struct crypto_skcipher *tfm = pask->private;
-   unsigned ivsize = crypto_skcipher_ivsize(tfm);
+   struct crypto_skcipher *skc = pask->private;
+   struct crypto_tfm *tfm = crypto_skcipher_tfm(skc);
+   unsigned int ivsize = crypto_skcipher_ivsize(skc);
+   int ret = af_alg_sendmsg(sock, msg, size, ivsize);
+
+   if (ret < 0)
+   return ret;
 
-   return af_alg_sendmsg(sock, msg, size, ivsize);
+   if (ctx->iiv == ALG_IIV_USE)
+   tfm->crt_flags |= CRYPTO_TFM_REQ_PARALLEL;
+
+   return ret;
 }
 
 static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 4860aa2c9be4..95c9d6e8fb70 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -133,6 +133,7 @@
 #define CRYPTO_TFM_REQ_WEAK_KEY0x0100
 #define CRYPTO_TFM_REQ_MAY_SLEEP   0x0200
 #define CRYPTO_TFM_REQ_MAY_BACKLOG 0x0400
+#define CRYPTO_TFM_REQ_PARALLEL0x0800
 #define CRYPTO_TFM_RES_WEAK_KEY0x0010
 #define CRYPTO_TFM_RES_BAD_KEY_LEN 0x0020
 #define CRYPTO_TFM_RES_BAD_KEY_SCHED   0x0040
-- 
2.14.3

[PATCH v2 1/4] crypto: AF_ALG AIO - lock context IV

[Stephan Müller]

The kernel crypto API requires the caller to set an IV in the request data
structure. That request data structure shall define one particular cipher
operation. During the cipher operation, the IV is read by the cipher
implementation and eventually the potentially updated IV (e.g. in case of
CBC) is written back to the memory location the request data structure
points to.

AF_ALG allows setting the IV with a sendmsg request, where the IV is stored
in the AF_ALG context that is unique to one particular AF_ALG socket. Note
the analogy: an AF_ALG socket is like a TFM where one recvmsg operation
uses one request with the TFM from the socket.

AF_ALG these days supports AIO operations with multiple IOCBs. I.e. with
one recvmsg call, multiple IOVECs can be specified. Each individual IOCB
(derived from one IOVEC) implies that one request data structure is
created with the data to be processed by the cipher implementation. The
IV that was set with the sendmsg call is registered with the request data
structure before the cipher operation.

In case of an AIO operation, the cipher operation invocation returns
immediately, queuing the request to the hardware. While the AIO request is
processed by the hardware, recvmsg processes the next IOVEC for which
another request is created. Again, the IV buffer from the AF_ALG socket
context is registered with the new request and the cipher operation is
invoked.

You may now see that there is a potential race condition regarding the IV
handling, because there is *no* separate IV buffer for the different
requests. This is nicely demonstrated with libkcapi using the following
command which creates an AIO request with two IOCBs each encrypting one
AES block in CBC mode:

kcapi  -d 2 -x 9  -e -c "cbc(aes)" -k
8d7dd9b0170ce0b5f2f8e1aa768e01e91da8bfc67fd486d081b28254c99eb423 -i
7fbc02ebf5b93322329df9bfccb635af -p 48981da18e4bb9ef7e2e3162d16b1910

When the first AIO request finishes before the 2nd AIO request is
processed, the returned value is:

8b19050f66582cb7f7e4b6c873819b7108afa0eaa7de29bac7d903576b674c32

I.e. two blocks where the IV output from the first request is the IV input
to the 2nd block.

In case the first AIO request is not completed before the 2nd request
commences, the result is two identical AES blocks (i.e. both use the same
IV):

8b19050f66582cb7f7e4b6c873819b718b19050f66582cb7f7e4b6c873819b71

This inconsistent result may even lead to the conclusion that there can be
a memory corruption in the IV buffer if both AIO requests write to the IV
buffer at the same time.

As the AF_ALG interface is used by user space, a mutex provides the
serialization which puts the caller to sleep in case a previous IOCB
processing is not yet finished.

If multiple IOCBs arrive that all are blocked, the mutex' FIFO operation
of processing arriving requests ensures that the blocked IOCBs are
unblocked in the right order.

CC:  #4.14
Fixes: e870456d8e7c8 ("crypto: algif_skcipher - overhaul memory management")
Fixes: d887c52d6ae43 ("crypto: algif_aead - overhaul memory management")
Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 31 +++
 crypto/algif_aead.c | 20 +---
 crypto/algif_skcipher.c | 12 +---
 include/crypto/if_alg.h |  5 +
 4 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 5231f421ad00..e7887621aa44 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1051,6 +1051,8 @@ void af_alg_async_cb(struct crypto_async_request *_req, 
int err)
struct kiocb *iocb = areq->iocb;
unsigned int resultlen;
 
+   af_alg_put_iv(sk);
+
/* Buffer size written by crypto operation. */
resultlen = areq->outlen;
 
@@ -1175,6 +1177,35 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, 
int flags,
 }
 EXPORT_SYMBOL_GPL(af_alg_get_rsgl);
 
+/**
+ * af_alg_get_iv
+ *
+ * @sk [in] AF_ALG socket
+ * @return 0 on success, < 0 on error
+ */
+int af_alg_get_iv(struct sock *sk)
+{
+   struct alg_sock *ask = alg_sk(sk);
+   struct af_alg_ctx *ctx = ask->private;
+
+   return mutex_lock_interruptible(>ivlock);
+}
+EXPORT_SYMBOL_GPL(af_alg_get_iv);
+
+/**
+ * af_alg_put_iv - release lock on IV in case CTX IV is used
+ *
+ * @sk [in] AF_ALG socket
+ */
+void af_alg_put_iv(struct sock *sk)
+{
+   struct alg_sock *ask = alg_sk(sk);
+   struct af_alg_ctx *ctx = ask->private;
+
+   mutex_unlock(>ivlock);
+}
+EXPORT_SYMBOL_GPL(af_alg_put_iv);
+
 static int __init af_alg_init(void)
 {
int err = proto_register(_proto, 0);
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 4b07edd5a9ff..402de50d4a58 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -159,10 +159,14 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
if (IS_ERR(areq))
return PTR_ERR(areq);
 
+   err = af_alg_get_iv(sk);
+   if (err)
+

[PATCH v2 2/4] crypto: AF_ALG - inline IV support

[Stephan Müller]

The kernel crypto API requires the caller to set an IV in the request
data structure. That request data structure shall define one particular
cipher operation. During the cipher operation, the IV is read by the
cipher implementation and eventually the potentially updated IV (e.g.
in case of CBC) is written back to the memory location the request data
structure points to.

AF_ALG allows setting the IV with a sendmsg request, where the IV is
stored in the AF_ALG context that is unique to one particular AF_ALG
socket. Note the analogy: an AF_ALG socket is like a TFM where one
recvmsg operation uses one request with the TFM from the socket.

AF_ALG these days supports AIO operations with multiple IOCBs. I.e.
with one recvmsg call, multiple IOVECs can be specified. Each
individual IOCB (derived from one IOVEC) implies that one request data
structure is created with the data to be processed by the cipher
implementation. The IV that was set with the sendmsg call is registered
with the request data structure before the cipher operation.

As of now, the individual IOCBs are serialized with respect to the IV
handling. This implies that the kernel does not perform a truly parallel
invocation of the cipher implementations. However, if the user wants to
perform cryptographic operations on multiple IOCBs where each IOCB is
truly independent from the other, parallel invocations are possible.
This would require that each IOCB provides its own IV to ensure true
separation of the IOCBs.

The solution is to allow providing the IV data supplied as part of the
plaintext/ciphertext. To do so, the AF_ALG interface treats the
ALG_SET_OP flag usable with sendmsg as a bit-array allowing to set the
cipher operation together with the flag whether the operation should
enable support for inline IV handling.

If inline IV handling is enabled, the IV is expected to be the first
part of the input plaintext/ciphertext. This IV is only used for one
cipher operation and will not retained in the kernel for subsequent
cipher operations.

The inline IV handling support is only allowed to be enabled during
the first sendmsg call for a context. Any subsequent sendmsg calls are
not allowed to change the setting of the inline IV handling (either
enable or disable it) as this would open up a race condition with the
locking and unlocking of the ctx->ivlock mutex.

The AEAD support required a slight re-arragning of the code, because
obtaining the IV implies that ctx->used is updated. Thus, the ctx->used
access in _aead_recvmsg must be moved below the IV gathering.

The AEAD code to find the first SG with data in the TX SGL is moved to a
common function as it is required by the IV gathering function as well.

This patch does not change the existing interface where user space is
allowed to provide an IV via sendmsg. It only extends the interface by
giving the user the choice to provide the IV either via sendmsg (the
current approach) or as part of the data (the additional approach).

Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 93 ++---
 crypto/algif_aead.c | 58 
 crypto/algif_skcipher.c | 12 +++---
 include/crypto/if_alg.h | 21 +-
 include/uapi/linux/if_alg.h |  6 ++-
 5 files changed, 143 insertions(+), 47 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index e7887621aa44..973233000d18 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -14,6 +14,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -834,6 +835,7 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, 
size_t size,
struct af_alg_control con = {};
long copied = 0;
bool enc = 0;
+   int iiv = ALG_IIV_DISABLE;
bool init = 0;
int err = 0;
 
@@ -843,7 +845,7 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, 
size_t size,
return err;
 
init = 1;
-   switch (con.op) {
+   switch (con.op & ALG_OP_CIPHER_MASK) {
case ALG_OP_ENCRYPT:
enc = 1;
break;
@@ -854,6 +856,9 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, 
size_t size,
return -EINVAL;
}
 
+   if (con.op & ALG_OP_INLINE_IV)
+   iiv = ALG_IIV_USE;
+
if (con.iv && con.iv->ivlen != ivsize)
return -EINVAL;
}
@@ -866,6 +871,19 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr 
*msg, size_t size,
 
if (init) {
ctx->enc = enc;
+
+   /*
+* IIV can only be enabled once with the first sendmsg call.
+* This prevents a race in locking and unlocking the
+* ctx->ivlock mutex.
+*/
+   if (ctx->iiv == ALG_IIV_UNSET) {
+   

Re: [RFC PATCH 2/3] crypto: hisilicon hacv1 driver

[Stephan Müller]

Am Dienstag, 30. Januar 2018, 16:29:52 CET schrieb Jonathan Cameron:

Hi Jonathan,

> + /* Special path for single element SGLs with small packets. */
> + if (sg_is_last(sgl) && sgl->length <= SEC_SMALL_PACKET_SIZE) {

This looks strangely familiar. Is this code affected by a similar issue fixed 
in https://patchwork.kernel.org/patch/10173981?

> +static int sec_alg_skcipher_setkey(struct crypto_skcipher *tfm,
> +  const u8 *key, unsigned int keylen,
> +  enum sec_cipher_alg alg)
> +{
> +   struct sec_alg_skcipher_ctx *ctx = crypto_skcipher_ctx(tfm);
> +   struct device *dev;
> +
> +   spin_lock(>lock);
> +   if (ctx->enc_key) {
> +   /* rekeying */
> +   dev = SEC_Q_DEV(ctx->queue);
> +   memset(ctx->enc_key, 0, SEC_MAX_CIPHER_KEY);
> +   memset(ctx->dec_key, 0, SEC_MAX_CIPHER_KEY);
> +   memset(>enc_req, 0, sizeof(ctx->enc_req));
> +   memset(>dec_req, 0, sizeof(ctx->dec_req));
> +   } else {
> +   /* new key */
> +   dev = SEC_Q_DEV(ctx->queue);
> +   ctx->enc_key = dma_zalloc_coherent(dev, SEC_MAX_CIPHER_KEY,
> +  >enc_pkey,
> GFP_ATOMIC); +   if (!ctx->enc_key) {
> +   spin_unlock(>lock);
> +   return -ENOMEM;
> +   }
> +   ctx->dec_key = dma_zalloc_coherent(dev, SEC_MAX_CIPHER_KEY,
> +  >dec_pkey,
> GFP_ATOMIC); +   if (!ctx->dec_key) {
> +   spin_unlock(>lock);
> +   goto out_free_enc;
> +   }
> +   }
> +   spin_unlock(>lock);
> +   if (sec_alg_skcipher_init_context(tfm, key, keylen, alg))
> +   goto out_free_all;
> +
> +   return 0;
> +
> +out_free_all:
> +   memset(ctx->dec_key, 0, SEC_MAX_CIPHER_KEY);
> +   dma_free_coherent(dev, SEC_MAX_CIPHER_KEY,
> + ctx->dec_key, ctx->dec_pkey);
> +   ctx->dec_key = NULL;
> +out_free_enc:
> +   memset(ctx->enc_key, 0, SEC_MAX_CIPHER_KEY);
> +   dma_free_coherent(dev, SEC_MAX_CIPHER_KEY,
> + ctx->enc_key, ctx->enc_pkey);
> +   ctx->enc_key = NULL;

Please use memzero_explicit.
> +
> +   return -ENOMEM;
> +}

> +static int sec_alg_skcipher_setkey_aes_xts(struct crypto_skcipher *tfm,
> +  const u8 *key, unsigned int
> keylen) +{
> +   enum sec_cipher_alg alg;
> +
> +   switch (keylen) {
> +   case AES_KEYSIZE_128 * 2:
> +   alg = SEC_AES_XTS_128;
> +   break;
> +   case AES_KEYSIZE_256 * 2:
> +   alg = SEC_AES_XTS_256;
> +   break;
> +   default:
> +   return -EINVAL;
> +   }

Please add xts_check_key()
> +
> +   return sec_alg_skcipher_setkey(tfm, key, keylen, alg);

> +static int sec_alg_skcipher_setkey_3des_ecb(struct crypto_skcipher *tfm,
> +   const u8 *key, unsigned int
> keylen) +{
> +   if (keylen != DES_KEY_SIZE * 3)
> +   return -EINVAL;
> +
> +   return sec_alg_skcipher_setkey(tfm, key, keylen,
> SEC_3DES_ECB_192_3KEY); +}
> +
> +static int sec_alg_skcipher_setkey_3des_cbc(struct crypto_skcipher *tfm,
> +   const u8 *key, unsigned int
> keylen) +{
> +   if (keylen != DES3_EDE_KEY_SIZE)
> +   return -EINVAL;
> +
> +   return sec_alg_skcipher_setkey(tfm, key, keylen,
> SEC_3DES_CBC_192_3KEY); +}

Please use __des3_ede_setkey

> +static void sec_skcipher_alg_callback(struct sec_bd_info *sec_resp,
> +   struct skcipher_request *skreq)
> +{
> + struct sec_crypto_request *sec_req = skcipher_request_ctx(skreq);
> + struct sec_alg_skcipher_ctx *ctx = sec_req->skcipher_ctx;
> + struct crypto_skcipher *atfm = crypto_skcipher_reqtfm(skreq);
> + struct crypto_async_request *nextrequest;
> + struct sec_crypto_request *nextsec_req;
> + struct skcipher_request *nextskreq;
> + int icv_or_skey_en;
> + int err = 0;
> +
> + icv_or_skey_en = (sec_resp->w0 & SEC_BD_W0_ICV_OR_SKEY_EN_M) >>
> + SEC_BD_W0_ICV_OR_SKEY_EN_S;
> + if (sec_resp->w1 & SEC_BD_W1_BD_INVALID || icv_or_skey_en == 3) {
> + dev_err(ctx->queue->dev_info->dev,
> + "Got an invalid answer %lu %d\n",
> + sec_resp->w1 & SEC_BD_W1_BD_INVALID,
> + icv_or_skey_en);
> + err = -EINVAL;
> + /*
> +  * We need to muddle on to avoid getting stuck with elements
> +  * on the queue. Error will be reported so userspace should
> +  * know a mess has occurred.
> +  */
> + }
> +
> + spin_lock(>queue->queuelock);
> + 

Re: Subject: [PATCH] crypto: add zbewalgo compression algorithm for zram

[Stephan Müller]

Am Dienstag, 30. Januar 2018, 20:49:00 CET schrieb Benjamin Warnke:

Hi Benjamin,

> > In general: I do not think that having larger C functions in header files
> > is a proper coding style.
> 
> How should I solve this?
> 
> Option 1:
> Move everything in the lib/zbewalgo folder into a single source file.
> This way there is no function defined in a header file.
> I separated the code into different files because the different partial
> compression algorithms are independent from each other.
> 
> Option 2:
> Move each header file in its own *.c file, while keeping the
> function-declarations in the header. If the code is scattered in multiple
> source files each of the partial algorithms would show up as an independent
> module. All these modules would load simultaneously with the module
> zbewalgo_compress The module zbewalgo_compress requires an array of all
> partial algorithms. This would spam the 'lsmod' list with unneccessary
> details.

A module may be compiled from multiple C files. So, moving the code into 
separate C files and link them into one object / KO should be considered.

> 
> > Why are there __attribute__ ((unused)) function parameters, such as in
> > compress_bitshuffle and others?
> 
> The zbewalgo algorithm is a container-algorithm for compression functions
> with the ability to concatenate multiple algorithms. To be able to call any
> compression algorithm the same way, I defined 'struct zbewalgo_alg' as the
> interface for all those independent compression algorithms. Some of the
> partial algorithms do not require all parameters. To silence compiler
> warnings (if additional warning flags are enabled) I explicitly add the
> 'unused'-parameter

Linux does not enable the compiler warning about unused parameters.

Ciao
Stephan

[PATCH] crypto: AF_ALG AIO - lock context IV

[Stephan Müller]

Hi Harsh,

may I as you to try the attached patch on your Chelsio driver? Please invoke 
the following command from libkcapi:

kcapi  -d 2 -x 9  -e -c "cbc(aes)" -k 
8d7dd9b0170ce0b5f2f8e1aa768e01e91da8bfc67fd486d081b28254c99eb423 -i 
7fbc02ebf5b93322329df9bfccb635af -p 48981da18e4bb9ef7e2e3162d16b1910

The result should now be a fully block-chained result.

Note, the patch goes on top of the inline IV patch.

Thanks

---8<---

In case of AIO where multiple IOCBs are sent to the kernel and inline IV
is not used, the ctx->iv is used for each IOCB. The ctx->iv is read for
the crypto operation and written back after the crypto operation. Thus,
processing of multiple IOCBs must be serialized.

As the AF_ALG interface is used by user space, a mutex provides the
serialization which puts the caller to sleep in case a previous IOCB
processing is not yet finished.

If multiple IOCBs arrive that all are blocked, the mutex' FIFO operation
of processing arriving requests ensures that the blocked IOCBs are
unblocked in the right order.

Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 22 ++
 crypto/algif_aead.c |  2 ++
 crypto/algif_skcipher.c |  2 ++
 include/crypto/if_alg.h |  3 +++
 4 files changed, 29 insertions(+)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 87394dc90ac0..3007c9d899da 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1059,6 +1059,8 @@ void af_alg_async_cb(struct crypto_async_request *_req, 
int err)
struct kiocb *iocb = areq->iocb;
unsigned int resultlen;
 
+   af_alg_put_iv(sk);
+
/* Buffer size written by crypto operation. */
resultlen = areq->outlen;
 
@@ -1227,6 +1229,11 @@ int af_alg_get_iv(struct sock *sk, struct 
af_alg_async_req *areq)
 
/* No inline IV or cipher has no IV, use ctx IV buffer. */
if (!ctx->iiv || !ctx->ivlen) {
+   int ret = mutex_lock_interruptible(>ivlock);
+
+   if (ret)
+   return ret;
+
areq->iv = ctx->iv;
areq->ivlen = 0;// areq->iv will not be freed
return 0;
@@ -1252,6 +1259,21 @@ int af_alg_get_iv(struct sock *sk, struct 
af_alg_async_req *areq)
 }
 EXPORT_SYMBOL_GPL(af_alg_get_iv);
 
+/**
+ * af_alg_put_iv - release lock on IV in case CTX IV is used
+ *
+ * @sk [in] AF_ALG socket
+ */
+void af_alg_put_iv(struct sock *sk)
+{
+   struct alg_sock *ask = alg_sk(sk);
+   struct af_alg_ctx *ctx = ask->private;
+
+   if (!ctx->iiv || !ctx->ivlen)
+   mutex_unlock(>ivlock);
+}
+EXPORT_SYMBOL_GPL(af_alg_put_iv);
+
 static int __init af_alg_init(void)
 {
int err = proto_register(_proto, 0);
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 7eb7cb132c09..165b2ca82e51 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -309,6 +309,7 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
>wait);
}
 
+   af_alg_put_iv(sk);
 
 free:
af_alg_free_resources(areq);
@@ -555,6 +556,7 @@ static int aead_accept_parent_nokey(void *private, struct 
sock *sk)
return -ENOMEM;
}
memset(ctx->iv, 0, ctx->ivlen);
+   mutex_init(>ivlock);
 
INIT_LIST_HEAD(>tsgl_list);
ctx->len = len;
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index d40e1d6797d8..a759cec446b4 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -151,6 +151,7 @@ static int _skcipher_recvmsg(struct socket *sock, struct 
msghdr *msg,
 >wait);
}
+   af_alg_put_iv(sk);
 
 free:
af_alg_free_resources(areq);
@@ -358,6 +359,7 @@ static int skcipher_accept_parent_nokey(void *private, 
struct sock *sk)
}
memset(ctx->iv, 0, ctx->ivlen);
+   mutex_init(>ivlock);
 
INIT_LIST_HEAD(>tsgl_list);
ctx->len = len;
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index ebc651ceb54a..666be8ac683c 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -160,6 +161,7 @@ struct af_alg_ctx {
 
void *iv;
unsigned int ivlen;
+   struct mutex ivlock;
size_t aead_assoclen;
 
struct crypto_wait wait;
@@ -268,6 +270,7 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, 
int flags,
struct af_alg_async_req *areq, size_t maxsize,
size_t *outlen);
 int af_alg_get_iv(struct sock *sk, struct af_alg_async_req *areq);
+void af_alg_put_iv(struct sock *sk);
 struct scatterlist *af_alg_get_tsgl_sg(struct af_alg_ctx *ctx);
 
 #endif /* _CRYPTO_IF_ALG_H */
-- 

Re: [PATCH] crypto: AF_ALG - inline IV support

[Stephan Müller]

Hi Herbert,

I tried to summarize the use cases of the AIO support at [1].

The use case covering the inline IV support is documented in section [2]. It 
naturally would depend on this patch to be accepted. What is your take on this 
use case?

What is your take on the issue outlined at [3]? Should the affected drivers be 
updated?

Thanks
Stephan

[1] https://github.com/smuellerDD/libkcapi/commit/
29132075b8f045f3f92367c0190add81ccf5da11

[2] https://github.com/smuellerDD/libkcapi/commit/
29132075b8f045f3f92367c0190add81ccf5da11#diff-
dce7a00cbc610df94f0dc27eb769d01dR644

[3] https://github.com/smuellerDD/libkcapi/commit/
29132075b8f045f3f92367c0190add81ccf5da11#diff-
dce7a00cbc610df94f0dc27eb769d01dR575

[PATCH v2] crypto: AES-NI GCM - handle zero length dst buffer

[Stephan Müller]

Hi Herbert,

Sorry, I forgot to CC you on this patch -- v2 is unchanged from the
original patch. It only adds you in copy.

---8<---

GCM can be invoked with a zero destination buffer. This is possible if
the AAD and the ciphertext have zero lengths and only the tag exists in
the source buffer (i.e. a source buffer cannot be zero). In this case,
the GCM cipher only performs the authentication and no decryption
operation.

When the destination buffer has zero length, it is possible that no page
is mapped to the SG pointing to the destination. In this case,
sg_page(req->dst) is an invalid access. Therefore, page accesses should
only be allowed if the req->dst->length is non-zero which is the
indicator that a page must exist.

This fixes a crash that can be triggered by user space via AF_ALG.

CC: 
Signed-off-by: Stephan Mueller 
---
 arch/x86/crypto/aesni-intel_glue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/crypto/aesni-intel_glue.c 
b/arch/x86/crypto/aesni-intel_glue.c
index a5ee78d723cd..34cf1c1f8c98 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -824,7 +824,7 @@ static int gcmaes_decrypt(struct aead_request *req, 
unsigned int assoclen,
if (sg_is_last(req->src) &&
(!PageHighMem(sg_page(req->src)) ||
req->src->offset + req->src->length <= PAGE_SIZE) &&
-   sg_is_last(req->dst) &&
+   sg_is_last(req->dst) && req->dst->length &&
(!PageHighMem(sg_page(req->dst)) ||
req->dst->offset + req->dst->length <= PAGE_SIZE)) {
one_entry_in_sg = 1;
-- 
2.14.3

[PATCH] crypto: AES-NI GCM - handle zero length dst buffer

[Stephan Müller]

GCM can be invoked with a zero destination buffer. This is possible if
the AAD and the ciphertext have zero lengths and only the tag exists in
the source buffer (i.e. a source buffer cannot be zero). In this case,
the GCM cipher only performs the authentication and no decryption
operation.

When the destination buffer has zero length, it is possible that no page
is mapped to the SG pointing to the destination. In this case,
sg_page(req->dst) is an invalid access. Therefore, page accesses should
only be allowed if the req->dst->length is non-zero which is the
indicator that a page must exist.

This fixes a crash that can be triggered by user space via AF_ALG.

CC: 
Signed-off-by: Stephan Mueller 
---
 arch/x86/crypto/aesni-intel_glue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/crypto/aesni-intel_glue.c 
b/arch/x86/crypto/aesni-intel_glue.c
index a5ee78d723cd..34cf1c1f8c98 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -824,7 +824,7 @@ static int gcmaes_decrypt(struct aead_request *req, 
unsigned int assoclen,
if (sg_is_last(req->src) &&
(!PageHighMem(sg_page(req->src)) ||
req->src->offset + req->src->length <= PAGE_SIZE) &&
-   sg_is_last(req->dst) &&
+   sg_is_last(req->dst) && req->dst->length &&
(!PageHighMem(sg_page(req->dst)) ||
req->dst->offset + req->dst->length <= PAGE_SIZE)) {
one_entry_in_sg = 1;
-- 
2.14.3

AES-NI: BUG in gcmaes_decrypt

[Stephan Müller]

Hi,

When compiling the current cryptodev-2.6 tree with CONFIG_DEBUG_SG and 
invoking a gcm(aes) decrypt operation with an empty ciphertext and an empty 
AAD, I get the following BUG:

[   79.294243] [ cut here ]
[   79.294903] kernel BUG at ./include/linux/scatterlist.h:130!
[   79.295808] invalid opcode:  [#1] SMP
[   79.296689] Modules linked in: ansi_cprng algif_rng ccm algif_aead 
algif_skcipher crypto_user des3_ede_x86_64 des_generic algif_hash af_alg 
ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 
nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ip_set nfnetlink 
ebtable_nat ebtable_broute bridge stp llc ip6table_mangle ip6table_raw 
ip6table_security iptable_mangle iptable_raw iptable_security ebtable_filter 
ebtables ip6table_filter ip6_tables crct10dif_pclmul crc32_pclmul 
ghash_clmulni_intel pcspkr virtio_net virtio_balloon i2c_piix4 sch_fq_codel 
virtio_blk virtio_console crc32c_intel serio_raw virtio_pci virtio_ring virtio
[   79.304600] CPU: 3 PID: 13182 Comm: lt-kcapi Not tainted 4.15.0-rc3+ #584
[   79.305395] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.10.2-2.fc27 04/01/2014
[   79.306514] RIP: 0010:gcmaes_decrypt.constprop.11+0x29f/0x310
[   79.307259] RSP: 0018:a9ae026c3ca8 EFLAGS: 00010212
[   79.307948] RAX: cc2a41ee1242 RBX: a192b8cbfa00 RCX: 
87654321
[   79.308853] RDX: a192b8c91410 RSI: 5a5e678f RDI: 

[   79.309749] RBP: 0010 R08: a192b8c91c60 R09: 
a192b8cbfa00
[   79.310652] R10: a9ae026c3d70 R11:  R12: 
a192b89a7060
[   79.311552] R13: 0010 R14: a192b8c91798 R15: 
0010
[   79.312446] FS:  7fe6275f8700() GS:a192bfd8() knlGS:

[   79.313643] CS:  0010 DS:  ES:  CR0: 80050033
[   79.314120] CR2: 7ffeb41ee000 CR3: 78244006 CR4: 
003606e0
[   79.315251] Call Trace:
[   79.315515]  ? sock_kfree_s+0x19/0x30
[   79.315845]  ? generic_gcmaes_decrypt+0x50/0x60
[   79.316251]  ? aead_recvmsg+0x5e1/0x670 [algif_aead]
[   79.316704]  ? aead_recvmsg+0x5e1/0x670 [algif_aead]
[   79.317144]  ? sock_read_iter+0x89/0xd0
[   79.317499]  ? __vfs_read+0xd1/0x120
[   79.317834]  ? vfs_read+0x89/0x130
[   79.318149]  ? SyS_read+0x42/0x90
[   79.318619]  ? do_syscall_64+0x5c/0x120
[   79.319501]  ? entry_SYSCALL64_slow_path+0x25/0x25


The BUG is triggered by the sg_page() invocation in gcmaes_decrypt which 
checks:

BUG_ON(sg->sg_magic != SG_MAGIC);


The issue can be triggered with libkcapi using the following test:

kcapi   -x 2   -c "gcm(aes)" -i 0d92aa861746b324f20ee6b7 -k 
f4a6a5e5f2066f6dd9ec6fc5169c29043560ef595c9e81e76f42d29212cc581c -a "" -t 
"5f24c68cbe6f32c29652442bf5d483ad" -q ""

Ciao
Stephan

Re: AF_ALG: skb limits

[Stephan Müller]

Am Dienstag, 12. Dezember 2017, 14:59:21 CET schrieb Jonathan Cameron:

Hi Jonathan,

> On Fri, 8 Dec 2017 13:43:20 +0100
> 
> Stephan Mueller  wrote:
> > Am Freitag, 8. Dezember 2017, 12:39:06 CET schrieb Jonathan Cameron:
> > 
> > Hi Jonathan,
> > 
> > > As a heads up, the other nasties we've found so far are around hitting
> > > limits on the various socket buffers.  When you run into those you can
> > > end
> > > up with parts of the data to be encrypted going through without it being
> > > obvious.
> > 
> > The entire code uses sock_alloc to prevent user space to chew up kernel
> > memory. I am aware that if you have a too low skb buffer limit, parts of
> > the cipher operation will not succeed as a malloc will fail.
> > 
> > But that is returned with an error to user space.
> 
> I 'think' there are some cases where you don't get an error to userspace -
> or at least not directly.
> 
> In af_alg_get_rsgl, we have an af_alg_readable(sk) call which simply breaks
> out of the loop - leaving us with len set to potentially part of the
> required length - without setting a appropriate return value.
> 
> I can't immediately see how this one is reported to userspace.

af_alg_get_rsgl is used to calculate and obtain the length of the buffers 
receiving the result (what is called RX buffers). That length is used for the 
cipher operation and finally reported to user space.

For example (TX is the buffer given to the kernel during sendmsg, RX during 
recvmsg) where the example disregards the alignment to block size:

sendmsg TX: 1000 bytes
recvmsg RX: 100 bytes given by user space, 100 bytes obtained by kernel
=> recvmsg returns 100
recvmsg RX: 1000 bytes given by user space, 900 bytes obtained by kernel
=> recvmsg returns 900

Note, the TX and RX buffer sizes may *not* be identical or even related. User 
space may send one large input block as TX, but "process" it with numerous 
small recvmsg calls. Each recvmsg call defines the cipher operation size. This 
is of particular importance for an AEAD cipher.
> 
> At least, with my driver, the first we see is an error returned by the
> processing engine. That ultimately gets reported to userspace the aio
> path.

So, you say that the error is returned. Isn't that what you want or expect?
> 
> I'm chasing a particularly evil bug in my driver at the moment that is
> locking up the CPU and IOMMUs - if I ever figure that one out I'll run some
> fuzzing around these limits and see if we can find other cases.

Ciao
Stephan

[PATCH v4] crypto: AF_ALG - whitelist mask and type

[Stephan Müller]

Hi,

sorry, I forgot the right tags.

---8<---

The user space interface allows specifying the type and mask field used
to allocate the cipher. Only a subset of the possible flags are intended
for user space. Therefore, white-list the allowed flags.

In case the user space caller uses at least one non-allowed flag, EINVAL
is returned.

Reported-by: syzbot 
Cc: 
Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 35d4dcea381f..5231f421ad00 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -150,7 +150,7 @@ EXPORT_SYMBOL_GPL(af_alg_release_parent);
 
 static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 {
-   const u32 forbidden = CRYPTO_ALG_INTERNAL;
+   const u32 allowed = CRYPTO_ALG_KERN_DRIVER_ONLY;
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
struct sockaddr_alg *sa = (void *)uaddr;
@@ -158,6 +158,10 @@ static int alg_bind(struct socket *sock, struct sockaddr 
*uaddr, int addr_len)
void *private;
int err;
 
+   /* If caller uses non-allowed flag, return error. */
+   if ((sa->salg_feat & ~allowed) || (sa->salg_mask & ~allowed))
+   return -EINVAL;
+
if (sock->state == SS_CONNECTED)
return -EINVAL;
 
@@ -176,9 +180,7 @@ static int alg_bind(struct socket *sock, struct sockaddr 
*uaddr, int addr_len)
if (IS_ERR(type))
return PTR_ERR(type);
 
-   private = type->bind(sa->salg_name,
-sa->salg_feat & ~forbidden,
-sa->salg_mask & ~forbidden);
+   private = type->bind(sa->salg_name, sa->salg_feat, sa->salg_mask);
if (IS_ERR(private)) {
module_put(type->owner);
return PTR_ERR(private);
-- 
2.14.3

[PATCH v3] crypto: AF_ALG - whitelist mask and type

[Stephan Müller]

The user space interface allows specifying the type and mask field used
to allocate the cipher. Only a subset of the possible flags are intended
for user space. Therefore, white-list the allowed flags.

In case the user space caller uses at least one non-allowed flag, EINVAL
is returned.

Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 35d4dcea381f..5231f421ad00 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -150,7 +150,7 @@ EXPORT_SYMBOL_GPL(af_alg_release_parent);
 
 static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 {
-   const u32 forbidden = CRYPTO_ALG_INTERNAL;
+   const u32 allowed = CRYPTO_ALG_KERN_DRIVER_ONLY;
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
struct sockaddr_alg *sa = (void *)uaddr;
@@ -158,6 +158,10 @@ static int alg_bind(struct socket *sock, struct sockaddr 
*uaddr, int addr_len)
void *private;
int err;
 
+   /* If caller uses non-allowed flag, return error. */
+   if ((sa->salg_feat & ~allowed) || (sa->salg_mask & ~allowed))
+   return -EINVAL;
+
if (sock->state == SS_CONNECTED)
return -EINVAL;
 
@@ -176,9 +180,7 @@ static int alg_bind(struct socket *sock, struct sockaddr 
*uaddr, int addr_len)
if (IS_ERR(type))
return PTR_ERR(type);
 
-   private = type->bind(sa->salg_name,
-sa->salg_feat & ~forbidden,
-sa->salg_mask & ~forbidden);
+   private = type->bind(sa->salg_name, sa->salg_feat, sa->salg_mask);
if (IS_ERR(private)) {
module_put(type->owner);
return PTR_ERR(private);
-- 
2.14.3

Re: [PATCH 1/3] crypto: salsa20-generic - cleanup and convert to skcipher API

[Stephan Müller]

Am Montag, 1. Januar 2018, 00:46:40 CET schrieb Eric Biggers:

Hi Eric,
> 
> -static void salsa20_wordtobyte(u8 output[64], const u32 input[16])
> +static void salsa20_block(u32 *state, u32 *stream)

Shouldn't stream be __le32? This could get rid of the type casting further 
down.

>  {
>   u32 x[16];
>   int i;
> 
> - memcpy(x, input, sizeof(x));
> - for (i = 20; i > 0; i -= 2) {
> + memcpy(x, state, sizeof(x));
> +
> + for (i = 0; i < 20; i += 2) {
>   x[ 4] ^= rol32((x[ 0] + x[12]),  7);
>   x[ 8] ^= rol32((x[ 4] + x[ 0]),  9);
>   x[12] ^= rol32((x[ 8] + x[ 4]), 13);
> @@ -95,145 +73,135 @@ static void salsa20_wordtobyte(u8 output[64], const
> u32 input[16]) x[14] ^= rol32((x[13] + x[12]), 13);
>   x[15] ^= rol32((x[14] + x[13]), 18);
>   }
> - for (i = 0; i < 16; ++i)
> - x[i] += input[i];
> - for (i = 0; i < 16; ++i)
> - U32TO8_LITTLE(output + 4 * i,x[i]);
> -}
> 
> -static const char sigma[16] = "expand 32-byte k";
> -static const char tau[16] = "expand 16-byte k";
> + for (i = 0; i < 16; i++)
> + stream[i] = (__force u32)cpu_to_le32(x[i] + state[i]);
> +
> + if (++state[8] == 0)
> + state[9]++;
> +}
> 
> -static void salsa20_keysetup(struct salsa20_ctx *ctx, const u8 *k, u32
> kbytes) +static void salsa20_docrypt(u32 *state, u8 *dst, const u8 *src,
> + unsigned int bytes)
>  {
> - const char *constants;
> + u32 stream[SALSA20_BLOCK_SIZE / sizeof(u32)];

dto, __le32?

Ciao
Stephan

Re: KASAN: use-after-free Read in crypto_aead_free_instance

[Stephan Müller]

Am Mittwoch, 20. Dezember 2017, 08:48:01 CET schrieb syzbot:

Hi,

> Hello,
> 
> syzkaller hit the following crash on
> 032b4cc8ff84490c4bc7c4ef8c91e6d83a637538
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
> C reproducer is attached
> syzkaller reproducer is attached. See https://goo.gl/kgGztJ
> for information about syzkaller reproducers
> 
> 
> ==
> BUG: KASAN: use-after-free in crypto_aead_free_instance+0xc0/0xd0
> crypto/aead.c:154
> Read of size 8 at addr 8801c32cf240 by task cryptomgr_test/6646
> 
> CPU: 1 PID: 6646 Comm: cryptomgr_test Not tainted 4.15.0-rc3+ #132
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
>   __dump_stack lib/dump_stack.c:17 [inline]
>   dump_stack+0x194/0x257 lib/dump_stack.c:53
>   print_address_description+0x73/0x250 mm/kasan/report.c:252
>   kasan_report_error mm/kasan/report.c:351 [inline]
>   kasan_report+0x25b/0x340 mm/kasan/report.c:409
>   __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:430
>   crypto_aead_free_instance+0xc0/0xd0 crypto/aead.c:154
>   crypto_free_instance+0x6d/0x100 crypto/algapi.c:77
>   crypto_destroy_instance+0x3c/0x80 crypto/algapi.c:85
>   crypto_alg_put crypto/internal.h:116 [inline]
>   crypto_remove_final+0x212/0x370 crypto/algapi.c:331
>   crypto_alg_tested+0x445/0x6f0 crypto/algapi.c:320
>   cryptomgr_test+0x17/0x30 crypto/algboss.c:226
>   kthread+0x37a/0x440 kernel/kthread.c:238
>   ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:441
> 
> Allocated by task 6641:
>   save_stack+0x43/0xd0 mm/kasan/kasan.c:447
>   set_track mm/kasan/kasan.c:459 [inline]
>   kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:551
>   kmem_cache_alloc_trace+0x136/0x750 mm/slab.c:3610
>   kmalloc include/linux/slab.h:499 [inline]
>   kzalloc include/linux/slab.h:688 [inline]
>   pcrypt_create_aead crypto/pcrypt.c:291 [inline]
>   pcrypt_create+0x137/0x6c0 crypto/pcrypt.c:346
>   cryptomgr_probe+0x74/0x240 crypto/algboss.c:75
>   kthread+0x37a/0x440 kernel/kthread.c:238
>   ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:441
> 
> Freed by task 3335:
>   save_stack+0x43/0xd0 mm/kasan/kasan.c:447
>   set_track mm/kasan/kasan.c:459 [inline]
>   kasan_slab_free+0x71/0xc0 mm/kasan/kasan.c:524
>   __cache_free mm/slab.c:3488 [inline]
>   kfree+0xca/0x250 mm/slab.c:3803
>   crypto_larval_destroy+0x110/0x150 crypto/api.c:107
>   crypto_alg_put crypto/internal.h:116 [inline]
>   crypto_larval_kill+0x1e8/0x2e0 crypto/api.c:167
>   crypto_alg_mod_lookup+0x178/0x1b0 crypto/api.c:283
>   crypto_find_alg crypto/api.c:501 [inline]
>   crypto_alloc_tfm+0xf3/0x2f0 crypto/api.c:534
>   crypto_alloc_aead+0x2c/0x40 crypto/aead.c:342
>   aead_bind+0x70/0x140 crypto/algif_aead.c:482
>   alg_bind+0x1ab/0x440 crypto/af_alg.c:179
>   SYSC_bind+0x1b4/0x3f0 net/socket.c:1454
>   SyS_bind+0x24/0x30 net/socket.c:1440
>   do_syscall_32_irqs_on arch/x86/entry/common.c:327 [inline]
>   do_fast_syscall_32+0x3ee/0xf9d arch/x86/entry/common.c:389
>   entry_SYSENTER_compat+0x51/0x60 arch/x86/entry/entry_64_compat.S:125
> 
This issue vanishes after applying the patch "[PATCH v2] crypto: AF_ALG - 
limit mask and type".

Ciao
Stephan

[PATCH v2] crypto: AF_ALG - limit mask and type

[Stephan Müller]

The user space interface allows specifying the type and the mask field
used to allocate the cipher. As user space can precisely select the
desired cipher by using either the name or the driver name, additional
selection options for cipher are not considered necessary and relevant
for user space.

This fixes a bug where user space is able to cause one cipher to be
registered multiple times potentially exhausting kernel memory.

Reported-by: syzbot 
Cc: 
Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 1e5353f62067..4f4cfc5a7ef3 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -150,7 +150,6 @@ EXPORT_SYMBOL_GPL(af_alg_release_parent);
 
 static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 {
-   const u32 forbidden = CRYPTO_ALG_INTERNAL;
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
struct sockaddr_alg *sa = (void *)uaddr;
@@ -176,9 +175,12 @@ static int alg_bind(struct socket *sock, struct sockaddr 
*uaddr, int addr_len)
if (IS_ERR(type))
return PTR_ERR(type);
 
-   private = type->bind(sa->salg_name,
-sa->salg_feat & ~forbidden,
-sa->salg_mask & ~forbidden);
+   /*
+* The use of the salg_feat and salg_mask are forbidden as they expose
+* too much of the low-level handling which is not suitable for
+* hostile code.
+*/
+   private = type->bind(sa->salg_name, 0, 0);
if (IS_ERR(private)) {
module_put(type->owner);
return PTR_ERR(private);
-- 
2.14.3

[PATCH] crypto: AF_ALG - limit mask and type

[Stephan Müller]

Hi Herbert,

you see the reported problem by simply using

sa.salg_mask = 0x;

Note, I am not fully sure about whether CRYPTO_AF_ALG_ALLOWED_MASK and
CRYPTO_AF_ALG_ALLOWED_TYPE have the correct value. But I think that all
that user space should reach is potentially the ASYNC flag and the
cipher types flags.

---8<---

The user space interface allows specifying the type and the mask field
used to allocate the cipher. Only a subset of the type and mask is
considered relevant to be set by user space if needed at all.

This fixes a bug where user space is able to cause one cipher to be
registered multiple times potentially exhausting kernel memory.

Reported-by: syzbot 
Cc: 
Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 7 +++
 crypto/algif_aead.c | 2 ++
 crypto/algif_hash.c | 2 ++
 crypto/algif_rng.c  | 2 ++
 crypto/algif_skcipher.c | 2 ++
 include/crypto/if_alg.h | 1 +
 include/linux/crypto.h  | 3 +++
 7 files changed, 19 insertions(+)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 1e5353f62067..16cfbde64048 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1172,6 +1172,13 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, 
int flags,
 }
 EXPORT_SYMBOL_GPL(af_alg_get_rsgl);
 
+void af_alg_restrict_type_mask(u32 *type, u32 *mask)
+{
+   *type &= CRYPTO_AF_ALG_ALLOWED_TYPE;
+   *mask &= CRYPTO_AF_ALG_ALLOWED_MASK;
+}
+EXPORT_SYMBOL_GPL(af_alg_restrict_type_mask);
+
 static int __init af_alg_init(void)
 {
int err = proto_register(_proto, 0);
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 9d73be28cf01..5d21db83bdfd 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -463,6 +463,8 @@ static void *aead_bind(const char *name, u32 type, u32 mask)
if (!tfm)
return ERR_PTR(-ENOMEM);
 
+   af_alg_restrict_type_mask(, );
+
aead = crypto_alloc_aead(name, type, mask);
if (IS_ERR(aead)) {
kfree(tfm);
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index 76d2e716c792..f7660e80cd05 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -419,6 +419,8 @@ static void *hash_bind(const char *name, u32 type, u32 mask)
if (!tfm)
return ERR_PTR(-ENOMEM);
 
+   af_alg_restrict_type_mask(, );
+
hash = crypto_alloc_ahash(name, type, mask);
if (IS_ERR(hash)) {
kfree(tfm);
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
index 150c2b6480ed..33a7064996f2 100644
--- a/crypto/algif_rng.c
+++ b/crypto/algif_rng.c
@@ -116,6 +116,8 @@ static struct proto_ops algif_rng_ops = {
 
 static void *rng_bind(const char *name, u32 type, u32 mask)
 {
+   af_alg_restrict_type_mask(, );
+
return crypto_alloc_rng(name, type, mask);
 }
 
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 9954b078f0b9..0a4987aa9d5c 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -314,6 +314,8 @@ static void *skcipher_bind(const char *name, u32 type, u32 
mask)
if (!tfm)
return ERR_PTR(-ENOMEM);
 
+   af_alg_restrict_type_mask(, );
+
skcipher = crypto_alloc_skcipher(name, type, mask);
if (IS_ERR(skcipher)) {
kfree(tfm);
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 6abf0a3604dc..8ade69d46025 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -250,5 +250,6 @@ struct af_alg_async_req *af_alg_alloc_areq(struct sock *sk,
 int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, int flags,
struct af_alg_async_req *areq, size_t maxsize,
size_t *outlen);
+void af_alg_restrict_type_mask(u32 *type, u32 *mask);
 
 #endif /* _CRYPTO_IF_ALG_H */
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 78508ca4b108..0d7694673fff 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -70,6 +70,9 @@
 #define CRYPTO_ALG_DYING   0x0040
 #define CRYPTO_ALG_ASYNC   0x0080
 
+#define CRYPTO_AF_ALG_ALLOWED_MASK 0x00ff
+#define CRYPTO_AF_ALG_ALLOWED_TYPE 0x00ff
+
 /*
  * Set this bit if and only if the algorithm requires another algorithm of
  * the same type to handle corner cases.
-- 
2.14.3

[PATCH] crypto: AF_ALG - fix race accessing cipher request

[Stephan Müller]

Hi Herbert,

This patch would go on top of 7d2c3f54e6f646887d019faa45f35d6fe9fe82ce
"crypto: af_alg - remove locking in async callback" found in Linus' tree
which is not yet in the cryptodev-2.6 tree.

In addition, this patch is already on top of the other patches discussed
on this list fixing similar issues. I.e. depending in which order you apply
the patches, there may be a hunk. In case you want me to rebase the patch,
please let me know.

---8<---
When invoking an asynchronous cipher operation, the invocation of the
callback may be performed before the subsequent operations in the
initial code path are invoked. The callback deletes the cipher request
data structure which implies that after the invocation of the
asynchronous cipher operation, this data structure must not be accessed
any more.

The setting of the return code size with the request data structure must
therefore be moved before the invocation of the asynchronous cipher
operation.

Fixes: e870456d8e7c ("crypto: algif_skcipher - overhaul memory management")
Fixes: d887c52d6ae4 ("crypto: algif_aead - overhaul memory management")
Reported-by: syzbot 
Cc:  # v4.14+
Signed-off-by: Stephan Mueller 
---
 crypto/algif_aead.c | 10 +-
 crypto/algif_skcipher.c | 10 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 15e561dc47b5..d7ec2f4ebaf9 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -291,6 +291,10 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
/* AIO operation */
sock_hold(sk);
areq->iocb = msg->msg_iocb;
+
+   /* Remember output size that will be generated. */
+   areq->outlen = outlen;
+
aead_request_set_callback(>cra_u.aead_req,
  CRYPTO_TFM_REQ_MAY_BACKLOG,
  af_alg_async_cb, areq);
@@ -298,12 +302,8 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
 crypto_aead_decrypt(>cra_u.aead_req);
 
/* AIO operation in progress */
-   if (err == -EINPROGRESS || err == -EBUSY) {
-   /* Remember output size that will be generated. */
-   areq->outlen = outlen;
-
+   if (err == -EINPROGRESS || err == -EBUSY)
return -EIOCBQUEUED;
-   }
 
sock_put(sk);
} else {
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 6fb595cd63ac..baef9bfccdda 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -125,6 +125,10 @@ static int _skcipher_recvmsg(struct socket *sock, struct 
msghdr *msg,
/* AIO operation */
sock_hold(sk);
areq->iocb = msg->msg_iocb;
+
+   /* Remember output size that will be generated. */
+   areq->outlen = len;
+
skcipher_request_set_callback(>cra_u.skcipher_req,
  CRYPTO_TFM_REQ_MAY_SLEEP,
  af_alg_async_cb, areq);
@@ -133,12 +137,8 @@ static int _skcipher_recvmsg(struct socket *sock, struct 
msghdr *msg,
crypto_skcipher_decrypt(>cra_u.skcipher_req);
 
/* AIO operation in progress */
-   if (err == -EINPROGRESS || err == -EBUSY) {
-   /* Remember output size that will be generated. */
-   areq->outlen = len;
-
+   if (err == -EINPROGRESS || err == -EBUSY)
return -EIOCBQUEUED;
-   }
 
sock_put(sk);
} else {
-- 
2.14.3

Re: [PATCH v2] crypto: AF_ALG - race-free access of encryption flag

[Stephan Müller]

Am Mittwoch, 29. November 2017, 08:10:49 CET schrieb Herbert Xu:

Hi Herbert,

> 
> It sort of worked for skcipher because it didn't care if ctx->enc
> or even ctx->iv changed midstream.  But even there I don't think
> we need to wait a second time.  In fact waiting a second time could
> result in a dead-lock if no sendmsg call came around.

Shouldn't we then create a patch for the pre-4.14 algif_skcipher code that 
moves the wait out of the while loop to the beginning of the function in 
recvmsg?

Ciao
Stephan

[PATCH v2] crypto: AF_ALG - wait for data at beginning of recvmsg

[Stephan Müller]

The wait for data is a non-atomic operation that can sleep and therefore
potentially release the socket lock. The release of the socket lock
allows another thread to modify the context data structure. The waiting
operation for new data therefore must be called at the beginning of
recvmsg. This prevents a race condition where checks of the members of
the context data structure are performed by recvmsg while there is a
potential for modification of these values.

Fixes: e870456d8e7c ("crypto: algif_skcipher - overhaul memory management")
Fixes: d887c52d6ae4 ("crypto: algif_aead - overhaul memory management")
Reported-by: syzbot 
Cc:  # v4.14+
Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 6 --
 crypto/algif_aead.c | 6 ++
 crypto/algif_skcipher.c | 6 ++
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index e720dfe962db..e75e188b145b 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1138,12 +1138,6 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, 
int flags,
if (!af_alg_readable(sk))
break;
 
-   if (!ctx->used) {
-   err = af_alg_wait_for_data(sk, flags);
-   if (err)
-   return err;
-   }
-
seglen = min_t(size_t, (maxsize - len),
   msg_data_left(msg));
 
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 7d2d162666e5..15e561dc47b5 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -111,6 +111,12 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
size_t usedpages = 0;   /* [in]  RX bufs to be used from user */
size_t processed = 0;   /* [in]  TX bufs to be consumed */
 
+   if (!ctx->used) {
+   err = af_alg_wait_for_data(sk, flags);
+   if (err)
+   return err;
+   }
+
/*
 * Data length provided by caller via sendmsg/sendpage that has not
 * yet been processed.
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 30cff827dd8f..6fb595cd63ac 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -72,6 +72,12 @@ static int _skcipher_recvmsg(struct socket *sock, struct 
msghdr *msg,
int err = 0;
size_t len = 0;
 
+   if (!ctx->used) {
+   err = af_alg_wait_for_data(sk, flags);
+   if (err)
+   return err;
+   }
+
/* Allocate cipher request for current operation. */
areq = af_alg_alloc_areq(sk, sizeof(struct af_alg_async_req) +
 crypto_skcipher_reqsize(tfm));
-- 
2.14.3

[PATCH] crypto: AF_ALG - wait for data at beginning of recvmsg

[Stephan Müller]

The wait for data is a non-atomic operation that can sleep and therefore
potentially release the socket lock. The release of the socket lock
allows another thread to modify the context data structure. The waiting
operation for new data therefore must be called at the beginning of
recvmsg. This prevents a race condition where checks of the members of
the context data structure are performed by recvmsg while there is a
potential for modification of these values.

For skcipher, ctx->used is used as an indicator whether to wait for new
data, because skcipher can operate on a subset of the overall data
to be processed.

In contrast, aead must check ctx->more which is a flag set by user space
indicating that all data has been sent. It is required for aead to wait
until all data intended to be send by the caller are received as
the authentication operation part of the aead cipher requires the
presence of the whole data.

Fixes: e870456d8e7c ("crypto: algif_skcipher - overhaul memory management")
Fixes: d887c52d6ae4 ("crypto: algif_aead - overhaul memory management")
Reported-by: syzbot 
Cc:  # v4.14+
Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 6 --
 crypto/algif_aead.c | 6 ++
 crypto/algif_skcipher.c | 6 ++
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index e720dfe962db..e75e188b145b 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1138,12 +1138,6 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, 
int flags,
if (!af_alg_readable(sk))
break;
 
-   if (!ctx->used) {
-   err = af_alg_wait_for_data(sk, flags);
-   if (err)
-   return err;
-   }
-
seglen = min_t(size_t, (maxsize - len),
   msg_data_left(msg));
 
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 7d2d162666e5..97243068af15 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -111,6 +111,12 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
size_t usedpages = 0;   /* [in]  RX bufs to be used from user */
size_t processed = 0;   /* [in]  TX bufs to be consumed */
 
+   if (ctx->more) {
+   err = af_alg_wait_for_data(sk, flags);
+   if (err)
+   return err;
+   }
+
/*
 * Data length provided by caller via sendmsg/sendpage that has not
 * yet been processed.
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 30cff827dd8f..6fb595cd63ac 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -72,6 +72,12 @@ static int _skcipher_recvmsg(struct socket *sock, struct 
msghdr *msg,
int err = 0;
size_t len = 0;
 
+   if (!ctx->used) {
+   err = af_alg_wait_for_data(sk, flags);
+   if (err)
+   return err;
+   }
+
/* Allocate cipher request for current operation. */
areq = af_alg_alloc_areq(sk, sizeof(struct af_alg_async_req) +
 crypto_skcipher_reqsize(tfm));
-- 
2.14.3

Re: general protection fault in crypto_remove_spawns

[Stephan Müller]

Am Montag, 27. November 2017, 19:56:46 CET schrieb syzbot:

Hi Herbert,

The issue seems to trigger a bug whose results we have seen before. When 
starting the reproducer and stopping it shortly thereafter, I see the numerous 
identical entries in /proc/crypto:

name : cmac(des3_ede)
driver   : cmac(des3_ede-asm)
module   : kernel
priority : 200
refcnt   : 1
selftest : passed
internal : no
type : shash
blocksize: 8
digestsize   : 8

name : cmac(des3_ede)
driver   : cmac(des3_ede-asm)
module   : kernel
priority : 200
refcnt   : 1
selftest : passed
internal : no
type : shash
blocksize: 8
digestsize   : 8

name : cmac(des3_ede)
driver   : cmac(des3_ede-asm)
module   : kernel
priority : 200
refcnt   : 1
selftest : passed
internal : no
type : shash
blocksize: 8
digestsize   : 8

...

And this list keeps on growing without end:

# ./repro

# less /proc/crypto | wc
   9559   26456  188754

# ./repro

# less /proc/crypto | wc
  11440   31586  226032

At one point in time I think the system simply has too many entries.

Ciao
Stephan

[PATCH v2] crypto: AF_ALG - race-free access of encryption flag

[Stephan Müller]

Hi Herbert,

I verified the correctnes of the patch with Eric's test program.
Without the patch, the issue is present. With the patch, the kernel
happily lives ever after.

Changes v2: change the submission into a proper patch

Ciao
Stephan

---8<---

The function af_alg_get_rsgl may sleep to wait for additional data. In
this case, the socket lock may be dropped. This allows user space to
change values in the socket data structure. Hence, all variables of the
context that are needed before and after the af_alg_get_rsgl must be
copied into a local variable.

This issue applies to the ctx->enc variable only. Therefore, this
value is copied into a local variable that is used for all operations
before and after the potential sleep and lock release. This implies that
any changes on this variable while the kernel waits for data will only
be picked up in another recvmsg operation.

Reported-by: syzbot 
Cc:  # v4.14+
Signed-off-by: Stephan Mueller 
---
 crypto/algif_aead.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 7d2d162666e5..4ba13c0b97ca 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -110,6 +110,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr 
*msg,
size_t outlen = 0;  /* [out] RX bufs produced by kernel */
size_t usedpages = 0;   /* [in]  RX bufs to be used from user */
size_t processed = 0;   /* [in]  TX bufs to be consumed */
+   bool enc = ctx->enc;/* prevent race if sock lock dropped */
 
/*
 * Data length provided by caller via sendmsg/sendpage that has not
@@ -137,7 +138,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr 
*msg,
 * buffer provides the tag which is consumed resulting in only the
 * plaintext without a buffer for the tag returned to the caller.
 */
-   if (ctx->enc)
+   if (enc)
outlen = used + as;
else
outlen = used - as;
@@ -211,7 +212,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr 
*msg,
/* Use the RX SGL as source (and destination) for crypto op. */
rsgl_src = areq->first_rsgl.sgl.sg;
 
-   if (ctx->enc) {
+   if (enc) {
/*
 * Encryption operation - The in-place cipher operation is
 * achieved by the following operation:
@@ -288,7 +289,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr 
*msg,
aead_request_set_callback(>cra_u.aead_req,
  CRYPTO_TFM_REQ_MAY_BACKLOG,
  af_alg_async_cb, areq);
-   err = ctx->enc ? crypto_aead_encrypt(>cra_u.aead_req) :
+   err = enc ? crypto_aead_encrypt(>cra_u.aead_req) :
 crypto_aead_decrypt(>cra_u.aead_req);
 
/* AIO operation in progress */
@@ -305,7 +306,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr 
*msg,
aead_request_set_callback(>cra_u.aead_req,
  CRYPTO_TFM_REQ_MAY_BACKLOG,
  crypto_req_done, >wait);
-   err = crypto_wait_req(ctx->enc ?
+   err = crypto_wait_req(enc ?
crypto_aead_encrypt(>cra_u.aead_req) :
crypto_aead_decrypt(>cra_u.aead_req),
>wait);
-- 
2.14.3

Re: general protection fault in scatterwalk_copychunks

[Stephan Müller]

Am Dienstag, 28. November 2017, 18:24:01 CET schrieb syzbot:

Hi,

> Hello,
> 
> syzkaller hit the following crash on
> 1ea8d039f9edcfefb20d8ddfe136930f6e551529
> git://git.cmpxchg.org/linux-mmots.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
> C reproducer is attached
> syzkaller reproducer is attached. See https://goo.gl/kgGztJ
> for information about syzkaller reproducers
> 
> 
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault:  [#1] SMP KASAN
> Dumping ftrace buffer:
> (ftrace buffer empty)
> Modules linked in:
> CPU: 0 PID: 17658 Comm: syzkaller625686 Not tainted 4.14.0-mm1+ #25
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> task: 8801c734e280 task.stack: 8801c5f1
> RIP: 0010:scatterwalk_start include/crypto/scatterwalk.h:86 [inline]
> RIP: 0010:scatterwalk_pagedone include/crypto/scatterwalk.h:111 [inline]
> RIP: 0010:scatterwalk_copychunks+0x337/0x480 crypto/scatterwalk.c:55
> RSP: 0018:8801c5f175c8 EFLAGS: 00010202
> RAX:  RBX:  RCX: 82502da9
> RDX: 0001 RSI:  RDI: 0008
> RBP: 8801c5f17628 R08: 8801c5f177c0 R09: 
> R10:  R11:  R12: 
> R13: 8801c5f177c0 R14: dc00 R15: 
> FS:  7f8ce5e49700() GS:8801db40() knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 20284000 CR3: 0001ce96c000 CR4: 001406f0
> DR0:  DR1:  DR2: 
> DR3:  DR6: fffe0ff0 DR7: 0400
> Call Trace:
>   blkcipher_next_slow crypto/blkcipher.c:175 [inline]
>   blkcipher_walk_next+0xca0/0x13d0 crypto/blkcipher.c:255
>   blkcipher_walk_first+0x261/0x570 crypto/blkcipher.c:346
>   blkcipher_walk_virt+0x219/0x2a0 crypto/blkcipher.c:308
>   skcipher_null_crypt+0x10e/0x2d0 crypto/crypto_null.c:85
>   skcipher_crypt_blkcipher crypto/skcipher.c:619 [inline]
>   skcipher_encrypt_blkcipher+0x213/0x310 crypto/skcipher.c:628
>   crypto_skcipher_encrypt include/crypto/skcipher.h:445 [inline]
>   crypto_aead_copy_sgl crypto/algif_aead.c:90 [inline]
>   _aead_recvmsg crypto/algif_aead.c:228 [inline]
>   aead_recvmsg+0xc96/0x1970 crypto/algif_aead.c:313
>   aead_recvmsg_nokey+0x60/0x80 crypto/algif_aead.c:431
>   sock_recvmsg_nosec net/socket.c:805 [inline]
>   sock_recvmsg+0xc9/0x110 net/socket.c:812
>   ___sys_recvmsg+0x29b/0x630 net/socket.c:2207
>   __sys_recvmsg+0xe2/0x210 net/socket.c:2252
>   SYSC_recvmsg net/socket.c:2264 [inline]
>   SyS_recvmsg+0x2d/0x50 net/socket.c:2259
>   entry_SYSCALL_64_fastpath+0x1f/0x96

After compiling the kernel with the given config and execute the test app for 
now 10 minutes, I am unable to reproduce the issue.

Ok, granted, I use the patches https://git.kernel.org/pub/scm/linux/kernel/
git/herbert/crypto-2.6.git/commit/?id=7d2c3f54e6f646887d019faa45f35d6fe9fe82ce 
and https://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git/
commit/?id=8e1fa89aa8bc2870009b4486644e4a58f2e2a4f5

Do you have an idea what I need to change to reproduce the issue? Note, my 
test system is a KVM/QEMU 64 bit virtual machine.

Thanks.

Ciao
Stephan

Re: KASAN: use-after-free Read in aead_recvmsg

[Stephan Müller]

Am Montag, 27. November 2017, 19:56:46 CET schrieb syzbot:

Hi,

> Hello,
> 
> syzkaller hit the following crash on
> 6fc478f80f6809cc4b1a4230f47a62d3b7378dc0
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
> C reproducer is attached
> syzkaller reproducer is attached. See https://goo.gl/kgGztJ
> for information about syzkaller reproducers

Could you please re-perform the test applying the patch https://
git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git/commit/?
id=8e1fa89aa8bc2870009b4486644e4a58f2e2a4f5 ?

At least I am not able to reproduce that issue using the repro.c with the 
patch applied.

Thanks a lot.

Ciao
Stephan

Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[Stephan Müller]

Am Freitag, 24. November 2017, 13:09:06 CET schrieb Krzysztof Kozlowski:

Hi Krzysztof,
> >> 
> >> 1. I was rather thinking about extending existing exynos-rng.c [1] so
> >> it would be using TRNG as seed for PRNG as this gives you much more
> >> random data. Instead you developed totally separate driver which has
> >> its own benefits - one can choose which interface he wants. Although
> >> it is a little bit duplication.
> > 
> > As far as I can tell, these are two different devices. However, PRNG
> > shares hardware with the hash engine. Indeed there is a hardware to
> > connect TRNG and PRNG, but, IMHO, it might be hard to model that
> > dependency in kernel.
> 
> It should be as simple as setting few more registers in SSS module
> (actually maybe just enabling TRNG_SEED_START in PRNG). You do not
> have to model it in a kernel like connecting some hw_rng entity to
> cryptoai's rng_alg. See the jitterentropy-kcapi.c. I understand that
> in that case existing exynos-rng.c could expose two different RNG
> devices - one PRNG based on user's seed and second TRNG (actually
> TRNG+PRNG).
> 
> It does not seem difficult to model but the question is whether that
> makes sense.

The usage strategy for the PRNGs registered at the kernel crypto API is as 
follows:

1. crypto_rng_alloc

2. crypto_rng_reset

3. crypto_rng_generate

If in step 2 you provide NULL as input, the kernel takes get_random_bytes as 
seed source. Step 2 is the mandatory.

The Linux-RNG can be fed internally from the hw_random framework by the 
function hwrng_fillfn. This function is only used if the current_quality or 
default_quality values in the hw_random framework is set.

For the TRNG, it seems to be not set per default, but could be set as either a 
boot argument or at runtime via /sys.

If that variable is set and the TRNG is registered, it feeds random data into 
the Linux-RNG which in turn is used per default to seed a PRNG. In this case, 
no detour via user space is needed to push data from TRNG to the PRNG. Using 
that mechanism allows you to benefit from additional entropy the Linux-RNG 
collects elsewhere.
> 
> > To me it seems easier to read TRNG (or
> > /dev/random) and and write the result to PRNG manually (in software).
> 
> Indeed this gives more flexibility to the user (choice of engine) but
> first, it is slower, and second it reduces the quality of random
> numbers (PRNG reseeds itself... but in this model cannot reseed from
> TRNG).

Given the reasons above, I would think that keeping the PRMG and TRNG separate 
as offered by the current patch seems reasonable. If configured correctly, the 
TRNG can seed the PRNG at any time (including boot time) without the need of 
user space.
> 
> Best regards,
> Krzysztof


Ciao
Stephan

[PATCH v3] crypto: AF_ALG - remove locking in async callback

[Stephan Müller]

The code paths protected by the socket-lock do not use or modify the
socket in a non-atomic fashion. The actions pertaining the socket do not
even need to be handled as an atomic operation. Thus, the socket-lock
can be safely ignored.

This fixes a bug regarding scheduling in atomic as the callback function
may be invoked in interrupt context.

In addition, the sock_hold is moved before the AIO encrypt/decrypt
operation to ensure that the socket is always present. This avoids a
tiny race window where the socket is unprotected and yet used by the AIO
operation.

Finally, the release of resources for a crypto operation is moved into a
common function of af_alg_free_resources.

Fixes: e870456d8e7c8 ("crypto: algif_skcipher - overhaul memory management")
Fixes: d887c52d6ae43 ("crypto: algif_aead - overhaul memory management")
Reported-by: Romain Izard 
Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 21 ++---
 crypto/algif_aead.c | 23 ---
 crypto/algif_skcipher.c | 23 ---
 include/crypto/if_alg.h |  1 +
 4 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 85cea9de324a..358749c38894 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1021,6 +1021,18 @@ ssize_t af_alg_sendpage(struct socket *sock, struct page 
*page,
 EXPORT_SYMBOL_GPL(af_alg_sendpage);
 
 /**
+ * af_alg_free_resources - release resources required for crypto request
+ */
+void af_alg_free_resources(struct af_alg_async_req *areq)
+{
+   struct sock *sk = areq->sk;
+
+   af_alg_free_areq_sgls(areq);
+   sock_kfree_s(sk, areq, areq->areqlen);
+}
+EXPORT_SYMBOL_GPL(af_alg_free_resources);
+
+/**
  * af_alg_async_cb - AIO callback handler
  *
  * This handler cleans up the struct af_alg_async_req upon completion of the
@@ -1036,18 +1048,13 @@ void af_alg_async_cb(struct crypto_async_request *_req, 
int err)
struct kiocb *iocb = areq->iocb;
unsigned int resultlen;
 
-   lock_sock(sk);
-
/* Buffer size written by crypto operation. */
resultlen = areq->outlen;
 
-   af_alg_free_areq_sgls(areq);
-   sock_kfree_s(sk, areq, areq->areqlen);
-   __sock_put(sk);
+   af_alg_free_resources(areq);
+   sock_put(sk);
 
iocb->ki_complete(iocb, err ? err : resultlen, 0);
-
-   release_sock(sk);
 }
 EXPORT_SYMBOL_GPL(af_alg_async_cb);
 
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index aacae0837aff..db9378a45296 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -268,12 +268,23 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
 
if (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) {
/* AIO operation */
+   sock_hold(sk);
areq->iocb = msg->msg_iocb;
aead_request_set_callback(>cra_u.aead_req,
  CRYPTO_TFM_REQ_MAY_BACKLOG,
  af_alg_async_cb, areq);
err = ctx->enc ? crypto_aead_encrypt(>cra_u.aead_req) :
 crypto_aead_decrypt(>cra_u.aead_req);
+
+   /* AIO operation in progress */
+   if (err == -EINPROGRESS || err == -EBUSY) {
+   /* Remember output size that will be generated. */
+   areq->outlen = outlen;
+
+   return -EIOCBQUEUED;
+   }
+
+   sock_put(sk);
} else {
/* Synchronous operation */
aead_request_set_callback(>cra_u.aead_req,
@@ -285,19 +296,9 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
>wait);
}
 
-   /* AIO operation in progress */
-   if (err == -EINPROGRESS) {
-   sock_hold(sk);
-
-   /* Remember output size that will be generated. */
-   areq->outlen = outlen;
-
-   return -EIOCBQUEUED;
-   }
 
 free:
-   af_alg_free_areq_sgls(areq);
-   sock_kfree_s(sk, areq, areq->areqlen);
+   af_alg_free_resources(areq);
 
return err ? err : outlen;
 }
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 9954b078f0b9..30cff827dd8f 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -117,6 +117,7 @@ static int _skcipher_recvmsg(struct socket *sock, struct 
msghdr *msg,
 
if (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) {
/* AIO operation */
+   sock_hold(sk);
areq->iocb = msg->msg_iocb;
skcipher_request_set_callback(>cra_u.skcipher_req,
  CRYPTO_TFM_REQ_MAY_SLEEP,
@@ -124,6 +125,16 @@ static int _skcipher_recvmsg(struct socket *sock, struct 
msghdr *msg,
err = ctx->enc ?

Re: [PATCH] crypto: algif_aead - skip SGL entries with NULL page

[Stephan Müller]

Am Freitag, 10. November 2017, 08:29:40 CET schrieb Stephan Müller:

Hi,

> The TX SGL may contain SGL entries that are assigned a NULL page. This
> may happen if a multi-stage AIO operation is performed where the data
> for each stage is pointed to by one SGL entry. Upon completion of that
> stage, af_alg_pull_tsgl will assign NULL to the SGL entry.
> 
> The NULL cipher used to copy the AAD from TX SGL to the destination
> buffer, however, cannot handle the case where the SGL starts with an SGL
> entry having a NULL page. Thus, the code needs to advance the start
> pointer into the SGL to the first non-NULL entry.
> 
> This fixes a crash visible on Intel x86 32 bit using the libkcapi test
> suite.

This one still has an issue with zero input. I will send a fix shortly.

Ciao
Stephan

[PATCH] crypto: algif_aead - skip SGL entries with NULL page

[Stephan Müller]

The TX SGL may contain SGL entries that are assigned a NULL page. This
may happen if a multi-stage AIO operation is performed where the data
for each stage is pointed to by one SGL entry. Upon completion of that
stage, af_alg_pull_tsgl will assign NULL to the SGL entry.

The NULL cipher used to copy the AAD from TX SGL to the destination
buffer, however, cannot handle the case where the SGL starts with an SGL
entry having a NULL page. Thus, the code needs to advance the start
pointer into the SGL to the first non-NULL entry.

This fixes a crash visible on Intel x86 32 bit using the libkcapi test
suite.

Fixes: 72548b093ee38 ("crypto: algif_aead - copy AAD from src to dst")
Signed-off-by: Stephan Mueller 
---
 crypto/algif_aead.c | 24 +---
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 6cdd4fb08335..250816971edf 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -101,10 +101,10 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
struct aead_tfm *aeadc = pask->private;
struct crypto_aead *tfm = aeadc->aead;
struct crypto_skcipher *null_tfm = aeadc->null_tfm;
-   unsigned int as = crypto_aead_authsize(tfm);
+   unsigned int i, as = crypto_aead_authsize(tfm);
struct af_alg_async_req *areq;
struct af_alg_tsgl *tsgl;
-   struct scatterlist *src;
+   struct scatterlist *rsgl_src, *tsgl_src = NULL;
int err = 0;
size_t used = 0;/* [in]  TX bufs to be en/decrypted */
size_t outlen = 0;  /* [out] RX bufs produced by kernel */
@@ -179,6 +179,16 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
 
processed = used + ctx->aead_assoclen;
tsgl = list_first_entry(>tsgl_list, struct af_alg_tsgl, list);
+   for (i = 0; i < tsgl->cur; i++) {
+   if (!sg_page(tsgl->sg + i))
+   continue;
+   tsgl_src = tsgl->sg + i;
+   break;
+   }
+   if (!tsgl_src) {
+   err = -EFAULT;
+   goto free;
+   }
 
/*
 * Copy of AAD from source to destination
@@ -194,7 +204,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr 
*msg,
 */
 
/* Use the RX SGL as source (and destination) for crypto op. */
-   src = areq->first_rsgl.sgl.sg;
+   rsgl_src = areq->first_rsgl.sgl.sg;
 
if (ctx->enc) {
/*
@@ -207,7 +217,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr 
*msg,
 *  v  v
 * RX SGL: AAD || PT || Tag
 */
-   err = crypto_aead_copy_sgl(null_tfm, tsgl->sg,
+   err = crypto_aead_copy_sgl(null_tfm, tsgl_src,
   areq->first_rsgl.sgl.sg, processed);
if (err)
goto free;
@@ -225,7 +235,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr 
*msg,
 */
 
 /* Copy AAD || CT to RX SGL buffer for in-place operation. */
-   err = crypto_aead_copy_sgl(null_tfm, tsgl->sg,
+   err = crypto_aead_copy_sgl(null_tfm, tsgl_src,
   areq->first_rsgl.sgl.sg, outlen);
if (err)
goto free;
@@ -257,11 +267,11 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
 areq->tsgl);
} else
/* no RX SGL present (e.g. authentication only) */
-   src = areq->tsgl;
+   rsgl_src = areq->tsgl;
}
 
/* Initialize the crypto operation */
-   aead_request_set_crypt(>cra_u.aead_req, src,
+   aead_request_set_crypt(>cra_u.aead_req, rsgl_src,
   areq->first_rsgl.sgl.sg, used, ctx->iv);
aead_request_set_ad(>cra_u.aead_req, ctx->aead_assoclen);
aead_request_set_tfm(>cra_u.aead_req, tfm);
-- 
2.13.6

[PATCH v2] crypto: AF_ALG - remove locking in async callback

[Stephan Müller]

The code paths protected by the socket-lock do not use or modify the
socket in a non-atomic fashion. The actions pertaining the socket do not
even need to be handled as an atomic operation. Thus, the socket-lock
can be safely ignored.

This fixes a bug regarding scheduling in atomic as the callback function
may be invoked in interrupt context.

In addition, the sock_hold is moved before the AIO encrypt/decrypt
operation to ensure that the socket is always present. This avoids a
tiny race window where the socket is unprotected and yet used by the AIO
operation.

Finally, the release of resources for a crypto operation is moved into a
common function of af_alg_free_resources.

Fixes: e870456d8e7c8 ("crypto: algif_skcipher - overhaul memory management")
Fixes: d887c52d6ae43 ("crypto: algif_aead - overhaul memory management")
Reported-by: Romain Izard 
Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 21 ++---
 crypto/algif_aead.c | 23 ---
 crypto/algif_skcipher.c | 23 ---
 include/crypto/if_alg.h |  1 +
 4 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 85cea9de324a..358749c38894 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1021,6 +1021,18 @@ ssize_t af_alg_sendpage(struct socket *sock, struct page 
*page,
 EXPORT_SYMBOL_GPL(af_alg_sendpage);
 
 /**
+ * af_alg_free_resources - release resources required for crypto request
+ */
+void af_alg_free_resources(struct af_alg_async_req *areq)
+{
+   struct sock *sk = areq->sk;
+
+   af_alg_free_areq_sgls(areq);
+   sock_kfree_s(sk, areq, areq->areqlen);
+}
+EXPORT_SYMBOL_GPL(af_alg_free_resources);
+
+/**
  * af_alg_async_cb - AIO callback handler
  *
  * This handler cleans up the struct af_alg_async_req upon completion of the
@@ -1036,18 +1048,13 @@ void af_alg_async_cb(struct crypto_async_request *_req, 
int err)
struct kiocb *iocb = areq->iocb;
unsigned int resultlen;
 
-   lock_sock(sk);
-
/* Buffer size written by crypto operation. */
resultlen = areq->outlen;
 
-   af_alg_free_areq_sgls(areq);
-   sock_kfree_s(sk, areq, areq->areqlen);
-   __sock_put(sk);
+   af_alg_free_resources(areq);
+   sock_put(sk);
 
iocb->ki_complete(iocb, err ? err : resultlen, 0);
-
-   release_sock(sk);
 }
 EXPORT_SYMBOL_GPL(af_alg_async_cb);
 
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index aacae0837aff..6cdd4fb08335 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -268,12 +268,23 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
 
if (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) {
/* AIO operation */
+   sock_hold(sk);
areq->iocb = msg->msg_iocb;
aead_request_set_callback(>cra_u.aead_req,
  CRYPTO_TFM_REQ_MAY_BACKLOG,
  af_alg_async_cb, areq);
err = ctx->enc ? crypto_aead_encrypt(>cra_u.aead_req) :
 crypto_aead_decrypt(>cra_u.aead_req);
+
+   /* AIO operation in progress */
+   if (err == -EINPROGRESS) {
+   /* Remember output size that will be generated. */
+   areq->outlen = outlen;
+
+   return -EIOCBQUEUED;
+   }
+
+   sock_put(sk);
} else {
/* Synchronous operation */
aead_request_set_callback(>cra_u.aead_req,
@@ -285,19 +296,9 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
>wait);
}
 
-   /* AIO operation in progress */
-   if (err == -EINPROGRESS) {
-   sock_hold(sk);
-
-   /* Remember output size that will be generated. */
-   areq->outlen = outlen;
-
-   return -EIOCBQUEUED;
-   }
 
 free:
-   af_alg_free_areq_sgls(areq);
-   sock_kfree_s(sk, areq, areq->areqlen);
+   af_alg_free_resources(areq);
 
return err ? err : outlen;
 }
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 9954b078f0b9..0e0dd44f0545 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -117,6 +117,7 @@ static int _skcipher_recvmsg(struct socket *sock, struct 
msghdr *msg,
 
if (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) {
/* AIO operation */
+   sock_hold(sk);
areq->iocb = msg->msg_iocb;
skcipher_request_set_callback(>cra_u.skcipher_req,
  CRYPTO_TFM_REQ_MAY_SLEEP,
@@ -124,6 +125,16 @@ static int _skcipher_recvmsg(struct socket *sock, struct 
msghdr *msg,
err = ctx->enc ?
crypto_skcipher_encrypt(>cra_u.skcipher_req) :
   

[PATCH] crypto: AF_ALG - remove locking in async callback

[Stephan Müller]

Am Mittwoch, 25. Oktober 2017, 17:26:31 CET schrieb Romain Izard:

Hi Romain,

the patch below should cover the issue you see. Would you mind testing it?

Thanks
Stephan

---8<---

The code paths protected by the socket-lock do not use or modify the
socket in a non-atomic fashion. The actions pertaining the socket do not
even need to be handled as an atomic operation. Thus, the socket-lock
can be safely ignored.

This fixes a bug regarding scheduling in atomic as the callback function
may be invoked in interrupt context.

Fixes: 2d97591ef43d0 ("crypto: af_alg - consolidation of duplicate code")
Reported-by: Romain Izard 
Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 337cf382718e..a41f08642eee 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1063,8 +1063,6 @@ void af_alg_async_cb(struct crypto_async_request *_req, 
int err)
struct kiocb *iocb = areq->iocb;
unsigned int resultlen;
 
-   lock_sock(sk);
-
/* Buffer size written by crypto operation. */
resultlen = areq->outlen;
 
@@ -1073,8 +1071,6 @@ void af_alg_async_cb(struct crypto_async_request *_req, 
int err)
__sock_put(sk);
 
iocb->ki_complete(iocb, err ? err : resultlen, 0);
-
-   release_sock(sk);
 }
 EXPORT_SYMBOL_GPL(af_alg_async_cb);
 
-- 
2.13.6

Re: [PATCH v2] crypto: shash - Fix zero-length shash ahash digest crash

[Stephan Müller]

Am Montag, 9. Oktober 2017, 17:30:02 CEST schrieb Herbert Xu:

Hi Herbert,

> The shash ahash digest adaptor function may crash if given a
> zero-length input together with a null SG list.  This is because
> it tries to read the SG list before looking at the length.
> 
> This patch fixes it by checking the length first.

The patch fixes the issue.
> 
> Cc: <sta...@vger.kernel.org>
> Reported-by: Stephan Müller<smuel...@chronox.de>
> Signed-off-by: Herbert Xu <herb...@gondor.apana.org.au>

Tested-by: Stephan Müller <smuel...@chronox.de>

Ciao
Stephan

Re: [PATCH 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Stephan Müller]

Am Montag, 9. Oktober 2017, 16:19:25 CEST schrieb Herbert Xu:

Hi Herbert,

> On Sat, Oct 07, 2017 at 02:56:24PM +0200, Stephan Müller wrote:
> > Though, this opens up the shash issue I tried to fix.
> 
> Does this patch fix the crash?

I get the following during boot:

[1.042673] [ cut here ]
[1.043208] kernel BUG at crypto/asymmetric_keys/public_key.c:96!
[1.044235] invalid opcode:  [#1] SMP
[1.044661] Modules linked in:
[1.044964] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 4.14.0-rc1+ #556
[1.045638] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.10.2-1.fc26 04/01/2014
[1.046397] task: 8ecefc88 task.stack: a776c031c000
[1.046943] RIP: 0010:public_key_verify_signature+0x25c/0x270
[1.047539] RSP: 0018:a776c031fcd8 EFLAGS: 00010246
[1.047997] RAX: a5cb1d5f RBX: 8eceb61f9780 RCX: 

[1.048618] RDX: 8eceb6169cc0 RSI: 8eceb6169cc0 RDI: 
8ecefc99be80
[1.049261] RBP: a776c031fcf0 R08: 00c9 R09: 
8ecefd003800
[1.049870] R10: c78580d88100 R11: 7fff R12: 
0001
[1.050487] R13: 8eceb61fa2a0 R14: a60c5870 R15: 
0542
[1.051118] FS:  () GS:8eceffd0() knlGS:

[1.051805] CS:  0010 DS:  ES:  CR0: 80050033
[1.052295] CR2: 7f89a5ee2000 CR3: 36036004 CR4: 
003606e0
[1.052853] Call Trace:
[1.053069]  ? x509_check_for_self_signed+0x86/0xd0
[1.053446]  x509_cert_parse+0x15e/0x1c0
[1.053764]  x509_key_preparse+0x26/0x1e0
[1.054094]  asymmetric_key_preparse+0x5c/0xd0
[1.054438]  key_create_or_update+0x137/0x430
[1.054789]  ? set_debug_rodata+0x17/0x17
[1.055119]  load_system_certificate_list+0x99/0xfa
[1.055494]  ? system_trusted_keyring_init+0x66/0x66
[1.055890]  ? set_debug_rodata+0x17/0x17
[1.056221]  do_one_initcall+0x41/0x160
[1.056519]  kernel_init_freeable+0x173/0x201
[1.056867]  ? rest_init+0xb0/0xb0
[1.057161]  kernel_init+0xe/0x110
[1.057426]  ret_from_fork+0x25/0x30
[1.057714] Code: 40 00 85 c0 b8 7f ff ff ff 44 0f 45 f8 eb 8a 48 8d bd e0 
fe ff ff e8 d4 ad 40 00 44 8b bd 00 ff ff ff e9 5a ff ff ff 0f 0b 0f 0b <0f> 
0b 0f 0b 41 bf ea ff ff ff e9 7a ff ff ff 0f 1f 44 00 00 0f 
[1.059208] RIP: public_key_verify_signature+0x25c/0x270 RSP: 
a776c031fcd8
[1.059782] ---[ end trace 5363a8b61ab8b581 ]---
[1.060236] Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x000b
[1.060236] 
[1.061073] Kernel Offset: 0x2400 from 0x8100 (relocation 
range: 0x8000-0xbfff)
[1.061990] ---[ end Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x000b
Ciao
Stephan

[ANNOUNCE] libkcapi v1.0.0 released

[Stephan Müller]

Hi,

The Linux kernel exports a network interface of type AF_ALG to allow user 
space to utilize the kernel crypto API. libkcapi uses this network interface 
and exports an easy to use API so that a developer does not need to consider 
the low-level network interface handling. 

The library does not implement any low level cipher algorithms. All consumer 
requests are sent to the kernel for processing. Results from the kernel crypto 
API are returned to the consumer via the library API.

The kernel interface and therefore this library can be used by unprivileged 
processes. By using the convenience API functions, one API call performs one 
complete cipher operation.

The library code archive also provides a drop-in replacement for the command 
line tools of sha*sum, fipscheck/fipshmac and sha512hmac. It also contains 
command line tools to use the hashing, symmetric ciphers and random number 
generation on the command line.

The source code and the documentation is available at [1].

[1] http://www.chronox.de/libkcapi.html

Changes 1.0.0
 * Fix: Small compile fixes for new checks of GCC 7
 * API Change: Rename all LOG_* enums to KCAPI_LOG_* to prevent namespace
   poisoning
 * Fix: soname and file name of library now compiles with
   conventions (thanks to Marcus Meissner)
 * Fix: kcapi-rng.c: unify FD/syscall read code and fix
   __NR_getrandom resolution
 * Enhancement: add kcapi-enc application to access symmetric encryption on
   command line
 * Fix: consolidate duplicate code in kcapi-hasher
 * Enhancement: add kcapi-dgst application to access hashes on command line
 * Enhancement: add kcapi-rng man page
 * Enhancement: add kcapi-rng --hex command line option
 * Fix: enable full symmetric AIO support
 * Fix: consolidate all test code into test/ and invoke all tests
   with test-invocation.sh
 * Fix: fix memleaks in error code paths as reported by clang
 * Fix: reduce memory footprint by rearranging data structures
 * Fix: kcapi-hasher is now fully FIPS 140-2 compliant as it now
   includes the integrity test for libkcapi.so
 * Enhancement: Add speed tests for MV-CESA accelerated ciphers and hash
   algorithms (thanks to Bastian Stender)
 * Test Enhancement: add kcapi-enc-test-large.c test testing edge conditions
   of AF_ALG
 * Test Enhancement: add virttest.sh - use of test system based on
   eudyptula-boot to test on linux-4.3.6, linux-4.4.86, linux-4.5, linux-4.7,
   linux-4.10, linux-4.12
 * Test Enhancement: add kcapi-fuzz-test.sh to support fuzzing the AF_ALG
   interfaces
 * Enhancement: add RPM SPEC file (tested with Fedora 26)
 * API Change: replace --disable-lib-asym with --enable-lib-asym as the
   algif_akcipher.c kernel interface is not likely to be added to the kernel
   anytime soon
 * API Enhancement: add KPP API which is not compiled by default, use
   --enable-lib-kpp (the algif_kpp.c kernel interface is not likely to be
   added to the Linux kernel any time soon)
 * Test Enhancement: Add KPP tests
 * Enhancement: Re-enable AIO support for symmetric and AEAD ciphers down to
   Linux kernels 4.1 and 4.7, respectively. This is due to integrating a
   fix against a kernel crash when using AIO.
 * Fix: simply KDF code base
 * API Enhancement: add message digest convenience functions kcapi_md_*sha*
 * API Enhancement: add cipher convenience functions kcapi_cipher_*_aes_*
 * API Enhancement: add rng convenience function kcapi_rng_get_bytes
 * API Change: remove kcapi_aead_getdata, use kcapi_aead_getdata_input and
   kcapi_aead_getdata_output instead
 * API Change: remove kcapi_aead_outbuflen, use kcapi_aead_outbuflen_enc and
   kcapi_aead_outbuflen_dec instead

Ciao
Stephan

Re: [PATCH 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Stephan Müller]

Am Samstag, 7. Oktober 2017, 05:29:48 CEST schrieb Herbert Xu:

Hi Herbert,

> I see the problem now.  This was introduced with the skcipher walk
> interface.  The blkcipher walk interface didn't have this issue.
> 
> I guess we should add a zero test vector once this is fixed.

Thank you.

This fixes the issue. 

Tested-by: Stephan Müller <smuel...@chronox.de>

Though, this opens up the shash issue I tried to fix.

[  213.090857] [ cut here ]
[  213.091318] kernel BUG at ./include/linux/scatterlist.h:123!
[  213.091986] invalid opcode:  [#1] SMP
[  213.092307] Modules linked in: algif_skcipher crypto_user authenc 
algif_aead af_alg ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6 
nf_defrag_ipv6 nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack 
ip_set nfnetlink ebtable_nat ebtable_broute bridge stp llc ip6table_mangle 
ip6table_raw ip6table_security iptable_mangle iptable_raw iptable_security 
ebtable_filter ebtables ip6table_filter ip6_tables crct10dif_pclmul 
crc32_pclmul ghash_clmulni_intel virtio_balloon virtio_net pcspkr i2c_piix4 
sch_fq_codel virtio_blk virtio_console crc32c_intel virtio_pci virtio_ring 
serio_raw virtio
[  213.092307] CPU: 0 PID: 1014 Comm: kcapi Not tainted 4.14.0-rc1+ #555
[  213.092307] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.10.2-1.fc26 04/01/2014
[  213.092307] task: 8b8838e94f80 task.stack: 9dbe40598000
[  213.092307] RIP: 0010:shash_ahash_digest+0xc9/0xd0
[  213.092307] RSP: 0018:9dbe4059bc38 EFLAGS: 00010286
[  213.092307] RAX: 87654321 RBX: 8b883bf64cb8 RCX: 
8b883bf64810
[  213.092307] RDX:  RSI: 8b883bf64d08 RDI: 
3bf64858
[  213.092307] RBP: 9dbe4059bc58 R08: 99402320 R09: 
9dbe4059bd70
[  213.092307] R10: 9dbe4059bcb8 R11: 8b883bf64810 R12: 
8b883bf64d08
[  213.092307] R13: 8b8837d1a248 R14: 8b8838555980 R15: 
8b883bf64810
[  213.092307] FS:  7facf85b8700() GS:8b883fc0() knlGS:

[  213.092307] CS:  0010 DS:  ES:  CR0: 80050033
[  213.092307] CR2:  CR3: 7845e002 CR4: 
003606f0
[  213.092307] Call Trace:
[  213.092307]  ? shash_ahash_digest+0xd0/0xd0
[  213.092307]  shash_async_digest+0x24/0x30
[  213.092307]  crypto_ahash_op+0x29/0x70
[  213.092307]  crypto_ahash_digest+0x16/0x20
[  213.092307]  crypto_authenc_genicv+0x7b/0xb0 [authenc]
[  213.092307]  ? simd_skcipher_encrypt+0xb7/0xc0
[  213.092307]  crypto_authenc_encrypt+0xa3/0x170 [authenc]
[  213.092307]  aead_recvmsg+0x2dd/0x5f0 [algif_aead]
[  213.092307]  sock_recvmsg+0x3d/0x50
[  213.092307]  sock_read_iter+0x86/0xc0
[  213.092307]  __vfs_read+0xcb/0x120
[  213.092307]  vfs_read+0x8e/0x130
[  213.092307]  SyS_read+0x46/0xa0
[  213.092307]  do_syscall_64+0x5f/0xf0
[  213.092307]  entry_SYSCALL64_slow_path+0x25/0x25
[  213.092307] RIP: 0033:0x7facf7ec41b0
[  213.092307] RSP: 002b:779f22b8 EFLAGS: 0246 ORIG_RAX: 

[  213.092307] RAX: ffda RBX: 1000 RCX: 
7facf7ec41b0
[  213.092307] RDX: 1000 RSI: 779f33c0 RDI: 
0006
[  213.092307] RBP: 01ad701c R08:  R09: 
779f2250
[  213.092307] R10:  R11: 0246 R12: 

[  213.092307] R13: 779f33c0 R14: 779f33c0 R15: 
779f23c0
[  213.092307] Code: 03 35 14 0e a4 00 48 01 fe 4c 89 e7 e8 71 fa ff ff 41 89 
c5 41 83 ae c0 08 00 00 01 41 f6 44 24 09 02 74 92 e8 79 6f 42 00 eb 8b <0f> 
0b 0f 0b 0f 1f 00 0f 1f 44 00 00 48 8b 47 20 55 48 8d 77 50 
[  213.092307] RIP: shash_ahash_digest+0xc9/0xd0 RSP: 9dbe4059bc38
[  213.125048] ---[ end trace 7b80ad4517eb7967 ]---

Ciao
Stephan

Re: [PATCH 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Stephan Müller]

Am Samstag, 7. Oktober 2017, 05:07:52 CEST schrieb Herbert Xu:

Hi Herbert,

> On Sat, Oct 07, 2017 at 04:53:46AM +0200, Stephan Müller wrote:
> > I use authenc(hmac(sha256),cbc(aes)) which in turn uses cbc-aes-aesni on
> > my
> > system.
> 
> So where exactly is it crashing in cbc-aes-aesni? AFAICS it should
> handle the zero case just fine.  Or is it hmac that's crashing as
> your other patch suggested?

The bug happens with the following invocation sequence:

setsockopt(3, SOL_ALG, 5, NULL, 1)  = -1 EBUSY (Device or resource busy)
sendmsg(6, {msg_name=NULL, msg_namelen=0, msg_iov=NULL, msg_iovlen=0, 
msg_control=[{cmsg_len=20, cmsg_level=SOL_ALG, cmsg_type=0x3}, {cmsg_len=40, 
cmsg_level=SOL_ALG, cmsg_type=0x2}, {cmsg_len=20, cmsg_level=SOL_ALG, 
cmsg_type=0x4}], msg_controllen=88, msg_flags=0}, MSG_MORE) = 0
vmsplice(5, [{iov_base="V", iov_len=1}], 1, SPLICE_F_GIFT) = 1
splice(4, NULL, 6, NULL, 1, 0)  = 1
read(6,  )  = ?
+++ killed by SIGKILL +++

The kernel reports the following crash:

[135385.003653] BUG: unable to handle kernel NULL pointer dereference at 
0010
[135385.004007] IP: skcipher_walk_skcipher+0x18/0xb0
[135385.004007] PGD 7bbf4067 P4D 7bbf4067 PUD 784a6067 PMD 0 
[135385.004007] Oops:  [#1] SMP
[135385.004007] Modules linked in: authenc algif_aead algif_rng algif_skcipher 
crypto_user algif_hash af_alg ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 
nf_conntrack_ipv6 nf_defrag_ipv6 nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack 
nf_conntrack ip_set nfnetlink ebtable_nat ebtable_broute bridge stp llc 
ip6table_mangle ip6table_raw ip6table_security iptable_mangle iptable_raw 
iptable_security ebtable_filter ebtables ip6table_filter ip6_tables 
crct10dif_pclmul crc32_pclmul ghash_clmulni_intel virtio_net virtio_balloon 
pcspkr i2c_piix4 sch_fq_codel virtio_blk virtio_console virtio_pci 
crc32c_intel virtio_ring serio_raw virtio
[135385.004007] CPU: 3 PID: 1148 Comm: lt-kcapi Not tainted 4.14.0-rc1+ #554
[135385.004007] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.10.2-1.fc26 04/01/2014
[135385.004007] task: 976fb9380d40 task.stack: 9fd280e24000
[135385.004007] RIP: 0010:skcipher_walk_skcipher+0x18/0xb0
[135385.004007] RSP: 0018:9fd280e27ba0 EFLAGS: 00010246
[135385.004007] RAX:  RBX: 9fd280e27be0 RCX: 

[135385.004007] RDX: 976fb85a5428 RSI: 976f76330d08 RDI: 
9fd280e27be0
[135385.004007] RBP: 9fd280e27bc0 R08: 87654321 R09: 
976fb842b880
[135385.004007] R10: 9fd280e27cb8 R11:  R12: 
0001
[135385.004007] R13: 976f76330d08 R14: 976fb842b800 R15: 

[135385.004007] FS:  7fcb922bb700() GS:976fbfd8() knlGS:

[135385.004007] CS:  0010 DS:  ES:  CR0: 80050033
[135385.004007] CR2: 0010 CR3: 798c4001 CR4: 
003606e0
[135385.004007] Call Trace:
[135385.004007]  ? skcipher_walk_virt+0x1e/0x40
[135385.004007]  cbc_encrypt+0x3e/0xc0
[135385.004007]  ? skcipher_null_crypt+0x64/0x80
[135385.004007]  simd_skcipher_encrypt+0xb7/0xc0
[135385.004007]  ? simd_skcipher_encrypt+0xb7/0xc0
[135385.004007]  crypto_authenc_encrypt+0x94/0x170 [authenc]
[135385.004007]  aead_recvmsg+0x2dd/0x5f0 [algif_aead]
[135385.004007]  sock_recvmsg+0x3d/0x50
[135385.004007]  sock_read_iter+0x86/0xc0
[135385.004007]  __vfs_read+0xcb/0x120
[135385.004007]  vfs_read+0x8e/0x130
[135385.004007]  SyS_read+0x46/0xa0
[135385.004007]  do_syscall_64+0x5f/0xf0
[135385.004007]  entry_SYSCALL64_slow_path+0x25/0x25
[135385.004007] RIP: 0033:0x7fcb91bc71b0
[135385.004007] RSP: 002b:7ffe41fc2898 EFLAGS: 0246 ORIG_RAX: 

[135385.004007] RAX: ffda RBX: 1000 RCX: 
7fcb91bc71b0
[135385.004007] RDX: 1000 RSI: 7ffe41fc39a0 RDI: 
0006
[135385.004007] RBP: 012f601c R08: 0001 R09: 

[135385.004007] R10:  R11: 0246 R12: 

[135385.004007] R13: 7ffe41fc39a0 R14: 7ffe41fc39a0 R15: 
7ffe41fc29a0
[135385.004007] Code: ff ff ff e9 42 ff ff ff 90 66 2e 0f 1f 84 00 00 00 00 00 
0f 1f 44 00 00 48 8b 46 10 48 8b 56 40 55 8b 8f 84 00 00 00 48 89 47 20 <8b> 
40 10 48 89 e5 83 e1 ef 89 47 28 48 8b 46 18 48 89 47 38 8b 
[135385.004007] RIP: skcipher_walk_skcipher+0x18/0xb0 RSP: 9fd280e27ba0
[135385.004007] CR2: 0010
[135385.004007] ---[ end trace 25c44edb63da431d ]---
[135385.004007] Kernel panic - not syncing: Fatal exception
[135385.004007] Kernel Offset: 0x1100 from 0x8100 (relocation 
range: 0x8000-0xbfff)
[135385.004007] ---[ end Kernel panic - not syncing: Fatal exception
> 
> Cheers,


Ciao
Stephan

Re: [PATCH 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Stephan Müller]

Am Samstag, 7. Oktober 2017, 04:46:35 CEST schrieb Herbert Xu:

Hi Herbert,

> Hmm this just papers over bugs in the underlying code.  Which
> algorithm is causing the crash with a zero input? They're supposed
> to handle this case.

The bug happens with authenc. It is surely possible to modify authenc. Yet I 
thought that covering such issue at a central spot at least prevents similar 
buts to be exploitable from userspace.

Ciao
Stephan

[PATCH v2] crypto: keywrap - simplify code

[Stephan Müller]

Hi Herbert,

Changes v2:
* use __be64 for A and R variables
* eliminate tbe variable
* use initial IV value with cpu_to_be64 when accessing variable A

---8<---

The code is simplified by using two __be64 values for the operation
instead of using two arrays of u8. This allows to get rid of the memory
alignment code. In addition, the crypto_xor can be replaced with a
native XOR operation. Finally, the definition of the variables is
re-arranged such that the data structures come before simple variables
to potentially reduce memory space.

Signed-off-by: Stephan Mueller 
---
 crypto/keywrap.c | 84 ++--
 1 file changed, 26 insertions(+), 58 deletions(-)

diff --git a/crypto/keywrap.c b/crypto/keywrap.c
index 72014f963ba7..744e35134c45 100644
--- a/crypto/keywrap.c
+++ b/crypto/keywrap.c
@@ -93,18 +93,10 @@ struct crypto_kw_ctx {
 
 struct crypto_kw_block {
 #define SEMIBSIZE 8
-   u8 A[SEMIBSIZE];
-   u8 R[SEMIBSIZE];
+   __be64 A;
+   __be64 R;
 };
 
-/* convert 64 bit integer into its string representation */
-static inline void crypto_kw_cpu_to_be64(u64 val, u8 *buf)
-{
-   __be64 *a = (__be64 *)buf;
-
-   *a = cpu_to_be64(val);
-}
-
 /*
  * Fast forward the SGL to the "end" length minus SEMIBSIZE.
  * The start in the SGL defined by the fast-forward is returned with
@@ -139,17 +131,10 @@ static int crypto_kw_decrypt(struct blkcipher_desc *desc,
struct crypto_blkcipher *tfm = desc->tfm;
struct crypto_kw_ctx *ctx = crypto_blkcipher_ctx(tfm);
struct crypto_cipher *child = ctx->child;
-
-   unsigned long alignmask = max_t(unsigned long, SEMIBSIZE,
-   crypto_cipher_alignmask(child));
-   unsigned int i;
-
-   u8 blockbuf[sizeof(struct crypto_kw_block) + alignmask];
-   struct crypto_kw_block *block = (struct crypto_kw_block *)
-   PTR_ALIGN(blockbuf + 0, alignmask + 1);
-
-   u64 t = 6 * ((nbytes) >> 3);
+   struct crypto_kw_block block;
struct scatterlist *lsrc, *ldst;
+   u64 t = 6 * ((nbytes) >> 3);
+   unsigned int i;
int ret = 0;
 
/*
@@ -160,7 +145,7 @@ static int crypto_kw_decrypt(struct blkcipher_desc *desc,
return -EINVAL;
 
/* Place the IV into block A */
-   memcpy(block->A, desc->info, SEMIBSIZE);
+   memcpy(, desc->info, SEMIBSIZE);
 
/*
 * src scatterlist is read-only. dst scatterlist is r/w. During the
@@ -171,32 +156,27 @@ static int crypto_kw_decrypt(struct blkcipher_desc *desc,
ldst = dst;
 
for (i = 0; i < 6; i++) {
-   u8 tbe_buffer[SEMIBSIZE + alignmask];
-   /* alignment for the crypto_xor and the _to_be64 operation */
-   u8 *tbe = PTR_ALIGN(tbe_buffer + 0, alignmask + 1);
-   unsigned int tmp_nbytes = nbytes;
struct scatter_walk src_walk, dst_walk;
+   unsigned int tmp_nbytes = nbytes;
 
while (tmp_nbytes) {
/* move pointer by tmp_nbytes in the SGL */
crypto_kw_scatterlist_ff(_walk, lsrc, tmp_nbytes);
/* get the source block */
-   scatterwalk_copychunks(block->R, _walk, SEMIBSIZE,
+   scatterwalk_copychunks(, _walk, SEMIBSIZE,
   false);
 
-   /* perform KW operation: get counter as byte string */
-   crypto_kw_cpu_to_be64(t, tbe);
/* perform KW operation: modify IV with counter */
-   crypto_xor(block->A, tbe, SEMIBSIZE);
+   block.A ^= cpu_to_be64(t);
t--;
/* perform KW operation: decrypt block */
-   crypto_cipher_decrypt_one(child, (u8*)block,
- (u8*)block);
+   crypto_cipher_decrypt_one(child, (u8*),
+ (u8*));
 
/* move pointer by tmp_nbytes in the SGL */
crypto_kw_scatterlist_ff(_walk, ldst, tmp_nbytes);
/* Copy block->R into place */
-   scatterwalk_copychunks(block->R, _walk, SEMIBSIZE,
+   scatterwalk_copychunks(, _walk, SEMIBSIZE,
   true);
 
tmp_nbytes -= SEMIBSIZE;
@@ -208,11 +188,10 @@ static int crypto_kw_decrypt(struct blkcipher_desc *desc,
}
 
/* Perform authentication check */
-   if (crypto_memneq("\xA6\xA6\xA6\xA6\xA6\xA6\xA6\xA6", block->A,
- SEMIBSIZE))
+   if (block.A != cpu_to_be64(0xa6a6a6a6a6a6a6a6))
ret = -EBADMSG;
 
-   memzero_explicit(block, sizeof(struct crypto_kw_block));
+  

[PATCH] crypto: keywrap - simplify code

[Stephan Müller]

The code is simplified by using two u64 values for the operation instead
of using two arrays of u8. This allows to get rid of the memory
alignment code. In addition, the crypto_xor can be replaced with a
native XOR operation. Finally, the definition of the variables is
re-arranged such that the data structures come before simple variables
to potentially reduce memory space.

Signed-off-by: Stephan Mueller 
---
 crypto/keywrap.c | 86 
 1 file changed, 30 insertions(+), 56 deletions(-)

diff --git a/crypto/keywrap.c b/crypto/keywrap.c
index 72014f963ba7..ce7a36fc6de4 100644
--- a/crypto/keywrap.c
+++ b/crypto/keywrap.c
@@ -93,18 +93,10 @@ struct crypto_kw_ctx {
 
 struct crypto_kw_block {
 #define SEMIBSIZE 8
-   u8 A[SEMIBSIZE];
-   u8 R[SEMIBSIZE];
+   u64 A;
+   u64 R;
 };
 
-/* convert 64 bit integer into its string representation */
-static inline void crypto_kw_cpu_to_be64(u64 val, u8 *buf)
-{
-   __be64 *a = (__be64 *)buf;
-
-   *a = cpu_to_be64(val);
-}
-
 /*
  * Fast forward the SGL to the "end" length minus SEMIBSIZE.
  * The start in the SGL defined by the fast-forward is returned with
@@ -139,17 +131,10 @@ static int crypto_kw_decrypt(struct blkcipher_desc *desc,
struct crypto_blkcipher *tfm = desc->tfm;
struct crypto_kw_ctx *ctx = crypto_blkcipher_ctx(tfm);
struct crypto_cipher *child = ctx->child;
-
-   unsigned long alignmask = max_t(unsigned long, SEMIBSIZE,
-   crypto_cipher_alignmask(child));
-   unsigned int i;
-
-   u8 blockbuf[sizeof(struct crypto_kw_block) + alignmask];
-   struct crypto_kw_block *block = (struct crypto_kw_block *)
-   PTR_ALIGN(blockbuf + 0, alignmask + 1);
-
-   u64 t = 6 * ((nbytes) >> 3);
+   struct crypto_kw_block block;
struct scatterlist *lsrc, *ldst;
+   u64 t = 6 * ((nbytes) >> 3);
+   unsigned int i;
int ret = 0;
 
/*
@@ -160,7 +145,7 @@ static int crypto_kw_decrypt(struct blkcipher_desc *desc,
return -EINVAL;
 
/* Place the IV into block A */
-   memcpy(block->A, desc->info, SEMIBSIZE);
+   memcpy(, desc->info, SEMIBSIZE);
 
/*
 * src scatterlist is read-only. dst scatterlist is r/w. During the
@@ -171,32 +156,30 @@ static int crypto_kw_decrypt(struct blkcipher_desc *desc,
ldst = dst;
 
for (i = 0; i < 6; i++) {
-   u8 tbe_buffer[SEMIBSIZE + alignmask];
-   /* alignment for the crypto_xor and the _to_be64 operation */
-   u8 *tbe = PTR_ALIGN(tbe_buffer + 0, alignmask + 1);
-   unsigned int tmp_nbytes = nbytes;
struct scatter_walk src_walk, dst_walk;
+   __be64 tbe;
+   unsigned int tmp_nbytes = nbytes;
 
while (tmp_nbytes) {
/* move pointer by tmp_nbytes in the SGL */
crypto_kw_scatterlist_ff(_walk, lsrc, tmp_nbytes);
/* get the source block */
-   scatterwalk_copychunks(block->R, _walk, SEMIBSIZE,
+   scatterwalk_copychunks(, _walk, SEMIBSIZE,
   false);
 
/* perform KW operation: get counter as byte string */
-   crypto_kw_cpu_to_be64(t, tbe);
+   tbe = cpu_to_be64(t);
/* perform KW operation: modify IV with counter */
-   crypto_xor(block->A, tbe, SEMIBSIZE);
+   block.A ^= tbe;
t--;
/* perform KW operation: decrypt block */
-   crypto_cipher_decrypt_one(child, (u8*)block,
- (u8*)block);
+   crypto_cipher_decrypt_one(child, (u8*),
+ (u8*));
 
/* move pointer by tmp_nbytes in the SGL */
crypto_kw_scatterlist_ff(_walk, ldst, tmp_nbytes);
/* Copy block->R into place */
-   scatterwalk_copychunks(block->R, _walk, SEMIBSIZE,
+   scatterwalk_copychunks(, _walk, SEMIBSIZE,
   true);
 
tmp_nbytes -= SEMIBSIZE;
@@ -208,11 +191,10 @@ static int crypto_kw_decrypt(struct blkcipher_desc *desc,
}
 
/* Perform authentication check */
-   if (crypto_memneq("\xA6\xA6\xA6\xA6\xA6\xA6\xA6\xA6", block->A,
- SEMIBSIZE))
+   if (block.A != 0xa6a6a6a6a6a6a6a6)
ret = -EBADMSG;
 
-   memzero_explicit(block, sizeof(struct crypto_kw_block));
+   memzero_explicit(, sizeof(struct crypto_kw_block));
 
return ret;
 }
@@ -224,17 +206,10 @@ static int 

Re: [PATCH 0/2] fix authenc() kernel crash

[Stephan Müller]

Am Sonntag, 24. September 2017, 08:22:54 CEST schrieb Stephan Müller:

Hi Herbert,

(private)

> Hi Herbert,
> 
> The two patches together fix a kernel crash that can be triggered via
> AF_ALG when using authenc() with zero plaintext.
> 
> The changes are also tested to verify that the hashing on null data
> is still supported.

You can test / verify the patch with the HEAD code of libkcapi found in [1].

Please execute:

./configure --enable-kcapi-test
make
bin/kcapi -h -x 2 -c "authenc(hmac(sha256),cbc(aes))" -d 2

Note, the last command may take a couple of seconds.

[1] https://github.com/smuellerDD/libkcapi


Ciao
Stephan

[PATCH 0/2] fix authenc() kernel crash

[Stephan Müller]

Hi Herbert,

The two patches together fix a kernel crash that can be triggered via
AF_ALG when using authenc() with zero plaintext.

The changes are also tested to verify that the hashing on null data
is still supported.

I suspect that the vulnerability fixed with patch 1 is present in
abklcipher that was used before the switch to skcipher. Thus, I would
suspect in older kernels that this vulnerability is still present.
Could you please provide guidance on how to address that issue in such
older kernels?

Stephan Mueller (2):
  crypto: skcipher - noop for enc/dec with NULL data
  crypto: shash - no kmap of zero SG

 crypto/shash.c| 4 +++-
 include/crypto/skcipher.h | 6 ++
 2 files changed, 9 insertions(+), 1 deletion(-)

-- 
2.13.5

[PATCH 2/2] crypto: shash - no kmap of zero SG

[Stephan Müller]

In case the caller provides an SG with zero data, prevent a kmap of the
page pointed to by the SG. In this case, it is possible that the page
does not exist.

This fixes a crash in authenc() when the plaintext is zero and thus the
encryption operation is a noop. In this case, no input data exists that
can be hashed. The crash is triggerable via AF_ALG from unprivileged
user space.

Fixes: 3b2f6df08258e ("crypto: hash - Export shash through ahash")
CC: Herbert Xu 
CC: 
Signed-off-by: Stephan Mueller 
---
 crypto/shash.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/shash.c b/crypto/shash.c
index 5e31c8d776df..32d0e1806bf4 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -278,9 +278,11 @@ int shash_ahash_digest(struct ahash_request *req, struct 
shash_desc *desc)
struct scatterlist *sg = req->src;
unsigned int offset = sg->offset;
unsigned int nbytes = req->nbytes;
+   unsigned int process = min(sg->length,
+  ((unsigned int)(PAGE_SIZE)) - offset);
int err;
 
-   if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
+   if (process && nbytes < process) {
void *data;
 
data = kmap_atomic(sg_page(sg));
-- 
2.13.5

[PATCH] crypto: DRBG - fix freeing of resources

[Stephan Müller]

During the change to use aligned buffers, the deallocation code path was
not updated correctly. The current code tries to free the aligned buffer
pointer and not the original buffer pointer as it is supposed to.

Thus, the code is updated to free the original buffer pointer and set
the aligned buffer pointer that is used throughout the code to NULL.

Fixes: 3cfc3b9721123 ("crypto: drbg - use aligned buffers")
CC: 
CC: Herbert Xu 
Signed-off-by: Stephan Mueller 
---
 crypto/drbg.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 633a88e93ab0..70018397e59a 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1133,10 +1133,10 @@ static inline void drbg_dealloc_state(struct drbg_state 
*drbg)
 {
if (!drbg)
return;
-   kzfree(drbg->V);
-   drbg->Vbuf = NULL;
-   kzfree(drbg->C);
-   drbg->Cbuf = NULL;
+   kzfree(drbg->Vbuf);
+   drbg->V = NULL;
+   kzfree(drbg->Cbuf);
+   drbg->C = NULL;
kzfree(drbg->scratchpadbuf);
drbg->scratchpadbuf = NULL;
drbg->reseed_ctr = 0;
-- 
2.13.5

Re: md5sum (from libkcapi) fails as splice() returns -ENOKEY

[Stephan Müller]

Am Montag, 11. September 2017, 19:07:31 CEST schrieb christophe leroy:

Hi christophe,

> Hello Stephan,
> 
> I'm trying to use md5sum from the latest libkcapi 0.14 and I getting a
> failure with return code -5.
> 
> What am I missing ? See strace below, splice() return -ENOKEY.

The ENOKEY error is due to an accept() at the wrong location. But I do not see 
that error:

tar xvfJ libkcapi-0.14.0.tar.xz
cd libkcapi-0.14.0
autoreconf -i
./configure --enable-kcapi-hasher
make
cd bin/.libs/ # I make no make install for testing
ln kcapi-hasher md5sum # create md5sum app
export LD_LIBRARY_PATH=../../.libs/ # location of current library

$ ./md5sum md5sum 
ddd1a82680b16fb6c241d81656b844df  md5sum

So, it works for me.

Can you please check whether you use the current library version? Note, 
library version 0.10.1 fixed the aforementioned accept() call for kernel 4.4 
and later.

$ ./md5sum -v
md5sum: libkcapi 0.14.0

Ciao
Stephan

Re: [PATCH v2] crypto: add NULL check to scatterwalk_start

[Stephan Müller]

Am Samstag, 9. September 2017, 00:20:50 CEST schrieb Stephan Müller:

Hi Herbert,

>   walk->sg = sg;
> - walk->offset = sg->offset;
> + if (sg)
> + walk->offset = sg->offset;
> + else
> + walk->offset = 0;
>  }

After running more fuzzing tests, I now cause other types of spurious crashes. 
Do you have any suggestion on how to handle that issue?

Changing skcipher_walk_skcipher with the following instead of the previously 
suggested patch does not help.

if (!req->cryptlen)
return 0;

Or do you see authenc() as a special case that does not support zero length 
plaintext/ciphertext?

[ 5420.521073] [ cut here ]
[ 5420.521770] kernel BUG at ./include/linux/scatterlist.h:123!
[ 5420.522736] invalid opcode:  [#1] SMP
[ 5420.523723] Modules linked in: ansi_cprng algif_rng ccm algif_skcipher 
des3_ede_x86_64 des_generic algif_hash crypto_user authenc algif_aead af_alg 
ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 
nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ip_set nfnetlink 
ebtable_nat ebtable_broute bridge stp llc ip6table_mangle ip6table_raw 
ip6table_security iptable_mangle iptable_raw iptable_security ebtable_filter 
ebtables ip6table_filter ip6_tables crct10dif_pclmul crc32_pclmul 
virtio_balloon ghash_clmulni_intel pcspkr i2c_piix4 virtio_net sch_fq_codel 
virtio_console virtio_blk crc32c_intel virtio_pci virtio_ring serio_raw virtio
[ 5420.523723] CPU: 3 PID: 20541 Comm: kcapi Not tainted 4.13.0-rc1+ #483
[ 5420.523723] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.10.2-1.fc26 04/01/2014
[ 5420.523723] task: a384b9ca6800 task.stack: a512c3a9c000
[ 5420.523723] RIP: 0010:shash_ahash_digest+0xc9/0xd0
[ 5420.523723] RSP: 0018:a512c3a9fc38 EFLAGS: 00010286
[ 5420.523723] RAX: 87654321 RBX: a38475fb44b8 RCX: 
a38475fb4010
[ 5420.523723] RDX:  RSI: a38475fb4508 RDI: 
75fb4088
[ 5420.523723] RBP: a512c3a9fc58 R08: 000147d6 R09: 
0007
[ 5420.523723] R10: a512c3a9fcb8 R11: 8211c14d R12: 
a38475fb4508
[ 5420.523723] R13: a384b7e88188 R14: a384b9b98600 R15: 
a38475fb4010
[ 5420.523723] FS:  7f7b9f535700() GS:a384bfd8() knlGS:

[ 5420.523723] CS:  0010 DS:  ES:  CR0: 80050033
[ 5420.523723] CR2:  CR3: 7a8ef000 CR4: 
003406e0
[ 5420.523723] Call Trace:
[ 5420.523723]  ? shash_ahash_digest+0xd0/0xd0
[ 5420.523723]  shash_async_digest+0x24/0x30
[ 5420.523723]  crypto_ahash_op+0x29/0x70
[ 5420.523723]  ? printk+0x43/0x4b
[ 5420.523723]  crypto_ahash_digest+0x16/0x20
[ 5420.523723]  crypto_authenc_genicv+0x7b/0xb0 [authenc]
[ 5420.523723]  ? simd_skcipher_encrypt+0xb7/0xc0
[ 5420.523723]  crypto_authenc_encrypt+0xb8/0x180 [authenc]
[ 5420.523723]  aead_recvmsg+0x510/0x5c0 [algif_aead]
[ 5420.523723]  sock_recvmsg+0x3d/0x50
[ 5420.523723]  sock_read_iter+0x86/0xc0
[ 5420.523723]  __vfs_read+0xcb/0x120
[ 5420.523723]  vfs_read+0x8e/0x130
[ 5420.523723]  SyS_read+0x46/0xa0
[ 5420.523723]  do_syscall_64+0x5b/0xc0
[ 5420.523723]  entry_SYSCALL64_slow_path+0x25/0x25
[ 5420.523723] RIP: 0033:0x7f7b9ee43180
[ 5420.523723] RSP: 002b:7ffd3f975718 EFLAGS: 0246 ORIG_RAX: 

[ 5420.523723] RAX: ffda RBX: 1000 RCX: 
7f7b9ee43180
[ 5420.523723] RDX: 1000 RSI: 7ffd3f976820 RDI: 
0006
[ 5420.523723] RBP: 00fa701c R08:  R09: 

[ 5420.523723] R10:  R11: 0246 R12: 

[ 5420.523723] R13: 7ffd3f976820 R14: 7ffd3f976820 R15: 
7ffd3f975820
[ 5420.523723] Code: 03 35 d4 9a a4 00 48 01 fe 4c 89 e7 e8 71 fa ff ff 41 89 
c5 41 83 ae 80 08 00 00 01 41 f6 44 24 09 02 74 92 e8 c9 c3 41 00 eb 8b <0f> 
0b 0f 0b 0f 1f 00 0f 1f 44 00 00 48 8b 47 20 55 48 8d 77 50 
[ 5420.523723] RIP: shash_ahash_digest+0xc9/0xd0 RSP: a512c3a9fc38



Ciao
Stephan

[PATCH v2] crypto: add NULL check to scatterwalk_start

[Stephan Müller]

Am Donnerstag, 7. September 2017, 08:01:08 CEST schrieb Herbert Xu:

Hi Herbert,

> On Thu, Sep 07, 2017 at 07:48:53AM +0200, Stephan Müller wrote:
> > There is already such check:
> > 
> > static inline int crypto_aead_decrypt(struct aead_request *req)
> > {
> > 
> > struct crypto_aead *aead = crypto_aead_reqtfm(req);
> > 
> > if (req->cryptlen < crypto_aead_authsize(aead))
> > 
> > return -EINVAL;
> > 
> > ...
> 
> That doesn't check assoclen, does it?
> 
> > > Perhaps we can simply
> > > truncate assoclen in aead_request_set_ad.
> > 
> > I am not sure that would work because at the time we set the AAD len, we
> > may not yet have cryptlen. I.e. aead_request_set_ad may be called before
> > aead_request_set_crypt.
> 
> We can add the truncation in both places.

The initially suggested fix was wrong in general: cryptlen only defines the 
length of the ciphertext/plaintext without the AAD. This means, cryptlen can 
surely be less than AAD.

The culprit is that in case authenc() is invoked from user space with AAD but 
with a zero plaintext. This in turn caused authenc() to invoke the CBC 
implementation with that zero plaintext which in turn simply starts a 
scatterwalk operation on the plaintext. This is in general appropriate as all 
code will handle zero lengths, except scatterwalk_start. This function assumes 
that there is always a valid SGL which is not true for for zero length input 
data.

Granted, we could fix authenc to simply not invoke the CBC operation in the 
outlined issue. However, I now stumbled over this function for a third time in 
a row over the last weeks in bugs triggerable via AF_ALG. I suspect that there 
are many more issues like this lingering in other cipher implementation. Hence, 
I feel it is prudent to fix an entire class of bugs with this patch.

Ciao
Stephan

---8<---

In edge conditions, scatterwalk_start is invoked with an empty SGL.
Although this is considered a wrong usage of scatterwalk_start, it can
still be invoked. Such invocation can occur if the data to be covered by
the SGL is zero. For example, if the authenc() cipher is invoked with an
empty plaintext, the CBC operation is invoked with an empty plaintext.

This patch fixes (at least) a crash that can be induced via AF_ALG from
unprivileged user space.

It can be argued whether authenc() should be changed to catch this
issue. Yet, this issue in scatterwalk_start was a culprit in other
kernel crash issues that have been fixed before invoking
scatterwalk_start. As this function is constantly being invoked via
AF_ALG from user space, harden the function to avoid a NULL pointer
deference is prudent and even a general fix for these common issues.
This fix therefore covers an entire class of bugs which are hard to
chase down in their own individual cipher implementations.

Fixes: ac02725812cb3 ("crypto: scatterwalk - Inline start/map/done")
CC: <sta...@vger.kernel.org>
CC: Herbert Xu <herb...@gondor.apana.org.au>
Signed-off-by: Stephan Mueller <smuel...@chronox.de>
---
 include/crypto/scatterwalk.h | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/crypto/scatterwalk.h b/include/crypto/scatterwalk.h
index 880e6be9e95e..0605d44b53bc 100644
--- a/include/crypto/scatterwalk.h
+++ b/include/crypto/scatterwalk.h
@@ -83,7 +83,10 @@ static inline void scatterwalk_start(struct scatter_walk 
*walk,
 struct scatterlist *sg)
 {
walk->sg = sg;
-   walk->offset = sg->offset;
+   if (sg)
+   walk->offset = sg->offset;
+   else
+   walk->offset = 0;
 }
 
 static inline void *scatterwalk_map(struct scatter_walk *walk)
-- 
2.13.5

Re: [PATCH] crypto: authenc - cryptlen must be at least AAD len

[Stephan Müller]

Am Donnerstag, 7. September 2017, 05:54:05 CEST schrieb Herbert Xu:

Hi Herbert,
> > 
> > What is your opinion: should this check be rather added to
> > crypto_aead_encrypt (similar to a sanity check found in
> > crypto_aead_decrypt)?
> 
> Doesn't this apply to decryption as well?

There is already such check:

static inline int crypto_aead_decrypt(struct aead_request *req)
{
struct crypto_aead *aead = crypto_aead_reqtfm(req);

if (req->cryptlen < crypto_aead_authsize(aead))
return -EINVAL;
...

> Perhaps we can simply
> truncate assoclen in aead_request_set_ad.

I am not sure that would work because at the time we set the AAD len, we may 
not yet have cryptlen. I.e. aead_request_set_ad may be called before 
aead_request_set_crypt.


Ciao
Stephan

Re: [PATCH] crypto: authenc - cryptlen must be at least AAD len

[Stephan Müller]

Am Mittwoch, 6. September 2017, 21:22:44 CEST schrieb Stephan Müller:

Hi Herbert,

> With AF_ALG, AAD len and cryptlen can be set freely by unprivileged
> user space. The cipher implementation must therefore validate the input
> data for sanity. For AEAD ciphers, this implies that cryptlen must be
> at least as large as AAD size.
> 
> This fixes a kernel crash that can be triggered via AF_ALG detected by
> the fuzzing test implemented with libkcapi.

What is your opinion: should this check be rather added to crypto_aead_encrypt 
(similar to a sanity check found in crypto_aead_decrypt)?

Ciao
Stephan

[PATCH] crypto: authenc - cryptlen must be at least AAD len

[Stephan Müller]

With AF_ALG, AAD len and cryptlen can be set freely by unprivileged
user space. The cipher implementation must therefore validate the input
data for sanity. For AEAD ciphers, this implies that cryptlen must be
at least as large as AAD size.

This fixes a kernel crash that can be triggered via AF_ALG detected by
the fuzzing test implemented with libkcapi.

CC: 
CC: Herbert Xu 
Signed-off-by: Stephan Mueller 
---
 crypto/authenc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/crypto/authenc.c b/crypto/authenc.c
index 875470b0e026..21e202fc32c1 100644
--- a/crypto/authenc.c
+++ b/crypto/authenc.c
@@ -209,6 +209,9 @@ static int crypto_authenc_encrypt(struct aead_request *req)
struct scatterlist *src, *dst;
int err;
 
+   if (req->assoclen > cryptlen)
+   return -EINVAL;
+
src = scatterwalk_ffwd(areq_ctx->src, req->src, req->assoclen);
dst = src;
 
-- 
2.13.5

[PATCH] crypto: AF_ALG - remove SGL end indicator when chaining

[Stephan Müller]

The SGL is MAX_SGL_ENTS + 1 in size. The last SG entry is used for the
chaining and is properly updated with the sg_chain invocation. During
the filling-in of the initial SG entries, sg_mark_end is called for each
SG entry. This is appropriate as long as no additional SGL is chained
with the current SGL. However, when a new SGL is chained and the last
SG entry is updated with sg_chain, the last but one entry still contains
the end marker from the sg_mark_end. This end marker must be removed as
otherwise a walk of the chained SGLs will cause a NULL pointer
dereference at the last but one SG entry, because sg_next will return
NULL.

Fixes: 8ff590903d5fc ("crypto: algif_skcipher - User-space interface
for skcipher operations")
CC: 
CC: Herbert Xu 
Signed-off-by: Stephan Mueller 
---
 crypto/algif_skcipher.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 43839b00fe6c..62449a8f14ce 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -139,8 +139,10 @@ static int skcipher_alloc_sgl(struct sock *sk)
sg_init_table(sgl->sg, MAX_SGL_ENTS + 1);
sgl->cur = 0;
 
-   if (sg)
+   if (sg) {
sg_chain(sg, MAX_SGL_ENTS + 1, sgl->sg);
+   sg_unmark_end(sg + (MAX_SGL_ENTS - 1));
+   }
 
list_add_tail(>list, >tsgl);
}
-- 
2.13.5

[PATCH] crypto: AF_ALG - update correct dst SGL entry

[Stephan Müller]

When two adjacent TX SGL are processed and parts of both TX SGLs
are pulled into the per-request TX SGL, the wrong per-request
TX SGL entries were updated.

This fixes a NULL pointer dereference when a cipher implementation walks
the TX SGL where some of the SGL entries were NULL.

Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index ffa9f4ccd9b4..337cf382718e 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -619,14 +619,14 @@ void af_alg_pull_tsgl(struct sock *sk, size_t used, 
struct scatterlist *dst,
struct af_alg_ctx *ctx = ask->private;
struct af_alg_tsgl *sgl;
struct scatterlist *sg;
-   unsigned int i, j;
+   unsigned int i, j = 0;
 
while (!list_empty(>tsgl_list)) {
sgl = list_first_entry(>tsgl_list, struct af_alg_tsgl,
   list);
sg = sgl->sg;
 
-   for (i = 0, j = 0; i < sgl->cur; i++) {
+   for (i = 0; i < sgl->cur; i++) {
size_t plen = min_t(size_t, used, sg[i].length);
struct page *page = sg_page(sg + i);
 
-- 
2.13.5

Re: [PATCH v8 3/4] crypto: AF_ALG -- add asymmetric cipher

[Stephan Müller]

Am Freitag, 11. August 2017, 14:51:10 CEST schrieb Tudor Ambarus:

Hi Tudor,

I have covered all your requests
> 
> > +   size_t used = 0;
> 
> initialization to zero not needed. You can directly initialize to
> ctx->used or don't initialize at all.

It is not initialized now. We cannot use ctx->used here as the socket (and 
thus the ctx data structure) is not locked yet.

> > +
> > +   /*
> > +* This error covers -EIOCBQUEUED which implies that we can
> > +* only handle one AIO request. If the caller wants to have
> > +* multiple AIO requests in parallel, he must make multiple
> > +* separate AIO calls.
> > +*/
> > +   if (err <= 0) {
> 
> why the equal?

We must get something out of the cipher operation as otherwise something is 
wrong. In this case I would like to error out to prevent an endless loop here.

> > +static int akcipher_setprivkey(void *private, const u8 *key,
> > +  unsigned int keylen)
> > +{
> > +   struct akcipher_tfm *tfm = private;
> > +   struct crypto_akcipher *akcipher = tfm->akcipher;
> > +   int err;
> > +
> > +   err = crypto_akcipher_set_priv_key(akcipher, key, keylen);
> > +   tfm->has_key = !err;
> > +
> > +   /* Return the maximum size of the akcipher operation. */
> > +   if (!err)
> > +   err = crypto_akcipher_maxsize(akcipher);
> 
> crypto subsystem returns zero when setkey is successful and introduces
> a new function for determining the maxsize. Should we comply with that?

The idea is that only when the the setting of the priv key fails, it returns 
the size of the expected privkey.

Which new function are you referring to?

Ciao
Stephan

[PATCH v4] crypto: only call put_page on referenced and used pages

[Stephan Müller]

Hi Herbert,

This patch was created against the current Linus development tree.

The functional test was conducted at the time v3 was aired. The patch
v4 is compile-tested.

Ciao
Stephan

---8<---
For asynchronous operation, SGs are allocated without a page mapped to
them or with a page that is not used (ref-counted). If the SGL is freed,
the code must only call put_page for an SG if there was a page assigned
and ref-counted in the first place.

This fixes a kernel crash when using io_submit with more than one iocb
using the sendmsg and sendpage (vmsplice/splice) interface.

Signed-off-by: Stephan Mueller 
---
 crypto/algif_skcipher.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 43839b00fe6c..903605dbc1a5 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -87,8 +87,13 @@ static void skcipher_free_async_sgls(struct 
skcipher_async_req *sreq)
}
sgl = sreq->tsg;
n = sg_nents(sgl);
-   for_each_sg(sgl, sg, n, i)
-   put_page(sg_page(sg));
+   for_each_sg(sgl, sg, n, i) {
+   struct page *page = sg_page(sg);
+
+   /* some SGs may not have a page mapped */
+   if (page && page_ref_count(page))
+   put_page(page);
+   }
 
kfree(sreq->tsg);
 }
-- 
2.13.4

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Stephan Müller]

Am Freitag, 11. August 2017, 07:13:30 CEST schrieb Marcel Holtmann:

Hi Marcel,

> > 
> > The last round of reviews for AF_ALG akcipher left off at an impasse
> > around a year ago: the consensus was that hardware key support was
> > needed, but that requirement was in conflict with the "always have a
> > software fallback" rule for the crypto subsystem. For example, a private
> > key securely generated by and stored in a TPM could not be copied out for
> > use by a software algorithm. Has anything come about to resolve this
> > impasse?
> > 
> > There were some patches around to add keyring support by associating a key
> > ID with an akcipher socket, but that approach ran in to a mismatch
> > between the proposed keyring API for the verify operation and the
> > semantics of AF_ALG verify.
> > 
> > AF_ALG is best suited for crypto use cases where a socket is set up once
> > and there are lots of reads and writes to justify the setup cost. With
> > asymmetric crypto, the setup cost is high when you might only use the
> > socket for a brief time to do one verify or encrypt operation.
> > 
> > Given the efficiency and hardware key issues, AF_ALG seems to be
> > mismatched with asymmetric crypto. Have you looked at the proposed
> > keyctl() support for crypto operations?
> we have also seen hardware now where the private key will never leave the
> crypto hardware. They public and private key is only generated for key
> exchange purposes and later on discarded again. Asymmetric ciphers are
> really not a good fit for AF_ALG and they should be solely supported via
> keyctl.

Thanks for the reminder. I have looked at that but I am unsure about whether 
this one covers asym crypto appropriately, too.

The issue is that some hardware may only offer accelerators without a full 
blown RSA siggen/ver logic (that pulls in PKCS or OAEP or others). How do you 
propose to cover raw primitives with keyctl?

Ciao
Stephan

[PATCH] crypto: AF_ALG - get_page upon reassignment to TX SGL

[Stephan Müller]

Hi Herbert,

The error can be triggered with the following test. Invoking that test
in a while [ 1 ] loop shows that no memory is leaked.

#include 
#include 

int main(int argc, char *argv[])
{
char buf[8192];
struct kcapi_handle *handle;
struct iovec iov;
int ret;

(void)argc;
(void)argv;

iov.iov_base = buf;

ret = kcapi_cipher_init(, "ctr(aes)", 0);
if (ret)
return ret;

ret = kcapi_cipher_setkey(handle, (unsigned char *)"0123456789abcdef", 
16);
if (ret)
return ret;

ret = kcapi_cipher_stream_init_enc(handle, (unsigned char 
*)"0123456789abcdef", NULL, 0);
if (ret < 0)
return ret;

iov.iov_len = 4152;
ret = kcapi_cipher_stream_update(handle, , 1);
if (ret < 0)
return ret;

iov.iov_len = 4096;
ret = kcapi_cipher_stream_op(handle, , 1);
if (ret < 0)
return ret;

kcapi_cipher_destroy(handle);

return 0;
}

---8<---

When a page is assigned to a TX SGL, call get_page to increment the
reference counter. It is possible that one page is referenced in
multiple SGLs:

- in the global TX SGL in case a previous af_alg_pull_tsgl only
reassigned parts of a page to a per-request TX SGL

- in the per-request TX SGL as assigned by af_alg_pull_tsgl

Note, multiple requests can be active at the same time whose TX SGLs all
point to different parts of the same page.

Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index d6936c0e08d9..ffa9f4ccd9b4 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -641,9 +641,9 @@ void af_alg_pull_tsgl(struct sock *sk, size_t used, struct 
scatterlist *dst,
if (dst_offset >= plen) {
/* discard page before offset */
dst_offset -= plen;
-   put_page(page);
} else {
/* reassign page to dst after offset */
+   get_page(page);
sg_set_page(dst + j, page,
plen - dst_offset,
sg[i].offset + dst_offset);
@@ -661,9 +661,7 @@ void af_alg_pull_tsgl(struct sock *sk, size_t used, struct 
scatterlist *dst,
if (sg[i].length)
return;
 
-   if (!dst)
-   put_page(page);
-
+   put_page(page);
sg_assign_page(sg + i, NULL);
}
 
-- 
2.13.4

Re: [PATCH v8 1/4] crypto: AF_ALG -- add sign/verify API

[Stephan Müller]

Am Donnerstag, 10. August 2017, 15:59:33 CEST schrieb Tudor Ambarus:

Hi Tudor,

> On 08/10/2017 04:03 PM, Stephan Mueller wrote:
> > Is there a style requirement for that? checkpatch.pl does not complain. I
> > thought that one liners in a conditional should not have braces?
> 
> Linux coding style requires braces in both branches when you have a
> branch with a statement and the other with multiple statements.
> 
> Checkpatch complains about this when you run it with --strict option.

Ok, then I will add it.

Thanks

Ciao
Stephan

[PATCH v8 3/4] crypto: AF_ALG -- add asymmetric cipher

[Stephan Müller]

This patch adds the user space interface for asymmetric ciphers. The
interface allows the use of sendmsg as well as vmsplice to provide data.

The akcipher interface implementation uses the common AF_ALG interface
code regarding TX and RX SGL handling.

Signed-off-by: Stephan Mueller 
---
 crypto/algif_akcipher.c | 466 
 include/crypto/if_alg.h |   2 +
 2 files changed, 468 insertions(+)
 create mode 100644 crypto/algif_akcipher.c

diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
new file mode 100644
index ..1b36eb0b6e8f
--- /dev/null
+++ b/crypto/algif_akcipher.c
@@ -0,0 +1,466 @@
+/*
+ * algif_akcipher: User-space interface for asymmetric cipher algorithms
+ *
+ * Copyright (C) 2017, Stephan Mueller 
+ *
+ * This file provides the user-space API for asymmetric ciphers.
+ *
+ * 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.
+ *
+ * The following concept of the memory management is used:
+ *
+ * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL is
+ * filled by user space with the data submitted via sendpage/sendmsg. Filling
+ * up the TX SGL does not cause a crypto operation -- the data will only be
+ * tracked by the kernel. Upon receipt of one recvmsg call, the caller must
+ * provide a buffer which is tracked with the RX SGL.
+ *
+ * During the processing of the recvmsg operation, the cipher request is
+ * allocated and prepared. As part of the recvmsg operation, the processed
+ * TX buffers are extracted from the TX SGL into a separate SGL.
+ *
+ * After the completion of the crypto operation, the RX SGL and the cipher
+ * request is released. The extracted TX SGL parts are released together with
+ * the RX SGL release.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct akcipher_tfm {
+   struct crypto_akcipher *akcipher;
+   bool has_key;
+};
+
+static int akcipher_sendmsg(struct socket *sock, struct msghdr *msg,
+   size_t size)
+{
+   return af_alg_sendmsg(sock, msg, size, 0);
+}
+
+static int _akcipher_recvmsg(struct socket *sock, struct msghdr *msg,
+size_t ignored, int flags)
+{
+   struct sock *sk = sock->sk;
+   struct alg_sock *ask = alg_sk(sk);
+   struct sock *psk = ask->parent;
+   struct alg_sock *pask = alg_sk(psk);
+   struct af_alg_ctx *ctx = ask->private;
+   struct akcipher_tfm *akc = pask->private;
+   struct crypto_akcipher *tfm = akc->akcipher;
+   struct af_alg_async_req *areq;
+   int err = 0;
+   int maxsize;
+   size_t len = 0;
+   size_t used = 0;
+
+   maxsize = crypto_akcipher_maxsize(tfm);
+   if (maxsize < 0)
+   return maxsize;
+
+   /* Allocate cipher request for current operation. */
+   areq = af_alg_alloc_areq(sk, sizeof(struct af_alg_async_req) +
+crypto_akcipher_reqsize(tfm));
+   if (IS_ERR(areq))
+   return PTR_ERR(areq);
+
+   /* convert iovecs of output buffers into RX SGL */
+   err = af_alg_get_rsgl(sk, msg, flags, areq, maxsize, );
+   if (err)
+   goto free;
+
+   /* ensure output buffer is sufficiently large */
+   if (len < maxsize) {
+   err = -EMSGSIZE;
+   goto free;
+   }
+
+   /*
+* Create a per request TX SGL for this request which tracks the
+* SG entries from the global TX SGL.
+*/
+   used = ctx->used;
+   areq->tsgl_entries = af_alg_count_tsgl(sk, used, 0);
+   if (!areq->tsgl_entries)
+   areq->tsgl_entries = 1;
+   areq->tsgl = sock_kmalloc(sk, sizeof(*areq->tsgl) * areq->tsgl_entries,
+ GFP_KERNEL);
+   if (!areq->tsgl) {
+   err = -ENOMEM;
+   goto free;
+   }
+   sg_init_table(areq->tsgl, areq->tsgl_entries);
+   af_alg_pull_tsgl(sk, used, areq->tsgl, 0);
+
+   /* Initialize the crypto operation */
+   akcipher_request_set_tfm(>cra_u.akcipher_req, tfm);
+   akcipher_request_set_crypt(>cra_u.akcipher_req, areq->tsgl,
+  areq->first_rsgl.sgl.sg, used, len);
+
+   if (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) {
+   /* AIO operation */
+   areq->iocb = msg->msg_iocb;
+   akcipher_request_set_callback(>cra_u.akcipher_req,
+ CRYPTO_TFM_REQ_MAY_SLEEP,
+ af_alg_async_cb, areq);
+   } else
+   /* Synchronous operation */
+   

[PATCH v8 2/4] crypto: AF_ALG -- add setpubkey setsockopt call

[Stephan Müller]

For supporting asymmetric ciphers, user space must be able to set the
public key. The patch adds a new setsockopt call for setting the public
key.

Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 18 +-
 include/crypto/if_alg.h |  1 +
 include/uapi/linux/if_alg.h |  1 +
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index a35a9f854a04..176921d7593a 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -203,13 +203,17 @@ static int alg_bind(struct socket *sock, struct sockaddr 
*uaddr, int addr_len)
 }
 
 static int alg_setkey(struct sock *sk, char __user *ukey,
- unsigned int keylen)
+ unsigned int keylen,
+ int (*setkey)(void *private, const u8 *key,
+   unsigned int keylen))
 {
struct alg_sock *ask = alg_sk(sk);
-   const struct af_alg_type *type = ask->type;
u8 *key;
int err;
 
+   if (!setkey)
+   return -ENOPROTOOPT;
+
key = sock_kmalloc(sk, keylen, GFP_KERNEL);
if (!key)
return -ENOMEM;
@@ -218,7 +222,7 @@ static int alg_setkey(struct sock *sk, char __user *ukey,
if (copy_from_user(key, ukey, keylen))
goto out;
 
-   err = type->setkey(ask->private, key, keylen);
+   err = setkey(ask->private, key, keylen);
 
 out:
sock_kzfree_s(sk, key, keylen);
@@ -248,10 +252,14 @@ static int alg_setsockopt(struct socket *sock, int level, 
int optname,
case ALG_SET_KEY:
if (sock->state == SS_CONNECTED)
goto unlock;
-   if (!type->setkey)
+
+   err = alg_setkey(sk, optval, optlen, type->setkey);
+   break;
+   case ALG_SET_PUBKEY:
+   if (sock->state == SS_CONNECTED)
goto unlock;
 
-   err = alg_setkey(sk, optval, optlen);
+   err = alg_setkey(sk, optval, optlen, type->setpubkey);
break;
case ALG_SET_AEAD_AUTHSIZE:
if (sock->state == SS_CONNECTED)
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 50a21488f3ba..d1de8ed3e77b 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -55,6 +55,7 @@ struct af_alg_type {
void *(*bind)(const char *name, u32 type, u32 mask);
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+   int (*setpubkey)(void *private, const u8 *key, unsigned int keylen);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index d81dcca5bdd7..02e61627e089 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -34,6 +34,7 @@ struct af_alg_iv {
 #define ALG_SET_OP 3
 #define ALG_SET_AEAD_ASSOCLEN  4
 #define ALG_SET_AEAD_AUTHSIZE  5
+#define ALG_SET_PUBKEY 6
 
 /* Operations */
 #define ALG_OP_DECRYPT 0
-- 
2.13.4

[PATCH v8 4/4] crypto: algif_akcipher - enable compilation

[Stephan Müller]

Add the Makefile and Kconfig updates to allow algif_akcipher to be
compiled.

Signed-off-by: Stephan Mueller 
---
 crypto/Kconfig  | 9 +
 crypto/Makefile | 1 +
 2 files changed, 10 insertions(+)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 0a121f9ddf8e..fdcec68545f3 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1760,6 +1760,15 @@ config CRYPTO_USER_API_AEAD
  This option enables the user-spaces interface for AEAD
  cipher algorithms.
 
+config CRYPTO_USER_API_AKCIPHER
+   tristate "User-space interface for asymmetric key cipher algorithms"
+   depends on NET
+   select CRYPTO_AKCIPHER2
+   select CRYPTO_USER_API
+   help
+ This option enables the user-spaces interface for asymmetric
+ key cipher algorithms.
+
 config CRYPTO_HASH_INFO
bool
 
diff --git a/crypto/Makefile b/crypto/Makefile
index d41f0331b085..12dbf2c5fe7c 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -133,6 +133,7 @@ obj-$(CONFIG_CRYPTO_USER_API_HASH) += algif_hash.o
 obj-$(CONFIG_CRYPTO_USER_API_SKCIPHER) += algif_skcipher.o
 obj-$(CONFIG_CRYPTO_USER_API_RNG) += algif_rng.o
 obj-$(CONFIG_CRYPTO_USER_API_AEAD) += algif_aead.o
+obj-$(CONFIG_CRYPTO_USER_API_AKCIPHER) += algif_akcipher.o
 
 ecdh_generic-y := ecc.o
 ecdh_generic-y += ecdh.o
-- 
2.13.4

[PATCH v8 0/4] crypto: add algif_akcipher user space API

[Stephan Müller]

Hi,

This patch set adds the AF_ALG user space API to externalize the
asymmetric cipher API recently added to the kernel crypto API.

The patch set is tested with the user space library of libkcapi [1].
Use [1] test/test.sh for a full test run. The test covers the
following scenarios:

* sendmsg of one IOVEC

* sendmsg of 16 IOVECs with non-linear buffer

* vmsplice of one IOVEC

* vmsplice of 15 IOVECs with non-linear buffer

* invoking multiple separate cipher operations with one
  open cipher handle

* encryption with private key (using vector from testmgr.h)

* encryption with public key (using vector from testmgr.h)

* decryption with private key (using vector from testmgr.h)

Note, to enable the test, edit line [2] from "4 99" to "4 13".

[1] http://www.chronox.de/libkcapi.html
[2] https://github.com/smuellerDD/libkcapi/blob/master/test/test.sh#L1452

Changes v8:
 * port to kernel 4.13
 * port to consolidated AF_ALG code

Stephan Mueller (4):
  crypto: AF_ALG -- add sign/verify API
  crypto: AF_ALG -- add setpubkey setsockopt call
  crypto: AF_ALG -- add asymmetric cipher
  crypto: algif_akcipher - enable compilation

 crypto/Kconfig  |   9 +
 crypto/Makefile |   1 +
 crypto/af_alg.c |  28 ++-
 crypto/algif_aead.c |  36 ++--
 crypto/algif_akcipher.c | 466 
 crypto/algif_skcipher.c |  26 ++-
 include/crypto/if_alg.h |   7 +-
 include/uapi/linux/if_alg.h |   3 +
 8 files changed, 543 insertions(+), 33 deletions(-)
 create mode 100644 crypto/algif_akcipher.c

-- 
2.13.4

[PATCH v8 1/4] crypto: AF_ALG -- add sign/verify API

[Stephan Müller]

Add the flags for handling signature generation and signature
verification.

The af_alg helper code as well as the algif_skcipher and algif_aead code
must be changed from a boolean indicating the cipher operation to an
integer because there are now 4 different cipher operations that are
defined. Yet, the algif_aead and algif_skcipher code still only allows
encryption and decryption cipher operations.

Signed-off-by: Stephan Mueller 
Signed-off-by: Tadeusz Struk 
---
 crypto/af_alg.c | 10 +-
 crypto/algif_aead.c | 36 
 crypto/algif_skcipher.c | 26 +-
 include/crypto/if_alg.h |  4 ++--
 include/uapi/linux/if_alg.h |  2 ++
 5 files changed, 50 insertions(+), 28 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index d6936c0e08d9..a35a9f854a04 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -859,7 +859,7 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, 
size_t size,
struct af_alg_tsgl *sgl;
struct af_alg_control con = {};
long copied = 0;
-   bool enc = 0;
+   int op = 0;
bool init = 0;
int err = 0;
 
@@ -870,11 +870,11 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr 
*msg, size_t size,
 
init = 1;
switch (con.op) {
+   case ALG_OP_VERIFY:
+   case ALG_OP_SIGN:
case ALG_OP_ENCRYPT:
-   enc = 1;
-   break;
case ALG_OP_DECRYPT:
-   enc = 0;
+   op = con.op;
break;
default:
return -EINVAL;
@@ -891,7 +891,7 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, 
size_t size,
}
 
if (init) {
-   ctx->enc = enc;
+   ctx->op = op;
if (con.iv)
memcpy(ctx->iv, con.iv->iv, ivsize);
 
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 516b38c3a169..77abc04cf942 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -60,7 +60,7 @@ static inline bool aead_sufficient_data(struct sock *sk)
 * The minimum amount of memory needed for an AEAD cipher is
 * the AAD and in case of decryption the tag.
 */
-   return ctx->used >= ctx->aead_assoclen + (ctx->enc ? 0 : as);
+   return ctx->used >= ctx->aead_assoclen + (ctx->op ? 0 : as);
 }
 
 static int aead_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
@@ -137,7 +137,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr 
*msg,
 * buffer provides the tag which is consumed resulting in only the
 * plaintext without a buffer for the tag returned to the caller.
 */
-   if (ctx->enc)
+   if (ctx->op)
outlen = used + as;
else
outlen = used - as;
@@ -196,7 +196,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr 
*msg,
/* Use the RX SGL as source (and destination) for crypto op. */
src = areq->first_rsgl.sgl.sg;
 
-   if (ctx->enc) {
+   if (ctx->op == ALG_OP_ENCRYPT) {
/*
 * Encryption operation - The in-place cipher operation is
 * achieved by the following operation:
@@ -212,7 +212,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr 
*msg,
if (err)
goto free;
af_alg_pull_tsgl(sk, processed, NULL, 0);
-   } else {
+   } else if (ctx->op == ALG_OP_DECRYPT) {
/*
 * Decryption operation - To achieve an in-place cipher
 * operation, the following  SGL structure is used:
@@ -258,6 +258,9 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr 
*msg,
} else
/* no RX SGL present (e.g. authentication only) */
src = areq->tsgl;
+   } else {
+   err = -EOPNOTSUPP;
+   goto free;
}
 
/* Initialize the crypto operation */
@@ -272,19 +275,28 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
aead_request_set_callback(>cra_u.aead_req,
  CRYPTO_TFM_REQ_MAY_BACKLOG,
  af_alg_async_cb, areq);
-   err = ctx->enc ? crypto_aead_encrypt(>cra_u.aead_req) :
-crypto_aead_decrypt(>cra_u.aead_req);
-   } else {
+   } else
/* Synchronous operation */
aead_request_set_callback(>cra_u.aead_req,
  CRYPTO_TFM_REQ_MAY_BACKLOG,
  af_alg_complete, >completion);
-   err = af_alg_wait_for_completion(ctx->enc ?
-

[PATCH] crypto: MPI - kunmap after finishing accessing buffer

[Stephan Müller]

Hi Herbert,

I found that issue while playing around with edge conditions in my
algif_akcipher implementation. This issue only manifests in a
segmentation violation on 32 bit machines and with an SGL where each
SG points to one byte. SGLs with larger buffers seem to be not
affected by this issue.

Yet this access-after-unmap should be a candidate for stable, IMHO.

---8<---

Using sg_miter_start and sg_miter_next, the buffer of an SG is kmap'ed
to *buff. The current code calls sg_miter_stop (and thus kunmap) on the
SG entry before the last access of *buff.

The patch moves the sg_miter_stop call after the last access to *buff to
ensure that the memory pointed to by *buff is still mapped.

Signed-off-by: Stephan Mueller 
---
 lib/mpi/mpicoder.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 5a0f75a3bf01..eead4b339466 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -364,11 +364,11 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, 
unsigned int nbytes)
}
 
miter.consumed = lzeros;
-   sg_miter_stop();
 
nbytes -= lzeros;
nbits = nbytes * 8;
if (nbits > MAX_EXTERN_MPI_BITS) {
+   sg_miter_stop();
pr_info("MPI: mpi too large (%u bits)\n", nbits);
return NULL;
}
@@ -376,6 +376,8 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned 
int nbytes)
if (nbytes > 0)
nbits -= count_leading_zeros(*buff) - (BITS_PER_LONG - 8);
 
+   sg_miter_stop();
+
nlimbs = DIV_ROUND_UP(nbytes, BYTES_PER_MPI_LIMB);
val = mpi_alloc(nlimbs);
if (!val)
-- 
2.13.4

[PATCH] crypto: algif_aead - fix comment regarding memory layout

[Stephan Müller]

Signed-off-by: Stephan Mueller 
---
 crypto/algif_aead.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 48d46e74ed0d..516b38c3a169 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -201,11 +201,11 @@ static int _aead_recvmsg(struct socket *sock, struct 
msghdr *msg,
 * Encryption operation - The in-place cipher operation is
 * achieved by the following operation:
 *
-* TX SGL: AAD || PT || Tag
+* TX SGL: AAD || PT
 *  |  |
 *  | copy |
 *  v  v
-* RX SGL: AAD || PT
+* RX SGL: AAD || PT || Tag
 */
err = crypto_aead_copy_sgl(null_tfm, tsgl->sg,
   areq->first_rsgl.sgl.sg, processed);
-- 
2.13.4

[PATCH v2] crypto: AF_ALG - consolidation of duplicate code

[Stephan Müller]

Hi Herbert,

as agreed, the individual patches from the first submission are now changed.

After review of the changes I had to apply to algif_aead and algif_skcipher,
I saw that they are all in the category that you agreed that can be rolled
into this patch. Though, I documented the changes so that a review should
be easier.

Ciao
Stephan

---8<---

Consolidate following data structures:

skcipher_async_req, aead_async_req -> af_alg_async_req
skcipher_rsgl, aead_rsql -> af_alg_rsgl
skcipher_tsgl, aead_tsql -> af_alg_tsgl
skcipher_ctx, aead_ctx -> af_alg_ctx

Consolidate following functions:

skcipher_sndbuf, aead_sndbuf -> af_alg_sndbuf
skcipher_writable, aead_writable -> af_alg_writable
skcipher_rcvbuf, aead_rcvbuf -> af_alg_rcvbuf
skcipher_readable, aead_readable -> af_alg_readable
aead_alloc_tsgl, skcipher_alloc_tsgl -> af_alg_alloc_tsgl
aead_count_tsgl, skcipher_count_tsgl -> af_alg_count_tsgl
aead_pull_tsgl, skcipher_pull_tsgl -> af_alg_pull_tsgl
aead_free_areq_sgls, skcipher_free_areq_sgls -> af_alg_free_areq_sgls
aead_wait_for_wmem, skcipher_wait_for_wmem -> af_alg_wait_for_wmem
aead_wmem_wakeup, skcipher_wmem_wakeup -> af_alg_wmem_wakeup
aead_wait_for_data, skcipher_wait_for_data -> af_alg_wait_for_data
aead_data_wakeup, skcipher_data_wakeup -> af_alg_data_wakeup
aead_sendmsg, skcipher_sendmsg -> af_alg_sendmsg
aead_sendpage, skcipher_sendpage -> af_alg_sendpage
aead_async_cb, skcipher_async_cb -> af_alg_async_cb
aead_poll, skcipher_poll -> af_alg_poll

Split out the following common code from recvmsg:

af_alg_alloc_areq: allocation of the request data structure for the
cipher operation

af_alg_get_rsgl: creation of the RX SGL anchored in the request data
structure

The following changes to the implementation without affecting the
functionality have been applied to synchronize slightly different code
bases in algif_skcipher and algif_aead:

The wakeup in af_alg_wait_for_data is triggered when either more data
is received or the indicator that more data is to be expected is
released. The first is triggered by user space, the second is
triggered by the kernel upon finishing the processing of data
(i.e. the kernel is ready for more).

af_alg_sendmsg uses size_t in min_t calculation for obtaining len.
Return code determination is consistent with algif_skcipher. The
scope of the variable i is reduced to match algif_aead. The type of the
variable i is switched from int to unsigned int to match algif_aead.

af_alg_sendpage does not contain the superfluous err = 0 from
aead_sendpage.

af_alg_async_cb requires to store the number of output bytes in
areq->outlen before the AIO callback is triggered.

The POLLIN / POLLRDNORM is now set when either not more data is given or
the kernel is supplied with data. This is consistent to the wakeup from
sleep when the kernel waits for data.

The request data structure is extended by the field last_rsgl which
points to the last RX SGL list entry. This shall help recvmsg
implementation to chain the RX SGL to other SG(L)s if needed. It is
currently used by algif_aead which chains the tag SGL to the RX SGL
during decryption.

Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 693 +++
 crypto/algif_aead.c | 701 +++-
 crypto/algif_skcipher.c | 638 +++
 include/crypto/if_alg.h | 170 
 4 files changed, 940 insertions(+), 1262 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 92a3d540d920..d6936c0e08d9 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 struct alg_type_list {
@@ -507,6 +508,698 @@ void af_alg_complete(struct crypto_async_request *req, 
int err)
 }
 EXPORT_SYMBOL_GPL(af_alg_complete);
 
+/**
+ * af_alg_alloc_tsgl - allocate the TX SGL
+ *
+ * @sk socket of connection to user space
+ * @return: 0 upon success, < 0 upon error
+ */
+int af_alg_alloc_tsgl(struct sock *sk)
+{
+   struct alg_sock *ask = alg_sk(sk);
+   struct af_alg_ctx *ctx = ask->private;
+   struct af_alg_tsgl *sgl;
+   struct scatterlist *sg = NULL;
+
+   sgl = list_entry(ctx->tsgl_list.prev, struct af_alg_tsgl, list);
+   if (!list_empty(>tsgl_list))
+   sg = sgl->sg;
+
+   if (!sg || sgl->cur >= MAX_SGL_ENTS) {
+   sgl = sock_kmalloc(sk, sizeof(*sgl) +
+  sizeof(sgl->sg[0]) * (MAX_SGL_ENTS + 1),
+  GFP_KERNEL);
+   if (!sgl)
+   return -ENOMEM;
+
+   sg_init_table(sgl->sg, MAX_SGL_ENTS + 1);
+   sgl->cur = 0;
+
+   if (sg)
+   sg_chain(sg, MAX_SGL_ENTS + 1, sgl->sg);
+
+   list_add_tail(>list, >tsgl_list);
+   }
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(af_alg_alloc_tsgl);
+
+/**
+ * aead_count_tsgl - Count 

Re: [PATCH 00/16] crypto: AF_ALG - consolidation

[Stephan Müller]

Am Dienstag, 1. August 2017, 11:08:33 CEST schrieb Herbert Xu:

Hi Herbert,
> 
> How about you separate into three patches? The first two make
> changes to algif_skcipher/algif_aead so that they can be merged,
> and the last one does the actual merging.
> 
Ok, I can do that.

How would you want to handle the following: the new af_alg_wait_for_data will 
include:

if (sk_wait_event(sk, , (ctx->used || !ctx->more)

whereas algif_aead only contains !ctx->more and algif_skcipher only contains 
ctx->used in the check? When merging both, having the common check is good for 
both, but not needed individually.

Shall I only make the change during the merger or add these changes to the 
modification of algif_skcipher/algif_aead?

Please then also disregard the recvmsg consolidation patch from this morning.

Ciao
Stephan

Re: [PATCH 00/16] crypto: AF_ALG - consolidation

[Stephan Müller]

Am Dienstag, 1. August 2017, 10:58:58 CEST schrieb Herbert Xu:

Hi Herbert,
> 
> This split makes no sense.  I don't see why you can't merge them
> into a single patch if it's just rearranging the code.

If you want to merge all into one patch, I am fine.

I thought that separating all out makes the patch review easier, considering 
that in two or three of the patches, small changes are implemented to allow a 
common code base. These changes, however, should not have any functional 
effect.

Ciao
Stephan