[PATCH] MODSIGN: Only sign modules if built in-tree

[PATCH] MODSIGN: Only sign modules if built in-tree

[Josh Boyer]

When building out-of-tree modules, the current modules_install target
will attempt to sign them if module signing is enabled.  This will only
work if the signing keys are present in the build tree.  That will
often not be the case for modules that are built out-of-tree against
distribution kernel development packages.  This distros will not include
the signing keys, and build errors such as:

    INSTALL /home/bruno/rpmbuild/BUILD/dahdi-linux-2.6.1/drivers/dahdi/dahdi_dyamic_eth.ko
    Can't read private key
    make[2]: *** [/home/bruno/rpmbuild/BUILD/dahdi-linux-2.6.1/drivers/dahdi/dahdi_dynamic.ko] Error 2

will prevent such modules from successfully being installed.  This changes
the mod_sign_cmd to only sign the modules if they are built in-tree.  Those
built externally can sign them manually.

Reported-by: Bruno Wolff III <bruno@wolff.to>
Signed-off-by: Josh Boyer <jwboyer@redhat.com>
---
 Makefile | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Makefile b/Makefile
index 42d0e56..3d10a87 100644
--- a/Makefile
+++ b/Makefile
@@ -720,6 +720,7 @@ export mod_strip_cmd
 
 
 ifeq ($(CONFIG_MODULE_SIG),y)
+ifeq ($(KBUILD_EXTMOD),)
 MODSECKEY = ./signing_key.priv
 MODPUBKEY = ./signing_key.x509
 export MODPUBKEY
@@ -727,6 +728,9 @@ mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
 else
 mod_sign_cmd = true
 endif
+else
+mod_sign_cmd = true
+endif
 export mod_sign_cmd
 
 
-- 
1.7.12.1

Re: [PATCH] MODSIGN: Only sign modules if built in-tree

[Rusty Russell]

Josh Boyer <jwboyer@redhat.com> writes:
> When building out-of-tree modules, the current modules_install target
> will attempt to sign them if module signing is enabled.  This will only
> work if the signing keys are present in the build tree.  That will
> often not be the case for modules that are built out-of-tree against
> distribution kernel development packages.  This distros will not include
> the signing keys, and build errors such as:
>
>     INSTALL /home/bruno/rpmbuild/BUILD/dahdi-linux-2.6.1/drivers/dahdi/dahdi_dyamic_eth.ko
>     Can't read private key
>     make[2]: *** [/home/bruno/rpmbuild/BUILD/dahdi-linux-2.6.1/drivers/dahdi/dahdi_dynamic.ko] Error 2
>
> will prevent such modules from successfully being installed.  This changes
> the mod_sign_cmd to only sign the modules if they are built in-tree.  Those
> built externally can sign them manually.

I prefer something like this (untested):

diff --git a/Makefile b/Makefile
index 42d0e56..cb66c8d 100644
--- a/Makefile
+++ b/Makefile
@@ -722,8 +722,14 @@ export mod_strip_cmd
 ifeq ($(CONFIG_MODULE_SIG),y)
 MODSECKEY = ./signing_key.priv
 MODPUBKEY = ./signing_key.x509
+ifeq ($(KBUILD_EXTMOD),)
+SIGNFAIL = false
+else
+# External builds might not have a signing key, don't break module_install.
+SIGNFAIL = true
+endif # KBUILD_EXTMOD
 export MODPUBKEY
-mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
+mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY) || $(SIGNFAIL)
 else
 mod_sign_cmd = true
 endif

Re: [PATCH] MODSIGN: Only sign modules if built in-tree

[Josh Boyer]

