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

[PATCH 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

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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 933623784ccd..75c46786a166 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;
@@ -109,7 +110,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[GCM_AES_IV_SIZE];
 
 	aead_req = aead_request_alloc(big_key_aead, GFP_KERNEL);
 	if (!aead_req)
@@ -425,6 +426,12 @@ 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) != GCM_AES_IV_SIZE)) {
+		WARN(1, "big key algorithm changed?");
+		return -EINVAL;
+	}
+
 	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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info@ http://vger.kernel.org/majordomo-info.html

[PATCH 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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 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
256, since that is a common size, is not to 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

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..74f8a853872e 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[256];
+			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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tycho Andersen]

On Mon, Apr 23, 2018 at 07:03:21PM -0600, Tycho Andersen wrote:
> 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
> 256, since that is a common size, is not to 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
> 
> 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..74f8a853872e 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[256];

Whoops, this should be 32, not 256. That shouldn't make any runtime
difference, but it'll closer match the allocation patterns from
before.

I'll let this sit for a bit and send v3.

Tycho
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Biggers]

Hi Tycho,

On Mon, Apr 23, 2018 at 07:03:19PM -0600, 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
> 
> 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 | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/security/keys/big_key.c b/security/keys/big_key.c
> index 933623784ccd..75c46786a166 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;
> @@ -109,7 +110,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[GCM_AES_IV_SIZE];
>  
>  	aead_req = aead_request_alloc(big_key_aead, GFP_KERNEL);
>  	if (!aead_req)
> @@ -425,6 +426,12 @@ 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) != GCM_AES_IV_SIZE)) {
> +		WARN(1, "big key algorithm changed?");
> +		return -EINVAL;
> +	}
> +

'big_key_aead' needs to be freed on error.

	err = -EINVAL;
	goto free_aead;

Also how about defining the IV size next to the algorithm name?
Then all the algorithm details would be on adjacent lines:

static const char big_key_alg_name[] = "gcm(aes)";
#define BIG_KEY_IV_SIZE         GCM_AES_IV_SIZE

- Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tycho Andersen]

Hi Eric,

On Mon, Apr 23, 2018 at 09:50:15PM -0700, Eric Biggers wrote:
> Hi Tycho,
> 
> On Mon, Apr 23, 2018 at 07:03:19PM -0600, 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
> > 
> > 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 | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/security/keys/big_key.c b/security/keys/big_key.c
> > index 933623784ccd..75c46786a166 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;
> > @@ -109,7 +110,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[GCM_AES_IV_SIZE];
> >  
> >  	aead_req = aead_request_alloc(big_key_aead, GFP_KERNEL);
> >  	if (!aead_req)
> > @@ -425,6 +426,12 @@ 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) != GCM_AES_IV_SIZE)) {
> > +		WARN(1, "big key algorithm changed?");
> > +		return -EINVAL;
> > +	}
> > +
> 
> 'big_key_aead' needs to be freed on error.
> 
> 	err = -EINVAL;
> 	goto free_aead;

oof, yes, thanks.

> Also how about defining the IV size next to the algorithm name?
> Then all the algorithm details would be on adjacent lines:
> 
> static const char big_key_alg_name[] = "gcm(aes)";
> #define BIG_KEY_IV_SIZE         GCM_AES_IV_SIZE

Sounds good, I'll fix both of these for v3.

Cheers,

Tycho
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tetsuo Handa]

Tycho Andersen wrote:
> > > +	if (unlikely(crypto_aead_ivsize(big_key_aead) != GCM_AES_IV_SIZE)) {
> > > +		WARN(1, "big key algorithm changed?");

Please avoid using WARN() WARN_ON() etc.
syzbot would catch it and panic() due to panic_on_warn == 1.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tycho Andersen]

On Tue, Apr 24, 2018 at 11:46:38PM +0900, Tetsuo Handa wrote:
> Tycho Andersen wrote:
> > > > +	if (unlikely(crypto_aead_ivsize(big_key_aead) != GCM_AES_IV_SIZE)) {
> > > > +		WARN(1, "big key algorithm changed?");
> 
> Please avoid using WARN() WARN_ON() etc.
> syzbot would catch it and panic() due to panic_on_warn == 1.

But it is really a programming bug in this case (and it seems better
than BUG()...). Isn't this exactly the sort of case we want to catch?

Tycho
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Serge E. Hallyn]

