[PATCH v3 1/3] big key: get rid of stack array allocation

[PATCH v3 1/3] big key: get rid of stack array allocation

[Tycho Andersen]

We're interested in getting rid of all of the stack allocated arrays in the
kernel [1]. This patch simply hardcodes the iv length to match that of the
hardcoded cipher.

[1]: https://lkml.org/lkml/2018/3/7/621

v2: hardcode the length of the nonce to be the GCM AES IV length, and do a
    sanity check in init(), Eric Biggers
v3: * remember to free big_key_aead when sanity check fails
    * define a constant for big key IV size so it can be changed along side
      the algorithm in the code

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
CC: David Howells <dhowells@redhat.com>
CC: James Morris <jmorris@namei.org>
CC: "Serge E. Hallyn" <serge@hallyn.com>
CC: Jason A. Donenfeld <Jason@zx2c4.com>
CC: Eric Biggers <ebiggers3@gmail.com>
---
 security/keys/big_key.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 933623784ccd..2806e70d7f8f 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -22,6 +22,7 @@
 #include <keys/user-type.h>
 #include <keys/big_key-type.h>
 #include <crypto/aead.h>
+#include <crypto/gcm.h>
 
 struct big_key_buf {
 	unsigned int		nr_pages;
@@ -85,6 +86,7 @@ struct key_type key_type_big_key = {
  * Crypto names for big_key data authenticated encryption
  */
 static const char big_key_alg_name[] = "gcm(aes)";
+#define BIG_KEY_IV_SIZE		GCM_AES_IV_SIZE
 
 /*
  * Crypto algorithms for big_key data authenticated encryption
@@ -109,7 +111,7 @@ static int big_key_crypt(enum big_key_op op, struct big_key_buf *buf, size_t dat
 	 * an .update function, so there's no chance we'll wind up reusing the
 	 * key to encrypt updated data. Simply put: one key, one encryption.
 	 */
-	u8 zero_nonce[crypto_aead_ivsize(big_key_aead)];
+	u8 zero_nonce[BIG_KEY_IV_SIZE];
 
 	aead_req = aead_request_alloc(big_key_aead, GFP_KERNEL);
 	if (!aead_req)
@@ -425,6 +427,13 @@ static int __init big_key_init(void)
 		pr_err("Can't alloc crypto: %d\n", ret);
 		return ret;
 	}
+
+	if (unlikely(crypto_aead_ivsize(big_key_aead) != BIG_KEY_IV_SIZE)) {
+		WARN(1, "big key algorithm changed?");
+		ret = -EINVAL;
+		goto free_aead;
+	}
+
 	ret = crypto_aead_setauthsize(big_key_aead, ENC_AUTHTAG_SIZE);
 	if (ret < 0) {
 		pr_err("Can't set crypto auth tag len: %d\n", ret);
-- 
2.17.0

[PATCH v3 2/3] dh key: get rid of stack allocated array

[Tycho Andersen]

We're interested in getting rid of all of the stack allocated arrays in the
kernel: https://lkml.org/lkml/2018/3/7/621

This particular vla is used as a temporary output buffer in case there is
too much hash output for the destination buffer. Instead, let's just
allocate a buffer that's big enough initially, but only copy back to
userspace the amount that was originally asked for.

v2: allocate enough in the original output buffer vs creating a temporary
    output buffer

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
CC: David Howells <dhowells@redhat.com>
CC: James Morris <jmorris@namei.org>
CC: "Serge E. Hallyn" <serge@hallyn.com>
CC: Eric Biggers <ebiggers3@gmail.com>
---
 security/keys/dh.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/security/keys/dh.c b/security/keys/dh.c
index d1ea9f325f94..9fecaea6c298 100644
--- a/security/keys/dh.c
+++ b/security/keys/dh.c
@@ -183,24 +183,13 @@ static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen,
 				goto err;
 		}
 
-		if (dlen < h) {
-			u8 tmpbuffer[h];
-
-			err = crypto_shash_final(desc, tmpbuffer);
-			if (err)
-				goto err;
-			memcpy(dst, tmpbuffer, dlen);
-			memzero_explicit(tmpbuffer, h);
-			return 0;
-		} else {
-			err = crypto_shash_final(desc, dst);
-			if (err)
-				goto err;
+		err = crypto_shash_final(desc, dst);
+		if (err)
+			goto err;
 
-			dlen -= h;
-			dst += h;
-			counter = cpu_to_be32(be32_to_cpu(counter) + 1);
-		}
+		dlen -= h;
+		dst += h;
+		counter = cpu_to_be32(be32_to_cpu(counter) + 1);
 	}
 
 	return 0;
