* [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers
@ 2017-04-01 19:17 Eric Biggers
2017-04-02 2:23 ` Mimi Zohar
2017-04-03 15:44 ` David Howells
0 siblings, 2 replies; 6+ messages in thread
From: Eric Biggers @ 2017-04-01 19:17 UTC (permalink / raw)
To: keyrings
Cc: David Howells, Andy Lutomirski, Herbert Xu, Mimi Zohar,
Eric Biggers, linux-kernel, stable
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers
2017-04-01 19:17 [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers Eric Biggers
@ 2017-04-02 2:23 ` Mimi Zohar
2017-04-02 3:33 ` Eric Biggers
2017-04-03 15:44 ` David Howells
1 sibling, 1 reply; 6+ messages in thread
From: Mimi Zohar @ 2017-04-02 2:23 UTC (permalink / raw)
To: Eric Biggers, keyrings
Cc: David Howells, Andy Lutomirski, Herbert Xu, Eric Biggers,
linux-kernel, stable
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;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers
2017-04-02 2:23 ` Mimi Zohar
@ 2017-04-02 3:33 ` Eric Biggers
2017-04-03 15:55 ` Mimi Zohar
0 siblings, 1 reply; 6+ messages in thread
From: Eric Biggers @ 2017-04-02 3:33 UTC (permalink / raw)
To: Mimi Zohar
Cc: keyrings, David Howells, Andy Lutomirski, Herbert Xu,
Eric Biggers, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers
2017-04-01 19:17 [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers Eric Biggers
2017-04-02 2:23 ` Mimi Zohar
@ 2017-04-03 15:44 ` David Howells
1 sibling, 0 replies; 6+ messages in thread
From: David Howells @ 2017-04-03 15:44 UTC (permalink / raw)
To: Eric Biggers
Cc: dhowells, keyrings, Andy Lutomirski, Herbert Xu, Mimi Zohar,
Eric Biggers, linux-kernel, stable
Pulled.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers
2017-04-02 3:33 ` Eric Biggers
@ 2017-04-03 15:55 ` Mimi Zohar
2017-04-03 18:21 ` Eric Biggers
0 siblings, 1 reply; 6+ messages in thread
From: Mimi Zohar @ 2017-04-03 15:55 UTC (permalink / raw)
To: Eric Biggers
Cc: keyrings, David Howells, Andy Lutomirski, Herbert Xu,
Eric Biggers, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers
2017-04-03 15:55 ` Mimi Zohar
@ 2017-04-03 18:21 ` Eric Biggers
0 siblings, 0 replies; 6+ messages in thread
From: Eric Biggers @ 2017-04-03 18:21 UTC (permalink / raw)
To: Mimi Zohar
Cc: keyrings, David Howells, Andy Lutomirski, Herbert Xu,
Eric Biggers, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-03 18:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-01 19:17 [PATCH] KEYS: encrypted: avoid encrypting/decrypting stack buffers Eric Biggers
2017-04-02 2:23 ` Mimi Zohar
2017-04-02 3:33 ` Eric Biggers
2017-04-03 15:55 ` Mimi Zohar
2017-04-03 18:21 ` Eric Biggers
2017-04-03 15:44 ` David Howells
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).