Quoting Tycho Andersen (tycho at tycho.ws):
> On Tue, Apr 24, 2018 at 11:46:38PM +0900, Tetsuo Handa wrote:
> > Tycho Andersen wrote:
> > > > > +	if (unlikely(crypto_aead_ivsize(big_key_aead) != GCM_AES_IV_SIZE)) {
> > > > > +		WARN(1, "big key algorithm changed?");
> > 
> > Please avoid using WARN() WARN_ON() etc.
> > syzbot would catch it and panic() due to panic_on_warn == 1.
> 
> But it is really a programming bug in this case (and it seems better
> than BUG()...). Isn't this exactly the sort of case we want to catch?
> 
> Tycho

Right - is there a url to some discussion about this?  Because not
using WARN when WARN should be used, because it troubles a bot, seems
the wrong solution.  If this *is* what's been agreed upon, then
what is the new recommended thing to do here?

-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Kees Cook]

On Tue, Apr 24, 2018 at 12:58 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> Quoting Tycho Andersen (tycho at tycho.ws):
>> On Tue, Apr 24, 2018 at 11:46:38PM +0900, Tetsuo Handa wrote:
>> > Tycho Andersen wrote:
>> > > > > +     if (unlikely(crypto_aead_ivsize(big_key_aead) != GCM_AES_IV_SIZE)) {
>> > > > > +             WARN(1, "big key algorithm changed?");
>> >
>> > Please avoid using WARN() WARN_ON() etc.
>> > syzbot would catch it and panic() due to panic_on_warn == 1.
>>
>> But it is really a programming bug in this case (and it seems better
>> than BUG()...). Isn't this exactly the sort of case we want to catch?
>>
>> Tycho
>
> Right - is there a url to some discussion about this?  Because not
> using WARN when WARN should be used, because it troubles a bot, seems
> the wrong solution.  If this *is* what's been agreed upon, then
> what is the new recommended thing to do here?

BUG() is basically supposed to never be used, as decreed by Linus.
WARN() here is entirely correct: if we encounter a case where
crypto_aead_ivsize(big_key_aead) != GCM_AES_IV_SIZE is not true, we
run the risk of stack memory corruption. If this is an EXPECTED
failure case, then okay, drop the WARN() but we have to keep the
-EINVAL.

-Kees

-- 
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tetsuo Handa]

Kees Cook wrote:
> On Tue, Apr 24, 2018 at 12:58 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> > Quoting Tycho Andersen (tycho at tycho.ws):
> >> On Tue, Apr 24, 2018 at 11:46:38PM +0900, Tetsuo Handa wrote:
> >> > Tycho Andersen wrote:
> >> > > > > +     if (unlikely(crypto_aead_ivsize(big_key_aead) != GCM_AES_IV_SIZE)) {
> >> > > > > +             WARN(1, "big key algorithm changed?");
> >> >
> >> > Please avoid using WARN() WARN_ON() etc.
> >> > syzbot would catch it and panic() due to panic_on_warn == 1.
> >>
> >> But it is really a programming bug in this case (and it seems better
> >> than BUG()...). Isn't this exactly the sort of case we want to catch?
> >>
> >> Tycho
> >
> > Right - is there a url to some discussion about this?  Because not
> > using WARN when WARN should be used, because it troubles a bot, seems
> > the wrong solution.  If this *is* what's been agreed upon, then
> > what is the new recommended thing to do here?
> 
> BUG() is basically supposed to never be used, as decreed by Linus.
> WARN() here is entirely correct: if we encounter a case where
> crypto_aead_ivsize(big_key_aead) != GCM_AES_IV_SIZE is not true, we
> run the risk of stack memory corruption. If this is an EXPECTED
> failure case, then okay, drop the WARN() but we have to keep the
> -EINVAL.

big_key_init() is __init function of built-in module which will be called
only once upon boot, isn't it? Then, there is no point to continue after
WARN(); BUG() is better here.



Moreover, if this is meant for sanity check in case something went wrong
(e.g. memory corruption), it is better to check at run time like

diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 9336237..bca04f2 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;
@@ -109,7 +110,12 @@ 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[GCM_AES_IV_SIZE];
+
+	if (crypto_aead_ivsize(big_key_aead) != sizeof(zero_nonce)) {
+		pr_err("big key algorithm changed?");
+		return -EINVAL;
+	}
 
 	aead_req = aead_request_alloc(big_key_aead, GFP_KERNEL);
 	if (!aead_req)

because crypto_aead_ivsize(big_key_aead) == GCM_AES_IV_SIZE is true
unless something goes wrong at run time, isn't it?



Moreover, zero_nonce[] can be "static" if all actions after memory allocation
are guarded by global big_key_aead_lock mutex?

diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 9336237..1e7d2d1 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;
@@ -109,27 +110,28 @@ 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)];
+	static u8 zero_nonce[GCM_AES_IV_SIZE];
+
+	if (crypto_aead_ivsize(big_key_aead) != sizeof(zero_nonce)) {
+		pr_err("big key algorithm changed?");
+		return -EINVAL;
+	}
 
 	aead_req = aead_request_alloc(big_key_aead, GFP_KERNEL);
 	if (!aead_req)
 		return -ENOMEM;
 
