bug in blkcipher_walk code

bug in blkcipher_walk code

[Stephan Mueller]

Hi Herbert,

Once in a while I seem to trigger a bug in the blkcipher_walk code which I 
cannot track down. This bug happens sporadically where I assume that it has 
something to do with the memory management in the slow path of blkcipher_walk.

I am using the CTR DRBG code that in turn uses the ctr-aes-aesni 
implementation. The bug only appears when I want to obtain a random number 
that is less than the CTR AES block size. In my particular case, I want 4 
bytes from the DRBG.

The bug happens in arch/x86/crypto/aesni-intel_glue.c:ctr_crypt_final() at the 
line:

	memcpy(dst, keystream, nbytes);

The bug looks like the following:

[   12.328676] BUG: unable to handle kernel paging request at ffffa17ae418b988
[   12.328680] IP: [<ffffffff82060eea>] ctr_crypt+0x19a/0x1c0
[   12.328681] PGD 66fed067
[   12.328681] PUD 0
[   12.328681]
[   12.328683] Oops: 0002 [#1] SMP
[   12.328692] Modules linked in: bridge(+) stp llc ebtable_nat ip6table_raw 
ip6table_security ip6table_mangle iptable_raw iptable_security iptable_mangle 
ebtable_filter ebtables ip6table_filter ip6_tables crct10dif_pclmul 
crc32_pclmul ghash_clmulni_intel pcspkr i2c_piix4 virtio_net virtio_balloon 
acpi_cpufreq sch_fq_codel virtio_console virtio_blk virtio_pci virtio_ring 
serio_raw crc32c_intel virtio
[   12.328693] CPU: 0 PID: 521 Comm: modprobe Not tainted 4.9.0-rc1+ #253
[   12.328694] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.9.1-1.fc24 04/01/2014
[   12.328694] task: ffffa17ab8453fc0 task.stack: ffffbdafc0744000
[   12.328696] RIP: 0010:[<ffffffff82060eea>]  [<ffffffff82060eea>] ctr_crypt
+0x19a/0x1c0
[   12.328696] RSP: 0018:ffffbdafc0747a60  EFLAGS: 00010002
[   12.328697] RAX: 0000000032e455a6 RBX: 0000000000000004 RCX: 
0000000000000002
[   12.328697] RDX: 0000000000000001 RSI: 0000000000000086 RDI: 
0000000000000086
[   12.328698] RBP: ffffbdafc0747b28 R08: ffffa17abc16e900 R09: 
0000000000000019
[   12.328698] R10: ffffa17a764f68b0 R11: 000000000002e918 R12: 
ffffbdafc0747b38
[   12.328698] R13: ffffa17a764f6840 R14: ffffa17ae418b988 R15: 
ffffbdafc0747a70
[   12.328699] FS:  00007f55f57a6700(0000) GS:ffffa17abfc00000(0000) knlGS:
0000000000000000
[   12.328700] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   12.328700] CR2: ffffa17ae418b988 CR3: 0000000079b26000 CR4: 
00000000003406f0
[   12.328703] Stack:
[   12.328705]  ffffa17abc16e900 ffffa17ab845fd80 2ae7e40732e455a6 
3a224612a8f9841d
[   12.328706]  fffffb4e81e117c0 ffffa17ab845fd80 fffffb4e829062c0 
ffffa17ae418b988
[   12.328707]  ffffbdafc0747ba8 ffffffff00000d80 ffffffff00000004 
ffffbdafc0747bc8
[   12.328708] Call Trace:
[   12.328712]  [<ffffffff823e5fd3>] __ablk_encrypt+0x43/0x50
[   12.328714]  [<ffffffff823e6012>] ablk_encrypt+0x32/0xc0
[   12.328716]  [<ffffffff823c4f2e>] skcipher_encrypt_ablkcipher+0x5e/0x60
[   12.328717]  [<ffffffff823dbb80>] drbg_kcapi_sym_ctr+0xb0/0x130
[   12.328719]  [<ffffffff823de153>] drbg_ctr_generate+0x53/0x80


Now, the interesting part is the following: the original memory pointer that 
shall be processed by the DRBG is in my example ffffffffc018b988 -- this 
pointer is used until the DRBG invokes crypto_skcipher_encrypt. However, when 
I print out the buffer pointer that is used as dst in the memcpy of 
ctr_crypt_final, I see ffffa17ae418b988 -- i.e. the buffer that causes paging 
failure.

