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 <[email protected]>
CC: David Howells <[email protected]>
CC: James Morris <[email protected]>
CC: "Serge E. Hallyn" <[email protected]>
CC: Jason A. Donenfeld <[email protected]>
CC: Eric Biggers <[email protected]>
---
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
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 <[email protected]>
CC: David Howells <[email protected]>
CC: James Morris <[email protected]>
CC: "Serge E. Hallyn" <[email protected]>
CC: Eric Biggers <[email protected]>
---
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
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 <[email protected]>
CC: David Howells <[email protected]>
CC: James Morris <[email protected]>
CC: "Serge E. Hallyn" <[email protected]>
CC: Eric Biggers <[email protected]>
---
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
Hi,
Any thoughts on this series?
Thanks,
Tycho
On Tue, Apr 24, 2018 at 1:26 PM, Tycho Andersen <[email protected]> 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 <[email protected]>
> CC: David Howells <[email protected]>
> CC: James Morris <[email protected]>
> CC: "Serge E. Hallyn" <[email protected]>
> CC: Jason A. Donenfeld <[email protected]>
> CC: Eric Biggers <[email protected]>
Please consider this and patches 2 and 3:
Reviewed-by: Kees Cook <[email protected]>
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
On Tue, 8 May 2018, Kees Cook wrote:
> On Tue, Apr 24, 2018 at 1:26 PM, Tycho Andersen <[email protected]> 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 <[email protected]>
> > CC: David Howells <[email protected]>
> > CC: James Morris <[email protected]>
> > CC: "Serge E. Hallyn" <[email protected]>
> > CC: Jason A. Donenfeld <[email protected]>
> > CC: Eric Biggers <[email protected]>
>
> Please consider this and patches 2 and 3:
>
> Reviewed-by: Kees Cook <[email protected]>
>
> 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
<[email protected]>
On Thu, 10 May 2018, James Morris wrote:
> >
> > Reviewed-by: Kees Cook <[email protected]>
> >
> > 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
<[email protected]>
On Wed, May 9, 2018 at 12:21 PM, James Morris <[email protected]> wrote:
> On Thu, 10 May 2018, James Morris wrote:
>
>> >
>> > Reviewed-by: Kees Cook <[email protected]>
>> >
>> > 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
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
<[email protected]>