On Thu, Nov 01, 2012 at 05:38:15PM +1030, Rusty Russell wrote:
> Josh Boyer <jwboyer@redhat.com> writes:
> > When building out-of-tree modules, the current modules_install target
> > will attempt to sign them if module signing is enabled.  This will only
> > work if the signing keys are present in the build tree.  That will
> > often not be the case for modules that are built out-of-tree against
> > distribution kernel development packages.  This distros will not include
> > the signing keys, and build errors such as:
> >
> >     INSTALL /home/bruno/rpmbuild/BUILD/dahdi-linux-2.6.1/drivers/dahdi/dahdi_dyamic_eth.ko
> >     Can't read private key
> >     make[2]: *** [/home/bruno/rpmbuild/BUILD/dahdi-linux-2.6.1/drivers/dahdi/dahdi_dynamic.ko] Error 2
> >
> > will prevent such modules from successfully being installed.  This changes
> > the mod_sign_cmd to only sign the modules if they are built in-tree.  Those
> > built externally can sign them manually.
> 
> I prefer something like this (untested):
> 
> diff --git a/Makefile b/Makefile
> index 42d0e56..cb66c8d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -722,8 +722,14 @@ export mod_strip_cmd
>  ifeq ($(CONFIG_MODULE_SIG),y)
>  MODSECKEY = ./signing_key.priv
>  MODPUBKEY = ./signing_key.x509
> +ifeq ($(KBUILD_EXTMOD),)
> +SIGNFAIL = false
> +else
> +# External builds might not have a signing key, don't break module_install.
> +SIGNFAIL = true
> +endif # KBUILD_EXTMOD
>  export MODPUBKEY
> -mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
> +mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY) || $(SIGNFAIL)
>  else
>  mod_sign_cmd = true
>  endif

OK.  I'll give this a spin locally today, but at first glance it seems
like it would do the same.

josh

Re: [PATCH] MODSIGN: Only sign modules if built in-tree

[Josh Boyer]

On Thu, Nov 01, 2012 at 07:26:55AM -0400, Josh Boyer wrote:
> > I prefer something like this (untested):
> > 
> > diff --git a/Makefile b/Makefile
> > index 42d0e56..cb66c8d 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -722,8 +722,14 @@ export mod_strip_cmd
> >  ifeq ($(CONFIG_MODULE_SIG),y)
> >  MODSECKEY = ./signing_key.priv
> >  MODPUBKEY = ./signing_key.x509
> > +ifeq ($(KBUILD_EXTMOD),)
> > +SIGNFAIL = false
> > +else
> > +# External builds might not have a signing key, don't break module_install.
> > +SIGNFAIL = true
> > +endif # KBUILD_EXTMOD
> >  export MODPUBKEY
> > -mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
> > +mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY) || $(SIGNFAIL)
> >  else
> >  mod_sign_cmd = true
> >  endif
> 
> OK.  I'll give this a spin locally today, but at first glance it seems
> like it would do the same.

We need to put $(SIGNFAIL) before the perl script invocation or we get
errors because mod_sign_cmd is passed an argument and sign-file is
treating the "|| $(SIGNFAIL)" as something it's passed.  That was the
only change I needed to make and it works as expected.

Do you want me to send a v2 of the patch, or will you add it yourself
given you've basically written the code?  Either way works for me.

josh

Re: [PATCH] MODSIGN: Only sign modules if built in-tree

[Rusty Russell]

Josh Boyer <jwboyer@redhat.com> writes:

> On Thu, Nov 01, 2012 at 07:26:55AM -0400, Josh Boyer wrote:
>> > I prefer something like this (untested):
>> > 
>> > diff --git a/Makefile b/Makefile
>> > index 42d0e56..cb66c8d 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -722,8 +722,14 @@ export mod_strip_cmd
>> >  ifeq ($(CONFIG_MODULE_SIG),y)
>> >  MODSECKEY = ./signing_key.priv
>> >  MODPUBKEY = ./signing_key.x509
>> > +ifeq ($(KBUILD_EXTMOD),)
>> > +SIGNFAIL = false
>> > +else
>> > +# External builds might not have a signing key, don't break module_install.
>> > +SIGNFAIL = true
>> > +endif # KBUILD_EXTMOD
>> >  export MODPUBKEY
>> > -mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
>> > +mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY) || $(SIGNFAIL)
>> >  else
>> >  mod_sign_cmd = true
>> >  endif
>> 
>> OK.  I'll give this a spin locally today, but at first glance it seems
>> like it would do the same.
>
> We need to put $(SIGNFAIL) before the perl script invocation or we get
> errors because mod_sign_cmd is passed an argument and sign-file is
> treating the "|| $(SIGNFAIL)" as something it's passed.  That was the
> only change I needed to make and it works as expected.
>
> Do you want me to send a v2 of the patch, or will you add it yourself
> given you've basically written the code?  Either way works for me.

Please re-submit.

Thanks,
Rusty.

[PATCH v2] MODSIGN: Only sign modules if built in-tree

