| Message ID | 20180424010321.14739-1-tycho@tycho.ws (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Tycho Andersen (tycho@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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 24, 2018 at 12:58 PM, Serge E. Hallyn <serge@hallyn.com> wrote: > Quoting Tycho Andersen (tycho@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
On Tue, Apr 24, 2018 at 02:58:45PM -0500, Serge E. Hallyn wrote: > Quoting Tycho Andersen (tycho@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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)