@@ -216,14 +205,16 @@ static int keyctl_dh_compute_kdf(struct kdf_sdesc *sdesc,
 {
 	uint8_t *outbuf = NULL;
 	int ret;
+	size_t outbuf_len = round_up(buflen,
+			             crypto_shash_digestsize(sdesc->shash.tfm));
 
-	outbuf = kmalloc(buflen, GFP_KERNEL);
+	outbuf = kmalloc(outbuf_len, GFP_KERNEL);
 	if (!outbuf) {
 		ret = -ENOMEM;
 		goto err;
 	}
 
-	ret = kdf_ctr(sdesc, kbuf, kbuflen, outbuf, buflen, lzero);
+	ret = kdf_ctr(sdesc, kbuf, kbuflen, outbuf, outbuf_len, lzero);
 	if (ret)
 		goto err;
 
-- 
2.17.0

[PATCH v3 3/3] dh key: get rid of stack allocated array for zeroes

[Tycho Andersen]

We're interested in getting rid of all of the stack allocated arrays in
the kernel: https://lkml.org/lkml/2018/3/7/621

This case is interesting, since we really just need an array of bytes that
are zero. The loop already ensures that if the array isn't exactly the
right size that enough zero bytes will be copied in. So, instead of
choosing this value to be the size of the hash, let's just choose it to be
32, since that is a common size, is not too big, and will not result in too
many extra iterations of the loop.

v2: split out from other patch, just hardcode array size instead of
    dynamically allocating something the right size
v3: fix typo of 256 -> 32

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
CC: David Howells <dhowells@redhat.com>
CC: James Morris <jmorris@namei.org>
CC: "Serge E. Hallyn" <serge@hallyn.com>
CC: Eric Biggers <ebiggers3@gmail.com>
---
 security/keys/dh.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/keys/dh.c b/security/keys/dh.c
index 9fecaea6c298..f7403821db7f 100644
--- a/security/keys/dh.c
+++ b/security/keys/dh.c
@@ -162,8 +162,8 @@ static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen,
 			goto err;
 
 		if (zlen && h) {
-			u8 tmpbuffer[h];
-			size_t chunk = min_t(size_t, zlen, h);
+			u8 tmpbuffer[32];
+			size_t chunk = min_t(size_t, zlen, sizeof(tmpbuffer));
 			memset(tmpbuffer, 0, chunk);
 
 			do {
@@ -173,7 +173,7 @@ static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen,
 					goto err;
 
 				zlen -= chunk;
-				chunk = min_t(size_t, zlen, h);
+				chunk = min_t(size_t, zlen, sizeof(tmpbuffer));
 			} while (zlen);
 		}
 
-- 
2.17.0

Re: [PATCH v3 1/3] big key: get rid of stack array allocation

[Tycho Andersen]

Hi,

Any thoughts on this series?

Thanks,

Tycho

Re: [PATCH v3 1/3] big key: get rid of stack array allocation

[Kees Cook]

On Tue, Apr 24, 2018 at 1:26 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> We're interested in getting rid of all of the stack allocated arrays in the
> kernel [1]. This patch simply hardcodes the iv length to match that of the
> hardcoded cipher.
>
> [1]: https://lkml.org/lkml/2018/3/7/621
>
> v2: hardcode the length of the nonce to be the GCM AES IV length, and do a
>     sanity check in init(), Eric Biggers
> v3: * remember to free big_key_aead when sanity check fails
>     * define a constant for big key IV size so it can be changed along side
>       the algorithm in the code
>
> Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> CC: David Howells <dhowells@redhat.com>
> CC: James Morris <jmorris@namei.org>
> CC: "Serge E. Hallyn" <serge@hallyn.com>
> CC: Jason A. Donenfeld <Jason@zx2c4.com>
> CC: Eric Biggers <ebiggers3@gmail.com>

Please consider this and patches 2 and 3:

Reviewed-by: Kees Cook <keescook@chromium.org>

James, are these something you can take into your tree?

Thanks!

-Kees

> ---
>  security/keys/big_key.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/security/keys/big_key.c b/security/keys/big_key.c
> index 933623784ccd..2806e70d7f8f 100644
> --- a/security/keys/big_key.c
> +++ b/security/keys/big_key.c
> @@ -22,6 +22,7 @@
>  #include <keys/user-type.h>
>  #include <keys/big_key-type.h>
>  #include <crypto/aead.h>
> +#include <crypto/gcm.h>
>
>  struct big_key_buf {
>         unsigned int            nr_pages;
> @@ -85,6 +86,7 @@ struct key_type key_type_big_key = {
>   * Crypto names for big_key data authenticated encryption
>   */
>  static const char big_key_alg_name[] = "gcm(aes)";
> +#define BIG_KEY_IV_SIZE                GCM_AES_IV_SIZE
>
>  /*
>   * Crypto algorithms for big_key data authenticated encryption
> @@ -109,7 +111,7 @@ static int big_key_crypt(enum big_key_op op, struct big_key_buf *buf, size_t dat
>          * an .update function, so there's no chance we'll wind up reusing the
>          * key to encrypt updated data. Simply put: one key, one encryption.
>          */
> -       u8 zero_nonce[crypto_aead_ivsize(big_key_aead)];
> +       u8 zero_nonce[BIG_KEY_IV_SIZE];
>
>         aead_req = aead_request_alloc(big_key_aead, GFP_KERNEL);
>         if (!aead_req)
> @@ -425,6 +427,13 @@ static int __init big_key_init(void)
>                 pr_err("Can't alloc crypto: %d\n", ret);
>                 return ret;
>         }
> +
> +       if (unlikely(crypto_aead_ivsize(big_key_aead) != BIG_KEY_IV_SIZE)) {
> +               WARN(1, "big key algorithm changed?");
> +               ret = -EINVAL;
> +               goto free_aead;
> +       }
> +
>         ret = crypto_aead_setauthsize(big_key_aead, ENC_AUTHTAG_SIZE);
>         if (ret < 0) {
>                 pr_err("Can't set crypto auth tag len: %d\n", ret);
> --
> 2.17.0
>



-- 
Kees Cook
Pixel Security

Re: [PATCH v3 1/3] big key: get rid of stack array allocation

[James Morris]

On Tue, 8 May 2018, Kees Cook wrote:

> On Tue, Apr 24, 2018 at 1:26 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > We're interested in getting rid of all of the stack allocated arrays in the
> > kernel [1]. This patch simply hardcodes the iv length to match that of the
> > hardcoded cipher.
> >
> > [1]: https://lkml.org/lkml/2018/3/7/621
> >
> > v2: hardcode the length of the nonce to be the GCM AES IV length, and do a
> >     sanity check in init(), Eric Biggers
> > v3: * remember to free big_key_aead when sanity check fails
> >     * define a constant for big key IV size so it can be changed along side
> >       the algorithm in the code
> >
> > Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> > CC: David Howells <dhowells@redhat.com>
> > CC: James Morris <jmorris@namei.org>
> > CC: "Serge E. Hallyn" <serge@hallyn.com>
> > CC: Jason A. Donenfeld <Jason@zx2c4.com>
> > CC: Eric Biggers <ebiggers3@gmail.com>
> 
> Please consider this and patches 2 and 3:
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> James, are these something you can take into your tree?

> 
> Thanks!
> 
> -Kees
> 
> > ---
> >  security/keys/big_key.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/keys/big_key.c b/security/keys/big_key.c
> > index 933623784ccd..2806e70d7f8f 100644
> > --- a/security/keys/big_key.c
> > +++ b/security/keys/big_key.c
> > @@ -22,6 +22,7 @@
> >  #include <keys/user-type.h>
> >  #include <keys/big_key-type.h>
> >  #include <crypto/aead.h>
> > +#include <crypto/gcm.h>
> >
> >  struct big_key_buf {
> >         unsigned int            nr_pages;
> > @@ -85,6 +86,7 @@ struct key_type key_type_big_key = {
Sure!

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH v3 1/3] big key: get rid of stack array allocation

[James Morris]

On Thu, 10 May 2018, James Morris wrote:

> > 
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > 
> > James, are these something you can take into your tree?
> 
> Sure!

Although, normally, these would likely come in to mine via David's tree.  

Please do that unless there's a special case here.


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH v3 1/3] big key: get rid of stack array allocation

[Kees Cook]

On Wed, May 9, 2018 at 12:21 PM, James Morris <jmorris@namei.org> wrote:
> On Thu, 10 May 2018, James Morris wrote:
>
>> >
>> > Reviewed-by: Kees Cook <keescook@chromium.org>
>> >
>> > James, are these something you can take into your tree?
>>
>> Sure!
>
> Although, normally, these would likely come in to mine via David's tree.
>
> Please do that unless there's a special case here.

David has not replied to any of the threads or versions since
originally posted on March 12. David, can you take these or at least
Ack them for James?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v3 1/3] big key: get rid of stack array allocation

[James Morris]

On Tue, 24 Apr 2018, Tycho Andersen wrote:

> We're interested in getting rid of all of the stack allocated arrays in the
> kernel [1]. This patch simply hardcodes the iv length to match that of the
> hardcoded cipher.
> 
> [1]: https://lkml.org/lkml/2018/3/7/621
> 
> v2: hardcode the length of the nonce to be the GCM AES IV length, and do a
>     sanity check in init(), Eric Biggers
> v3: * remember to free big_key_aead when sanity check fails
>     * define a constant for big key IV size so it can be changed along side
>       the algorithm in the code

All applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general
and next-testing


Thanks!


-- 
James Morris
<jmorris@namei.org>