During tracing the blkcipher_walk code I see that the slow code path is used 
when the request size is smaller than the block size. That slow code path 
allocates new memory that will be used for the dst pointer in ctr_crypt_final.

May I ask you for checking whether the allocation and the memory pointer logic 
has an issue that would cause a paging failure?

Ciao
Stephan

Re: bug in blkcipher_walk code

[Stephan Mueller]

Am Freitag, 18. November 2016, 12:31:10 CET schrieb Stephan Mueller:

Hi Herbert,

> Hi Herbert,
> 
> Once in a while I seem to trigger a bug in the blkcipher_walk code which I
> cannot track down. This bug happens sporadically where I assume that it has
> something to do with the memory management in the slow path of
> blkcipher_walk.
> 
> I am using the CTR DRBG code that in turn uses the ctr-aes-aesni
> implementation. The bug only appears when I want to obtain a random number
> that is less than the CTR AES block size. In my particular case, I want 4
> bytes from the DRBG.
> 
> The bug happens in arch/x86/crypto/aesni-intel_glue.c:ctr_crypt_final() at
> the line:
> 
> 	memcpy(dst, keystream, nbytes);
> 
> The bug looks like the following:
> 
> [   12.328676] BUG: unable to handle kernel paging request at
> ffffa17ae418b988 [   12.328680] IP: [<ffffffff82060eea>]
> ctr_crypt+0x19a/0x1c0
> [   12.328681] PGD 66fed067
> [   12.328681] PUD 0
> [   12.328681]
> [   12.328683] Oops: 0002 [#1] SMP
> [   12.328692] Modules linked in: bridge(+) stp llc ebtable_nat ip6table_raw
> ip6table_security ip6table_mangle iptable_raw iptable_security
> iptable_mangle ebtable_filter ebtables ip6table_filter ip6_tables
> crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcspkr i2c_piix4
> virtio_net virtio_balloon acpi_cpufreq sch_fq_codel virtio_console
> virtio_blk virtio_pci virtio_ring serio_raw crc32c_intel virtio
> [   12.328693] CPU: 0 PID: 521 Comm: modprobe Not tainted 4.9.0-rc1+ #253
> [   12.328694] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.9.1-1.fc24 04/01/2014
> [   12.328694] task: ffffa17ab8453fc0 task.stack: ffffbdafc0744000
> [   12.328696] RIP: 0010:[<ffffffff82060eea>]  [<ffffffff82060eea>]
> ctr_crypt +0x19a/0x1c0
> [   12.328696] RSP: 0018:ffffbdafc0747a60  EFLAGS: 00010002
> [   12.328697] RAX: 0000000032e455a6 RBX: 0000000000000004 RCX:
> 0000000000000002
> [   12.328697] RDX: 0000000000000001 RSI: 0000000000000086 RDI:
> 0000000000000086
> [   12.328698] RBP: ffffbdafc0747b28 R08: ffffa17abc16e900 R09:
> 0000000000000019
> [   12.328698] R10: ffffa17a764f68b0 R11: 000000000002e918 R12:
> ffffbdafc0747b38
> [   12.328698] R13: ffffa17a764f6840 R14: ffffa17ae418b988 R15:
> ffffbdafc0747a70
> [   12.328699] FS:  00007f55f57a6700(0000) GS:ffffa17abfc00000(0000) knlGS:
> 0000000000000000
> [   12.328700] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   12.328700] CR2: ffffa17ae418b988 CR3: 0000000079b26000 CR4:
> 00000000003406f0
> [   12.328703] Stack:
> [   12.328705]  ffffa17abc16e900 ffffa17ab845fd80 2ae7e40732e455a6
> 3a224612a8f9841d
> [   12.328706]  fffffb4e81e117c0 ffffa17ab845fd80 fffffb4e829062c0
> ffffa17ae418b988
> [   12.328707]  ffffbdafc0747ba8 ffffffff00000d80 ffffffff00000004
> ffffbdafc0747bc8
> [   12.328708] Call Trace:
> [   12.328712]  [<ffffffff823e5fd3>] __ablk_encrypt+0x43/0x50
> [   12.328714]  [<ffffffff823e6012>] ablk_encrypt+0x32/0xc0
> [   12.328716]  [<ffffffff823c4f2e>] skcipher_encrypt_ablkcipher+0x5e/0x60
> [   12.328717]  [<ffffffff823dbb80>] drbg_kcapi_sym_ctr+0xb0/0x130
> [   12.328719]  [<ffffffff823de153>] drbg_ctr_generate+0x53/0x80
> 
> 
> Now, the interesting part is the following: the original memory pointer that
> shall be processed by the DRBG is in my example ffffffffc018b988 -- this
> pointer is used until the DRBG invokes crypto_skcipher_encrypt. However,
> when I print out the buffer pointer that is used as dst in the memcpy of
> ctr_crypt_final, I see ffffa17ae418b988 -- i.e. the buffer that causes
> paging failure.
> 
> During tracing the blkcipher_walk code I see that the slow code path is used
> when the request size is smaller than the block size. That slow code path
> allocates new memory that will be used for the dst pointer in
> ctr_crypt_final.
> 
> May I ask you for checking whether the allocation and the memory pointer
> logic has an issue that would cause a paging failure?