[Josh Boyer]

When building out-of-tree modules, the current modules_install target
will attempt to sign them if module signing is enabled.  This will only
work if the signing keys are present in the build tree.  That will
often not be the case for modules that are built out-of-tree against
distribution kernel development packages.  This distros will not include
the signing keys, and build errors such as:

    INSTALL /home/bruno/rpmbuild/BUILD/dahdi-linux-2.6.1/drivers/dahdi/dahdi_dya
mic_eth.ko
    Can't read private key
    make[2]: *** [/home/bruno/rpmbuild/BUILD/dahdi-linux-2.6.1/drivers/dahdi/dah
di_dynamic.ko] Error 2

will prevent such modules from successfully being installed.  This changes
the mod_sign_cmd to only sign the modules if they are built in-tree.  Those
built externally can sign them manually.

Reported-by: Bruno Wolff III <bruno@wolff.to>
Suggested-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Josh Boyer <jwboyer@redhat.com>
---
 Makefile | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 14c93b3..7e27d51 100644
--- a/Makefile
+++ b/Makefile
@@ -722,8 +722,14 @@ export mod_strip_cmd
 ifeq ($(CONFIG_MODULE_SIG),y)
 MODSECKEY = ./signing_key.priv
 MODPUBKEY = ./signing_key.x509
+ifeq ($(KBUILD_EXTMOD),)
+SIGNFAIL = false
+else
+# External builds might not have a signing key, don't break module_install.
+SIGNFAIL = true
+endif # KBUILD_EXTMOD
 export MODPUBKEY
-mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
+mod_sign_cmd = $(SIGNFAIL) || perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
 else
 mod_sign_cmd = true
 endif
-- 
1.7.12.1

Re: [PATCH v2] MODSIGN: Only sign modules if built in-tree

[Rusty Russell]

Josh Boyer <jwboyer@redhat.com> writes:
> diff --git a/Makefile b/Makefile
> index 14c93b3..7e27d51 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -722,8 +722,14 @@ export mod_strip_cmd
>  ifeq ($(CONFIG_MODULE_SIG),y)
>  MODSECKEY = ./signing_key.priv
>  MODPUBKEY = ./signing_key.x509
> +ifeq ($(KBUILD_EXTMOD),)
> +SIGNFAIL = false
> +else
> +# External builds might not have a signing key, don't break module_install.
> +SIGNFAIL = true
> +endif # KBUILD_EXTMOD
>  export MODPUBKEY
> -mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
> +mod_sign_cmd = $(SIGNFAIL) || perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
>  else
>  mod_sign_cmd = true
>  endif

Huh?  'true || perl' never runs perl due to short circuiting.

Let's do this instead.  Tested here, please ack.

Thanks,
Rusty.

modules: don't break modules_install on external modules with no key.

The script still spits out an error ("Can't read private key") but we
don't break modules_install.

Reported-by: Bruno Wolff III <bruno@wolff.to>
Original-patch-by: Josh Boyer <jwboyer@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst
index dda4b2b..ecbb447 100644
--- a/scripts/Makefile.modinst
+++ b/scripts/Makefile.modinst
@@ -16,8 +16,9 @@ PHONY += $(modules)
 __modinst: $(modules)
 	@:
 
+# Don't stop modules_install if we can't sign external modules.
 quiet_cmd_modules_install = INSTALL $@
-      cmd_modules_install = mkdir -p $(2); cp $@ $(2) ; $(mod_strip_cmd) $(2)/$(notdir $@) ; $(mod_sign_cmd) $(2)/$(notdir $@)
+      cmd_modules_install = mkdir -p $(2); cp $@ $(2) ; $(mod_strip_cmd) $(2)/$(notdir $@) ; $(mod_sign_cmd) $(2)/$(notdir $@) $(patsubst %,|| true,$(KBUILD_EXTMOD))
 
 # Modules built outside the kernel source tree go into extra by default
 INSTALL_MOD_DIR ?= extra

Re: [PATCH v2] MODSIGN: Only sign modules if built in-tree

[Josh Boyer]

