Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1614585 > unrolled thread

[PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers

Back to article view | Back to linux.kernel

Contents

  [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers Eric Biggers <ebiggers3@gmail.com> - 2017-04-01 21:20 +0200
    Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack  buffers Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-04-02 04:30 +0200
      Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack  buffers Eric Biggers <ebiggers3@gmail.com> - 2017-04-02 05:40 +0200
        Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack  buffers Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-04-03 18:00 +0200
          Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack  buffers Eric Biggers <ebiggers3@gmail.com> - 2017-04-03 20:30 +0200
    Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers David Howells <dhowells@redhat.com> - 2017-04-03 17:50 +0200

#1614585 — [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers

From: Eric Biggers <ebiggers@google.com>

Since v4.9, the crypto API cannot (normally) be used to encrypt/decrypt
stack buffers because the stack may be virtually mapped.  Fix this for
the padding buffers in encrypted-keys by using ZERO_PAGE for the
encryption padding and by allocating a temporary heap buffer for the
decryption padding.

Tested with CONFIG_DEBUG_SG=y:
	keyctl new_session
	keyctl add user master "abcdefghijklmnop" @s
	keyid=$(keyctl add encrypted desc "new user:master 25" @s)
	datablob="$(keyctl pipe $keyid)"
	keyctl unlink $keyid
	keyid=$(keyctl add encrypted desc "load $datablob" @s)
	datablob2="$(keyctl pipe $keyid)"
	[ "$datablob" = "$datablob2" ] && echo "Success!"

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: stable@vger.kernel.org # 4.9+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/encrypted-keys/encrypted.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 0010955d7876..1845d47474a0 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -480,12 +480,9 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload,
 	struct skcipher_request *req;
 	unsigned int encrypted_datalen;
 	u8 iv[AES_BLOCK_SIZE];
-	unsigned int padlen;
-	char pad[16];
 	int ret;
 
 	encrypted_datalen = roundup(epayload->decrypted_datalen, blksize);
-	padlen = encrypted_datalen - epayload->decrypted_datalen;
 
 	req = init_skcipher_req(derived_key, derived_keylen);
 	ret = PTR_ERR(req);
@@ -493,11 +490,10 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload,
 		goto out;
 	dump_decrypted_data(epayload);
 
-	memset(pad, 0, sizeof pad);
 	sg_init_table(sg_in, 2);
 	sg_set_buf(&sg_in[0], epayload->decrypted_data,
 		   epayload->decrypted_datalen);
-	sg_set_buf(&sg_in[1], pad, padlen);
+	sg_set_page(&sg_in[1], ZERO_PAGE(0), AES_BLOCK_SIZE, 0);
 
 	sg_init_table(sg_out, 1);
 	sg_set_buf(sg_out, epayload->encrypted_data, encrypted_datalen);
@@ -584,9 +580,14 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload,
 	struct skcipher_request *req;
 	unsigned int encrypted_datalen;
 	u8 iv[AES_BLOCK_SIZE];
-	char pad[16];
+	u8 *pad;
 	int ret;
 
+	/* Throwaway buffer to hold the unused zero padding at the end */
+	pad = kmalloc(AES_BLOCK_SIZE, GFP_KERNEL);
+	if (!pad)
+		return -ENOMEM;
+
 	encrypted_datalen = roundup(epayload->decrypted_datalen, blksize);
 	req = init_skcipher_req(derived_key, derived_keylen);
 	ret = PTR_ERR(req);
@@ -594,13 +595,12 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload,
 		goto out;
 	dump_encrypted_data(epayload, encrypted_datalen);
 
-	memset(pad, 0, sizeof pad);
 	sg_init_table(sg_in, 1);
 	sg_init_table(sg_out, 2);
 	sg_set_buf(sg_in, epayload->encrypted_data, encrypted_datalen);
 	sg_set_buf(&sg_out[0], epayload->decrypted_data,
 		   epayload->decrypted_datalen);
-	sg_set_buf(&sg_out[1], pad, sizeof pad);
+	sg_set_buf(&sg_out[1], pad, AES_BLOCK_SIZE);
 
 	memcpy(iv, epayload->iv, sizeof(iv));
 	skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, iv);
@@ -612,6 +612,7 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload,
 		goto out;
 	dump_decrypted_data(epayload);
 out:
+	kfree(pad);
 	return ret;
 }
 