Following up this issue, I found the location where the wrong memory pointer 
is produced -- the following call tree is used:

1. set up of SGL with proper pointer

2. skcipher_encrypt_ablkcipher with SGL

3. invocation of ctr_crypt from arch/x86/crypto/aesni-intel_glue.c

4. blkcipher_walk_virt_block

5. blkcipher_walk_first

6. blkcipher_walk_next (this code does not use the code path to allocate a 
page)

7. blkcipher_next_fast

        walk->dst.virt.addr = walk->src.virt.addr;
			-> copy src virt address into dst address pointer

		Now, the diff path is used:
        if (diff) {
                walk->flags |= BLKCIPHER_WALK_DIFF;
                blkcipher_map_dst(walk);
        }

8. blkcipher_map_dst

        walk->dst.virt.addr = scatterwalk_map(&walk->out);

		==> this pointer is wrong

The interesting point is that step 8 gets the low and high bits right, but not 
the bits in the middle:

The real data pointer for the dst buffer is ffffffffc0332988. The data pointer 
used by the crypto API is ffff96a995332988 -- as often as I see the issue, 
this similarity in the pointer values is always there.


Please note that the caller uses a static variable that shall be used as dst 
buffer.

Thanks
Stephan

[PATCH] crypto: CTR DRBG - prevent invalid SG mappings

[Stephan Mueller]

Hi Herbert,

as discussed in another thread, SGs must not be used with stack memory 
pointers. This issue was the culprit to the error I see with the CTR DRBG. The 
attached patch fixes the issue.

---8<---

When using SGs, only heap memory (memory that is valid as per
virt_addr_valid) is allowed to be referenced. The CTR DRBG used to
reference the caller-provided memory directly in an SG. In case the
caller provided stack memory pointers, the SG mapping is not considered
to be valid. In some cases, this would even cause a paging fault.

The change adds a new scratch buffer that is used in case the
caller-provided buffer is deemed not suitable for use in an SG. The
crypto operation of the CTR DRBG produces its output with that scratch
buffer.

The scratch buffer is allocated during allocation time of the CTR DRBG
as its access is protected with the DRBG mutex.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/drbg.c         | 35 +++++++++++++++++++++++++++++++----
 include/crypto/drbg.h |  2 ++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 9a95b61..cbbd19f 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -262,6 +262,7 @@ 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
 
 /* BCC function for CTR DRBG as defined in 10.4.3 */
 static int drbg_ctr_bcc(struct drbg_state *drbg,
@@ -1644,6 +1645,9 @@ static int drbg_fini_sym_kernel(struct drbg_state *drbg)
 	kfree(drbg->ctr_null_value_buf);
 	drbg->ctr_null_value = NULL;
 
+	kfree(drbg->outscratchpadbuf);
+	drbg->outscratchpadbuf = NULL;
+
 	return 0;
 }
 
@@ -1708,6 +1712,15 @@ static int drbg_init_sym_kernel(struct drbg_state 
*drbg)
 	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) {
+		drbg_fini_sym_kernel(drbg);
+		return -ENOMEM;
+	}
+	drbg->outscratchpad = (u8 *)PTR_ALIGN(drbg->outscratchpadbuf,
+					      alignmask + 1);
+
 	return alignmask;
 }
 
