From a964f395614af195cf5c5caa84a9c487b86d5ba5 Mon Sep 17 00:00:00 2001 From: Tycho Andersen Date: Tue, 24 Apr 2018 14:26:37 -0600 Subject: big key: get rid of stack array allocation 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 Reviewed-by: Kees Cook CC: David Howells CC: James Morris CC: "Serge E. Hallyn" CC: Jason A. Donenfeld CC: Eric Biggers Signed-off-by: James Morris --- security/keys/big_key.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) (limited to 'security/keys') 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 #include #include +#include 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); -- cgit From 383203eff718d7397ffc68ac6c3ed644d3017fc7 Mon Sep 17 00:00:00 2001 From: Tycho Andersen Date: Tue, 24 Apr 2018 14:26:38 -0600 Subject: dh key: get rid of stack allocated array 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 Reviewed-by: Kees Cook CC: David Howells CC: James Morris CC: "Serge E. Hallyn" CC: Eric Biggers Signed-off-by: James Morris --- security/keys/dh.c | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-) (limited to 'security/keys') 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; -- cgit From 890e2abe1028c39e5399101a2c277219cd637aaa Mon Sep 17 00:00:00 2001 From: Tycho Andersen Date: Tue, 24 Apr 2018 14:26:39 -0600 Subject: dh key: get rid of stack allocated array for zeroes 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 Reviewed-by: Kees Cook CC: David Howells CC: James Morris CC: "Serge E. Hallyn" CC: Eric Biggers Signed-off-by: James Morris --- security/keys/dh.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'security/keys') 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); } -- cgit