+	mutex_lock(&big_key_aead_lock);
 	memset(zero_nonce, 0, sizeof(zero_nonce));
 	aead_request_set_crypt(aead_req, buf->sg, buf->sg, datalen, zero_nonce);
 	aead_request_set_callback(aead_req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
 	aead_request_set_ad(aead_req, 0);
-
-	mutex_lock(&big_key_aead_lock);
-	if (crypto_aead_setkey(big_key_aead, key, ENC_KEY_SIZE)) {
+	if (crypto_aead_setkey(big_key_aead, key, ENC_KEY_SIZE))
 		ret = -EAGAIN;
-		goto error;
-	}
-	if (op == BIG_KEY_ENC)
+	else if (op == BIG_KEY_ENC)
 		ret = crypto_aead_encrypt(aead_req);
 	else
 		ret = crypto_aead_decrypt(aead_req);
-error:
 	mutex_unlock(&big_key_aead_lock);
 	aead_request_free(aead_req);
 	return ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tycho Andersen]

On Wed, Apr 25, 2018 at 07:36:21PM +0900, Tetsuo Handa wrote:
> Kees Cook wrote:
> > On Tue, Apr 24, 2018 at 12:58 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> > > Quoting Tycho Andersen (tycho at tycho.ws):
> > >> On Tue, Apr 24, 2018 at 11:46:38PM +0900, Tetsuo Handa wrote:
> > >> > Tycho Andersen wrote:
> > >> > > > > +     if (unlikely(crypto_aead_ivsize(big_key_aead) != GCM_AES_IV_SIZE)) {
> > >> > > > > +             WARN(1, "big key algorithm changed?");
> > >> >
> > >> > Please avoid using WARN() WARN_ON() etc.
> > >> > syzbot would catch it and panic() due to panic_on_warn == 1.
> > >>
> > >> But it is really a programming bug in this case (and it seems better
> > >> than BUG()...). Isn't this exactly the sort of case we want to catch?
> > >>
> > >> Tycho
> > >
> > > Right - is there a url to some discussion about this?  Because not
> > > using WARN when WARN should be used, because it troubles a bot, seems
> > > the wrong solution.  If this *is* what's been agreed upon, then
> > > what is the new recommended thing to do here?
> > 
> > BUG() is basically supposed to never be used, as decreed by Linus.
> > WARN() here is entirely correct: if we encounter a case where
> > crypto_aead_ivsize(big_key_aead) != GCM_AES_IV_SIZE is not true, we
> > run the risk of stack memory corruption. If this is an EXPECTED
> > failure case, then okay, drop the WARN() but we have to keep the
> > -EINVAL.
> 
> big_key_init() is __init function of built-in module which will be called
> only once upon boot, isn't it? Then, there is no point to continue after
> WARN(); BUG() is better here.

I don't think so. The machine can still boot and work just fine, but
big key crypto will not be available. I suspect there are some
machines out there that don't need big key, so there's no reason for
the boot to fail. That's the rub about WARN vs BUG -- that in most
cases things can continue on happily.

> Moreover, if this is meant for sanity check in case something went wrong
> (e.g. memory corruption), it is better to check at run time like

But the algorithm is hard coded at the top of the file, so one check
is enough.

Tycho
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Biggers]

On Tue, Apr 24, 2018 at 02:58:45PM -0500, Serge E. Hallyn wrote:
> Quoting Tycho Andersen (tycho at tycho.ws):
> > On Tue, Apr 24, 2018 at 11:46:38PM +0900, Tetsuo Handa wrote:
> > > Tycho Andersen wrote:
> > > > > > +	if (unlikely(crypto_aead_ivsize(big_key_aead) != GCM_AES_IV_SIZE)) {
> > > > > > +		WARN(1, "big key algorithm changed?");
> > > 
> > > Please avoid using WARN() WARN_ON() etc.
> > > syzbot would catch it and panic() due to panic_on_warn == 1.
> > 
> > But it is really a programming bug in this case (and it seems better
> > than BUG()...). Isn't this exactly the sort of case we want to catch?
> > 
> > Tycho
> 
> Right - is there a url to some discussion about this?  Because not
> using WARN when WARN should be used, because it troubles a bot, seems
> the wrong solution.  If this *is* what's been agreed upon, then
> what is the new recommended thing to do here?
> 
> -serge

WARN() is for recoverable kernel bugs, which this is, so WARN() is correct here.
Fuzzers often find cases where WARN() is used on invalid user input or other
cases that are not kernel bugs, and then it has to be removed or replaced with
pr_warn().  But here it is appropriate.  Unfortunately a lot of developers still
seem confused; improving the comments in include/asm-generic/bug.h might help.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html