On Mon, Nov 05, 2012 at 12:31:39PM +1030, Rusty Russell wrote:
> Josh Boyer <jwboyer@redhat.com> writes:
> > diff --git a/Makefile b/Makefile
> > index 14c93b3..7e27d51 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -722,8 +722,14 @@ export mod_strip_cmd
> >  ifeq ($(CONFIG_MODULE_SIG),y)
> >  MODSECKEY = ./signing_key.priv
> >  MODPUBKEY = ./signing_key.x509
> > +ifeq ($(KBUILD_EXTMOD),)
> > +SIGNFAIL = false
> > +else
> > +# External builds might not have a signing key, don't break module_install.
> > +SIGNFAIL = true
> > +endif # KBUILD_EXTMOD
> >  export MODPUBKEY
> > -mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
> > +mod_sign_cmd = $(SIGNFAIL) || perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
> >  else
> >  mod_sign_cmd = true
> >  endif
> 
> Huh?  'true || perl' never runs perl due to short circuiting.

Right.  Why would we even bother running the perl fragment for
out-of-tree modules, when the entire purpose of the patch was to not
sign out-of-tree modules?

> Let's do this instead.  Tested here, please ack.

Sure, will test shortly.

josh

Re: [PATCH v2] MODSIGN: Only sign modules if built in-tree

[Josh Boyer]

On Mon, Nov 05, 2012 at 12:31:39PM +1030, Rusty Russell wrote:
> Josh Boyer <jwboyer@redhat.com> writes:
> > diff --git a/Makefile b/Makefile
> > index 14c93b3..7e27d51 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -722,8 +722,14 @@ export mod_strip_cmd
> >  ifeq ($(CONFIG_MODULE_SIG),y)
> >  MODSECKEY = ./signing_key.priv
> >  MODPUBKEY = ./signing_key.x509
> > +ifeq ($(KBUILD_EXTMOD),)
> > +SIGNFAIL = false
> > +else
> > +# External builds might not have a signing key, don't break module_install.
> > +SIGNFAIL = true
> > +endif # KBUILD_EXTMOD
> >  export MODPUBKEY
> > -mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
> > +mod_sign_cmd = $(SIGNFAIL) || perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
> >  else
> >  mod_sign_cmd = true
> >  endif
> 
> Huh?  'true || perl' never runs perl due to short circuiting.

Ah.  Maybe you were going for "sign all modules if keys are available,
but don't break external if they aren't" semantics.  I was just skipping
it entirely for external modules.

> Let's do this instead.  Tested here, please ack.

Either method works for me, and I tested this locally as you asked.
Seems to be working well.

> modules: don't break modules_install on external modules with no key.
> 
> The script still spits out an error ("Can't read private key") but we
> don't break modules_install.
> 
> Reported-by: Bruno Wolff III <bruno@wolff.to>
> Original-patch-by: Josh Boyer <jwboyer@redhat.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Acked-by: Josh Boyer <jwboyer@redhat.com>

josh

> 
> diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst
> index dda4b2b..ecbb447 100644
> --- a/scripts/Makefile.modinst
> +++ b/scripts/Makefile.modinst
> @@ -16,8 +16,9 @@ PHONY += $(modules)
>  __modinst: $(modules)
>  	@:
>  
> +# Don't stop modules_install if we can't sign external modules.
>  quiet_cmd_modules_install = INSTALL $@
> -      cmd_modules_install = mkdir -p $(2); cp $@ $(2) ; $(mod_strip_cmd) $(2)/$(notdir $@) ; $(mod_sign_cmd) $(2)/$(notdir $@)
> +      cmd_modules_install = mkdir -p $(2); cp $@ $(2) ; $(mod_strip_cmd) $(2)/$(notdir $@) ; $(mod_sign_cmd) $(2)/$(notdir $@) $(patsubst %,|| true,$(KBUILD_EXTMOD))
>  
>  # Modules built outside the kernel source tree go into extra by default
>  INSTALL_MOD_DIR ?= extra

Re: [PATCH v2] MODSIGN: Only sign modules if built in-tree

[Rusty Russell]