@@ -1737,15 +1750,22 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
 			      u8 *outbuf, u32 outlen)
 {
 	struct scatterlist sg_in;
+	bool virt_addr_valid = virt_addr_valid(outbuf);
+	int ret = 0;
 
 	sg_init_one(&sg_in, inbuf, inlen);
 
 	while (outlen) {
 		u32 cryptlen = min_t(u32, inlen, outlen);
 		struct scatterlist sg_out;
-		int ret;
 
-		sg_init_one(&sg_out, outbuf, cryptlen);
+		/* If output buffer is not valid for SGL, use scratchpad */
+		if (virt_addr_valid)
+			sg_init_one(&sg_out, outbuf, cryptlen);
+		else {
+			cryptlen = min_t(u32, cryptlen, DRBG_OUTSCRATCHLEN);
+			sg_init_one(&sg_out, drbg->outscratchpad, cryptlen);
+		}
 		skcipher_request_set_crypt(drbg->ctr_req, &sg_in, &sg_out,
 					   cryptlen, drbg->V);
 		ret = crypto_skcipher_encrypt(drbg->ctr_req);
@@ -1761,15 +1781,22 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
 				break;
 			}
 		default:
-			return ret;
+			goto out;
 		}
 		init_completion(&drbg->ctr_completion);
 
+		if (!virt_addr_valid)
+			memcpy(outbuf, drbg->outscratchpad, cryptlen);
+
 		outlen -= cryptlen;
 		outbuf += cryptlen;
 	}
+	ret = 0;
 
-	return 0;
+out:
+	if (!virt_addr_valid)
+		memzero_explicit(drbg->outscratchpad, DRBG_OUTSCRATCHLEN);
+	return ret;
 }
 #endif /* CONFIG_CRYPTO_DRBG_CTR */
 
diff --git a/include/crypto/drbg.h b/include/crypto/drbg.h
index 61580b1..22f884c 100644
--- a/include/crypto/drbg.h
+++ b/include/crypto/drbg.h
@@ -124,6 +124,8 @@ struct drbg_state {
 	struct skcipher_request *ctr_req;	/* CTR mode request handle */
 	__u8 *ctr_null_value_buf;		/* CTR mode unaligned buffer */
 	__u8 *ctr_null_value;			/* CTR mode aligned zero buf */
+	__u8 *outscratchpadbuf;			/* CTR mode output scratchpad */
+        __u8 *outscratchpad;			/* CTR mode aligned outbuf */
 	struct completion ctr_completion;	/* CTR mode async handler */
 	int ctr_async_err;			/* CTR mode async error */
 
-- 
2.9.3

Re: [PATCH] crypto: CTR DRBG - prevent invalid SG mappings

[Herbert Xu]

On Sat, Nov 26, 2016 at 09:54:14AM +0100, Stephan Mueller wrote:
> Hi Herbert,
> 
> as discussed in another thread, SGs must not be used with stack memory 
> pointers. This issue was the culprit to the error I see with the CTR DRBG. The 
> attached patch fixes the issue.

Sorry but your patch is corrupted.  Please resend.

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

[PATCH RESEND] crypto: CTR DRBG - prevent invalid SG mappings

[Stephan Mueller]

When using SGs, only heap memory (memory that is valid as per
virt_addr_valid) is allowed to be referenced. The CTR DRBG used to
reference the caller-provided memory directly in an SG. In case the
caller provided stack memory pointers, the SG mapping is not considered
to be valid. In some cases, this would even cause a paging fault.

The change adds a new scratch buffer that is used in case the
caller-provided buffer is deemed not suitable for use in an SG. The
crypto operation of the CTR DRBG produces its output with that scratch
buffer.

The scratch buffer is allocated during allocation time of the CTR DRBG
as its access is protected with the DRBG mutex.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/drbg.c         | 35 +++++++++++++++++++++++++++++++----
 include/crypto/drbg.h |  2 ++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 9a95b61..cbbd19f 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -262,6 +262,7 @@ 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
 
 /* BCC function for CTR DRBG as defined in 10.4.3 */
 static int drbg_ctr_bcc(struct drbg_state *drbg,
@@ -1644,6 +1645,9 @@ static int drbg_fini_sym_kernel(struct drbg_state *drbg)
 	kfree(drbg->ctr_null_value_buf);
 	drbg->ctr_null_value = NULL;
 
+	kfree(drbg->outscratchpadbuf);
+	drbg->outscratchpadbuf = NULL;
+
 	return 0;
 }
 
@@ -1708,6 +1712,15 @@ static int drbg_init_sym_kernel(struct drbg_state *drbg)
 	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) {
+		drbg_fini_sym_kernel(drbg);
+		return -ENOMEM;
+	}
+	drbg->outscratchpad = (u8 *)PTR_ALIGN(drbg->outscratchpadbuf,
+					      alignmask + 1);
+
 	return alignmask;
 }
 
