Date:	Wed, 20 May 2015 11:17:01 +0100
From:	David Woodhouse <dwmw2@...radead.org>
To:	Abelardo Ricart III <aricart@...nix.com>,
	Michal Marek <mmarek@...e.cz>
Cc:	torvalds@...ux-foundation.org, linux-kernel@...r.kernel.org,
	sedat.dilek@...il.com, dhowells@...hat.com, keyrings@...ux-nfs.org,
	rusty@...tcorp.com.au, linux-security-module@...r.kernel.org,
	james.l.morris@...cle.com, gregkh@...uxfoundation.org
Subject: Re: [PATCH] MODSIGN: Change default key details [ver #2]

On Fri, 2015-05-01 at 17:41 -0400, Abelardo Ricart III wrote:
> 
> From module-signing.txt:
> 
> > Under normal conditions, the kernel build will automatically generate a new
> > keypair using openssl if one does not exist in the files:
> >
> >        signing_key.priv
> >        signing_key.x509
> 
> Nope, sorry, not true. Even if your keys exist, due to unfortunate 
> parallel make/disk write order/racy kbuild/goblins your x509.genkey 
> file has a newer timestamp than your keys, and now your keys are 
> going to get tossed (regenerated and overwritten, yay!). Worse still, 
> I think they could even get tossed AFTER the build decides "hey nice 
> keys, I'll just use those". Either way, this patch is ultimately 
> correct because this is exactly the kind of racy scenario order-only 
> prerequisites was made for. We should not care anything about 
> x509.genkey if our signing keys already exist. Period.
> 
> Here's my two-line patch strictly defining the build order, for your perusal.
> 
> Signed-off-by: Abelardo Ricart III <aricart@...nix.com>
> ---
> 
> diff --git a/kernel/Makefile b/kernel/Makefile
> index 1408b33..10c8df0 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -168,7 +168,8 @@ ifndef CONFIG_MODULE_SIG_HASH
>  $(error Could not determine digest type to use from kernel config)
>  endif
>  
> -signing_key.priv signing_key.x509: x509.genkey
> +signing_key.priv signing_key.x509: | x509.genkey
> +       $(warning *** X.509 module signing key pair not found in root of source tree ***)
>         @echo "###"
>         @echo "### Now generating an X.509 key pair to be used for signing modules."
>         @echo "###"

I think we still need this, or something equivalent. I'll take a closer
look at the dependencies and see if there's a better option — that bit
about keys being tossed AFTER the build is already using them does
definitely seem like a bug.

This approach might suffice as a last resort, but I don't like it much.
For an ephemeral key, we *do* want to rebuild it if you touch
x509.genkey. And signing_key.{priv,x509} *are* now purely intended to
be ephemeral — see 
https://lkml.org/lkml/2015/5/18/527 and my patches which David Howells
has now merged into his tree.

I suspect the real bug here will turn out to be caused by the fact that
signing_key.priv and signing_key.x509 are distinct Makefile targets and
we are *also* generating each as a side-effect of generating the other.
And make doesn't know about the side-effects.

Maybe we'd do better with a single rule for 'signing_key.pem' which
contains both key and cert. Which is the way it is for an external key
anyway. I'll look at implementing that.

I also wonder if the problem that Linus saw with "X.509 certificate
list changed" was a different issue, in the $(wildcard *.x509)
handling. I am increasingly convinced we should just ditch that entire
can of worms and take a single file named in a Kconfig option. I'll throw that together too and see if it makes me 
happy.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@...el.com                              Intel Corporation

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (5691 bytes)

Powered by blists - more mailing lists