-- 
2.12.1

[toc] | [next] | [standalone]

#1614623 — Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers

Hi Eric,

On Sat, 2017-04-01 at 12:17 -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Since v4.9, the crypto API cannot (normally) be used to encrypt/decrypt
> stack buffers because the stack may be virtually mapped.  Fix this for
> the padding buffers in encrypted-keys by using ZERO_PAGE for the
> encryption padding and by allocating a temporary heap buffer for the
> decryption padding.
> 
> Tested with CONFIG_DEBUG_SG=y:
> 	keyctl new_session
> 	keyctl add user master "abcdefghijklmnop" @s
> 	keyid=$(keyctl add encrypted desc "new user:master 25" @s)
> 	datablob="$(keyctl pipe $keyid)"
> 	keyctl unlink $keyid
> 	keyid=$(keyctl add encrypted desc "load $datablob" @s)
> 	datablob2="$(keyctl pipe $keyid)"
> 	[ "$datablob" = "$datablob2" ] && echo "Success!"

Have you created an encrypted key on a kernel without this patch and
attempted to load that key on a kernel with this patch?  Does it still
work?

Mimi

> 
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: stable@vger.kernel.org # 4.9+
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  security/keys/encrypted-keys/encrypted.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> index 0010955d7876..1845d47474a0 100644
> --- a/security/keys/encrypted-keys/encrypted.c
> +++ b/security/keys/encrypted-keys/encrypted.c
> @@ -480,12 +480,9 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload,
>  	struct skcipher_request *req;
>  	unsigned int encrypted_datalen;
>  	u8 iv[AES_BLOCK_SIZE];
> -	unsigned int padlen;
> -	char pad[16];
>  	int ret;
> 
>  	encrypted_datalen = roundup(epayload->decrypted_datalen, blksize);
> -	padlen = encrypted_datalen - epayload->decrypted_datalen;
> 
>  	req = init_skcipher_req(derived_key, derived_keylen);
>  	ret = PTR_ERR(req);
> @@ -493,11 +490,10 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload,
>  		goto out;
>  	dump_decrypted_data(epayload);
> 
> -	memset(pad, 0, sizeof pad);
>  	sg_init_table(sg_in, 2);
>  	sg_set_buf(&sg_in[0], epayload->decrypted_data,
>  		   epayload->decrypted_datalen);
> -	sg_set_buf(&sg_in[1], pad, padlen);
> +	sg_set_page(&sg_in[1], ZERO_PAGE(0), AES_BLOCK_SIZE, 0);
> 
>  	sg_init_table(sg_out, 1);
>  	sg_set_buf(sg_out, epayload->encrypted_data, encrypted_datalen);
> @@ -584,9 +580,14 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload,
>  	struct skcipher_request *req;
>  	unsigned int encrypted_datalen;
>  	u8 iv[AES_BLOCK_SIZE];
> -	char pad[16];
> +	u8 *pad;
>  	int ret;
> 
> +	/* Throwaway buffer to hold the unused zero padding at the end */
> +	pad = kmalloc(AES_BLOCK_SIZE, GFP_KERNEL);
> +	if (!pad)
> +		return -ENOMEM;
> +
>  	encrypted_datalen = roundup(epayload->decrypted_datalen, blksize);
>  	req = init_skcipher_req(derived_key, derived_keylen);
>  	ret = PTR_ERR(req);
> @@ -594,13 +595,12 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload,
>  		goto out;
>  	dump_encrypted_data(epayload, encrypted_datalen);
> 
> -	memset(pad, 0, sizeof pad);
>  	sg_init_table(sg_in, 1);
>  	sg_init_table(sg_out, 2);
>  	sg_set_buf(sg_in, epayload->encrypted_data, encrypted_datalen);
>  	sg_set_buf(&sg_out[0], epayload->decrypted_data,
>  		   epayload->decrypted_datalen);
> -	sg_set_buf(&sg_out[1], pad, sizeof pad);
> +	sg_set_buf(&sg_out[1], pad, AES_BLOCK_SIZE);
> 
>  	memcpy(iv, epayload->iv, sizeof(iv));
>  	skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, iv);
> @@ -612,6 +612,7 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload,
>  		goto out;
>  	dump_decrypted_data(epayload);
>  out:
> +	kfree(pad);
>  	return ret;
>  }
> 