@@ -1737,15 +1750,22 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
 			      u8 *outbuf, u32 outlen)
 {
 	struct scatterlist sg_in;
+	bool virt_addr_valid = virt_addr_valid(outbuf);
+	int ret = 0;
 
 	sg_init_one(&sg_in, inbuf, inlen);
 
 	while (outlen) {
 		u32 cryptlen = min_t(u32, inlen, outlen);
 		struct scatterlist sg_out;
-		int ret;
 
-		sg_init_one(&sg_out, outbuf, cryptlen);
+		/* If output buffer is not valid for SGL, use scratchpad */
+		if (virt_addr_valid)
+			sg_init_one(&sg_out, outbuf, cryptlen);
+		else {
+			cryptlen = min_t(u32, cryptlen, DRBG_OUTSCRATCHLEN);
+			sg_init_one(&sg_out, drbg->outscratchpad, cryptlen);
+		}
 		skcipher_request_set_crypt(drbg->ctr_req, &sg_in, &sg_out,
 					   cryptlen, drbg->V);
 		ret = crypto_skcipher_encrypt(drbg->ctr_req);
@@ -1761,15 +1781,22 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
 				break;
 			}
 		default:
-			return ret;
+			goto out;
 		}
 		init_completion(&drbg->ctr_completion);
 
+		if (!virt_addr_valid)
+			memcpy(outbuf, drbg->outscratchpad, cryptlen);
+
 		outlen -= cryptlen;
 		outbuf += cryptlen;
 	}
+	ret = 0;
 
-	return 0;
+out:
+	if (!virt_addr_valid)
+		memzero_explicit(drbg->outscratchpad, DRBG_OUTSCRATCHLEN);
+	return ret;
 }
 #endif /* CONFIG_CRYPTO_DRBG_CTR */
 
diff --git a/include/crypto/drbg.h b/include/crypto/drbg.h
index 61580b1..22f884c 100644
--- a/include/crypto/drbg.h
+++ b/include/crypto/drbg.h
@@ -124,6 +124,8 @@ struct drbg_state {
 	struct skcipher_request *ctr_req;	/* CTR mode request handle */
 	__u8 *ctr_null_value_buf;		/* CTR mode unaligned buffer */
 	__u8 *ctr_null_value;			/* CTR mode aligned zero buf */
+	__u8 *outscratchpadbuf;			/* CTR mode output scratchpad */
+        __u8 *outscratchpad;			/* CTR mode aligned outbuf */
 	struct completion ctr_completion;	/* CTR mode async handler */
 	int ctr_async_err;			/* CTR mode async error */
 
-- 
2.9.3

Re: [PATCH RESEND] crypto: CTR DRBG - prevent invalid SG mappings

[Herbert Xu]

On Mon, Nov 28, 2016 at 02:39:09PM +0100, Stephan Mueller wrote:
>
> @@ -1737,15 +1750,22 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
>  			      u8 *outbuf, u32 outlen)
>  {
>  	struct scatterlist sg_in;
> +	bool virt_addr_valid = virt_addr_valid(outbuf);
> +	int ret = 0;
>  
>  	sg_init_one(&sg_in, inbuf, inlen);
>  
>  	while (outlen) {
>  		u32 cryptlen = min_t(u32, inlen, outlen);
>  		struct scatterlist sg_out;
> -		int ret;
>  
> -		sg_init_one(&sg_out, outbuf, cryptlen);
> +		/* If output buffer is not valid for SGL, use scratchpad */
> +		if (virt_addr_valid)
> +			sg_init_one(&sg_out, outbuf, cryptlen);
> +		else {
> +			cryptlen = min_t(u32, cryptlen, DRBG_OUTSCRATCHLEN);
> +			sg_init_one(&sg_out, drbg->outscratchpad, cryptlen);
> +		}

I'm sorry but this is just way too ugly.  Please use the scratchpad
unconditionally.

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

[PATCH v2] crypto: CTR DRBG - prevent invalid SG mappings

[Stephan Mueller]

When using SGs, only heap memory (memory that is valid as per
virt_addr_valid) is allowed to be referenced. The CTR DRBG used to
reference the caller-provided memory directly in an SG. In case the
caller provided stack memory pointers, the SG mapping is not considered
to be valid. In some cases, this would even cause a paging fault.

The change adds a new scratch buffer that is used unconditionally to
catch the cases where the caller-provided buffer is not suitable for
use in an SG. The crypto operation of the CTR DRBG produces its output
with that scratch buffer and finally copies the content of the
scratch buffer to the caller's buffer.

The scratch buffer is allocated during allocation time of the CTR DRBG
as its access is protected with the DRBG mutex.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/drbg.c         | 29 ++++++++++++++++++++++++-----
 include/crypto/drbg.h |  2 ++
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index 9a95b61..8ffda67 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -262,6 +262,7 @@ 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
 
 /* BCC function for CTR DRBG as defined in 10.4.3 */
 static int drbg_ctr_bcc(struct drbg_state *drbg,
@@ -1644,6 +1645,9 @@ static int drbg_fini_sym_kernel(struct drbg_state *drbg)
 	kfree(drbg->ctr_null_value_buf);
 	drbg->ctr_null_value = NULL;
 
+	kfree(drbg->outscratchpadbuf);
+	drbg->outscratchpadbuf = NULL;
+
 	return 0;
 }
 
@@ -1708,6 +1712,15 @@ static int drbg_init_sym_kernel(struct drbg_state *drbg)
 	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) {
+		drbg_fini_sym_kernel(drbg);
+		return -ENOMEM;
+	}
+	drbg->outscratchpad = (u8 *)PTR_ALIGN(drbg->outscratchpadbuf,
+					      alignmask + 1);
+
 	return alignmask;
 }
 
