[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Wed, 25 Apr 2018 08:15:07 -0600
From: Tycho Andersen <tycho@...ho.ws>
To: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc: keescook@...omium.org, serge@...lyn.com, ebiggers3@...il.com,
dhowells@...hat.com, keyrings@...r.kernel.org,
linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org,
kernel-hardening@...ts.openwall.com, jmorris@...ei.org,
Jason@...c4.com
Subject: Re: [PATCH 1/3] big key: get rid of stack array allocation
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@...lyn.com> wrote:
> > > Quoting Tycho Andersen (tycho@...ho.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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