[toc] | [prev] | [next] | [standalone]

#1614639 — Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers

On Sat, Apr 01, 2017 at 10:23:57PM -0400, Mimi Zohar wrote:
> On Sat, 2017-04-01 at 12:17 -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Since v4.9, the crypto API cannot (normally) be used to encrypt/decrypt
> > stack buffers because the stack may be virtually mapped.  Fix this for
> > the padding buffers in encrypted-keys by using ZERO_PAGE for the
> > encryption padding and by allocating a temporary heap buffer for the
> > decryption padding.
> > 
> > Tested with CONFIG_DEBUG_SG=y:
> > 	keyctl new_session
> > 	keyctl add user master "abcdefghijklmnop" @s
> > 	keyid=$(keyctl add encrypted desc "new user:master 25" @s)
> > 	datablob="$(keyctl pipe $keyid)"
> > 	keyctl unlink $keyid
> > 	keyid=$(keyctl add encrypted desc "load $datablob" @s)
> > 	datablob2="$(keyctl pipe $keyid)"
> > 	[ "$datablob" = "$datablob2" ] && echo "Success!"
> 
> Have you created an encrypted key on a kernel without this patch and
> attempted to load that key on a kernel with this patch?  Does it still
> work?
> 

Yes, a key exported from an unpatched kernel (with DEBUG_SG and DEBUG_VIRTUAL
turned off so it doesn't crash) can be loaded on a patched kernel, then exported
again.  The exported data is identical.

Eric

[toc] | [prev] | [next] | [standalone]

#1615362 — Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers

On Sat, 2017-04-01 at 20:33 -0700, Eric Biggers wrote:
> On Sat, Apr 01, 2017 at 10:23:57PM -0400, Mimi Zohar wrote:
> > On Sat, 2017-04-01 at 12:17 -0700, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > Since v4.9, the crypto API cannot (normally) be used to encrypt/decrypt
> > > stack buffers because the stack may be virtually mapped.  Fix this for
> > > the padding buffers in encrypted-keys by using ZERO_PAGE for the
> > > encryption padding and by allocating a temporary heap buffer for the
> > > decryption padding.
> > > 
> > > Tested with CONFIG_DEBUG_SG=y:
> > > 	keyctl new_session
> > > 	keyctl add user master "abcdefghijklmnop" @s
> > > 	keyid=$(keyctl add encrypted desc "new user:master 25" @s)
> > > 	datablob="$(keyctl pipe $keyid)"
> > > 	keyctl unlink $keyid
> > > 	keyid=$(keyctl add encrypted desc "load $datablob" @s)
> > > 	datablob2="$(keyctl pipe $keyid)"
> > > 	[ "$datablob" = "$datablob2" ] && echo "Success!"
> > 
> > Have you created an encrypted key on a kernel without this patch and
> > attempted to load that key on a kernel with this patch?  Does it still
> > work?
> > 
> 
> Yes, a key exported from an unpatched kernel (with DEBUG_SG and DEBUG_VIRTUAL
> turned off so it doesn't crash) can be loaded on a patched kernel, then exported
> again.  The exported data is identical.

This patch removes calculating the "padlen".  Will this change break
other use cases?

Mimi

[toc] | [prev] | [next] | [standalone]

#1615462 — Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers

On Mon, Apr 03, 2017 at 11:55:42AM -0400, Mimi Zohar wrote:
> 
> This patch removes calculating the "padlen".  Will this change break
> other use cases?
> 

No, the number of bytes being encrypted is still 'encrypted_datalen' which is
passed to skcipher_request_set_crypt().  It's okay if the input scatterlist is
longer than that; only the first 'encrypted_datalen' bytes will be used.

- Eric

[toc] | [prev] | [next] | [standalone]

#1615346

Pulled.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web