@@ -1737,15 +1750,16 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
 			      u8 *outbuf, u32 outlen)
 {
 	struct scatterlist sg_in;
+	int ret;
 
 	sg_init_one(&sg_in, inbuf, inlen);
 
 	while (outlen) {
-		u32 cryptlen = min_t(u32, inlen, outlen);
+		u32 cryptlen = min3(inlen, outlen, (u32)DRBG_OUTSCRATCHLEN);
 		struct scatterlist sg_out;
-		int ret;
 
-		sg_init_one(&sg_out, outbuf, cryptlen);
+		/* Output buffer may not be valid for SGL, use scratchpad */
+		sg_init_one(&sg_out, drbg->outscratchpad, cryptlen);
 		skcipher_request_set_crypt(drbg->ctr_req, &sg_in, &sg_out,
 					   cryptlen, drbg->V);
 		ret = crypto_skcipher_encrypt(drbg->ctr_req);
@@ -1761,15 +1775,20 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
 				break;
 			}
 		default:
-			return ret;
+			goto out;
 		}
 		init_completion(&drbg->ctr_completion);
 
+		memcpy(outbuf, drbg->outscratchpad, cryptlen);
+
 		outlen -= cryptlen;
 		outbuf += cryptlen;
 	}
+	ret = 0;
 
-	return 0;
+out:
+	memzero_explicit(drbg->outscratchpad, DRBG_OUTSCRATCHLEN);
+	return ret;
 }
 #endif /* CONFIG_CRYPTO_DRBG_CTR */
 
diff --git a/include/crypto/drbg.h b/include/crypto/drbg.h
index 61580b1..22f884c 100644
--- a/include/crypto/drbg.h
+++ b/include/crypto/drbg.h
@@ -124,6 +124,8 @@ struct drbg_state {
 	struct skcipher_request *ctr_req;	/* CTR mode request handle */
 	__u8 *ctr_null_value_buf;		/* CTR mode unaligned buffer */
 	__u8 *ctr_null_value;			/* CTR mode aligned zero buf */
+	__u8 *outscratchpadbuf;			/* CTR mode output scratchpad */
+        __u8 *outscratchpad;			/* CTR mode aligned outbuf */
 	struct completion ctr_completion;	/* CTR mode async handler */
 	int ctr_async_err;			/* CTR mode async error */
 
-- 
2.9.3

Re: [PATCH v2] crypto: CTR DRBG - prevent invalid SG mappings

[Herbert Xu]

On Tue, Nov 29, 2016 at 09:45:04AM +0100, Stephan Mueller wrote:
> When using SGs, only heap memory (memory that is valid as per
> virt_addr_valid) is allowed to be referenced. The CTR DRBG used to
> reference the caller-provided memory directly in an SG. In case the
> caller provided stack memory pointers, the SG mapping is not considered
> to be valid. In some cases, this would even cause a paging fault.
> 
> The change adds a new scratch buffer that is used unconditionally to
> catch the cases where the caller-provided buffer is not suitable for
> use in an SG. The crypto operation of the CTR DRBG produces its output
> with that scratch buffer and finally copies the content of the
> scratch buffer to the caller's buffer.
> 
> The scratch buffer is allocated during allocation time of the CTR DRBG
> as its access is protected with the DRBG mutex.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>

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