Josh Boyer <jwboyer@redhat.com> writes:
> On Mon, Nov 05, 2012 at 12:31:39PM +1030, Rusty Russell wrote:
>> Josh Boyer <jwboyer@redhat.com> writes:
>> > diff --git a/Makefile b/Makefile
>> > index 14c93b3..7e27d51 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -722,8 +722,14 @@ export mod_strip_cmd
>> >  ifeq ($(CONFIG_MODULE_SIG),y)
>> >  MODSECKEY = ./signing_key.priv
>> >  MODPUBKEY = ./signing_key.x509
>> > +ifeq ($(KBUILD_EXTMOD),)
>> > +SIGNFAIL = false
>> > +else
>> > +# External builds might not have a signing key, don't break module_install.
>> > +SIGNFAIL = true
>> > +endif # KBUILD_EXTMOD
>> >  export MODPUBKEY
>> > -mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
>> > +mod_sign_cmd = $(SIGNFAIL) || perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
>> >  else
>> >  mod_sign_cmd = true
>> >  endif
>> 
>> Huh?  'true || perl' never runs perl due to short circuiting.
>
> Ah.  Maybe you were going for "sign all modules if keys are available,
> but don't break external if they aren't" semantics.  I was just skipping
> it entirely for external modules.

Exactly.  This way you get warnings, not failure.  You probably want
that, since you'll (at least) taint your kernel when you load those
modules.

I've applied this in my fixes branch, will push to Linus later this
week.

Thanks,
Rusty.

Re: [PATCH v2] MODSIGN: Only sign modules if built in-tree

[Josh Boyer]

On Tue, Nov 06, 2012 at 12:04:02PM +1030, Rusty Russell wrote:
> Josh Boyer <jwboyer@redhat.com> writes:
> > On Mon, Nov 05, 2012 at 12:31:39PM +1030, Rusty Russell wrote:
> >> Josh Boyer <jwboyer@redhat.com> writes:
> >> > diff --git a/Makefile b/Makefile
> >> > index 14c93b3..7e27d51 100644
> >> > --- a/Makefile
> >> > +++ b/Makefile
> >> > @@ -722,8 +722,14 @@ export mod_strip_cmd
> >> >  ifeq ($(CONFIG_MODULE_SIG),y)
> >> >  MODSECKEY = ./signing_key.priv
> >> >  MODPUBKEY = ./signing_key.x509
> >> > +ifeq ($(KBUILD_EXTMOD),)
> >> > +SIGNFAIL = false
> >> > +else
> >> > +# External builds might not have a signing key, don't break module_install.
> >> > +SIGNFAIL = true
> >> > +endif # KBUILD_EXTMOD
> >> >  export MODPUBKEY
> >> > -mod_sign_cmd = perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
> >> > +mod_sign_cmd = $(SIGNFAIL) || perl $(srctree)/scripts/sign-file $(MODSECKEY) $(MODPUBKEY)
> >> >  else
> >> >  mod_sign_cmd = true
> >> >  endif
> >> 
> >> Huh?  'true || perl' never runs perl due to short circuiting.
> >
> > Ah.  Maybe you were going for "sign all modules if keys are available,
> > but don't break external if they aren't" semantics.  I was just skipping
> > it entirely for external modules.
> 
> Exactly.  This way you get warnings, not failure.  You probably want
> that, since you'll (at least) taint your kernel when you load those
> modules.
> 
> I've applied this in my fixes branch, will push to Linus later this
> week.

Great.  Thanks Rusty!

josh

Re: [PATCH v2] MODSIGN: Only sign modules if built in-tree

[Bruno Wolff III]

On Tue, Nov 06, 2012 at 07:54:17 -0500,
   Josh Boyer <jwboyer@redhat.com> wrote:
>On Tue, Nov 06, 2012 at 12:04:02PM +1030, Rusty Russell wrote:
>>
>> I've applied this in my fixes branch, will push to Linus later this
>> week.
>
>Great.  Thanks Rusty!
>
>josh

I tested building dahdi-linux with kernel-3.7.0-0.rc4.git1.1.fc19 which 
has this patch and the build went fine. I won't be able to test that the 
module works until tonight, but even if it ends up being broken the signing 
patch is unlikely to be the cause.

Re: [PATCH v2] MODSIGN: Only sign modules if built in-tree

[Bruno Wolff III]

On Wed, Nov 07, 2012 at 13:21:41 -0600,
   Bruno Wolff III <bruno@wolff.to> wrote:
>
>I tested building dahdi-linux with kernel-3.7.0-0.rc4.git1.1.fc19 
>which has this patch and the build went fine. I won't be able to test 
>that the module works until tonight, but even if it ends up being 
>broken the signing patch is unlikely to be the cause.

The module seems to be working correctly. So everything looks good.