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 <[email protected]>
Signed-off-by: Josh Boyer <[email protected]>
---
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
Josh Boyer <[email protected]> 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
On Thu, Nov 01, 2012 at 05:38:15PM +1030, Rusty Russell wrote:
> Josh Boyer <[email protected]> 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
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
Josh Boyer <[email protected]> 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.
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 <[email protected]>
Suggested-by: Rusty Russell <[email protected]>
Signed-off-by: Josh Boyer <[email protected]>
---
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
Josh Boyer <[email protected]> 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 <[email protected]>
Original-patch-by: Josh Boyer <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
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
On Mon, Nov 05, 2012 at 12:31:39PM +1030, Rusty Russell wrote:
> Josh Boyer <[email protected]> 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
On Mon, Nov 05, 2012 at 12:31:39PM +1030, Rusty Russell wrote:
> Josh Boyer <[email protected]> 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 <[email protected]>
> Original-patch-by: Josh Boyer <[email protected]>
> Signed-off-by: Rusty Russell <[email protected]>
Acked-by: Josh Boyer <[email protected]>
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
Josh Boyer <[email protected]> writes:
> On Mon, Nov 05, 2012 at 12:31:39PM +1030, Rusty Russell wrote:
>> Josh Boyer <[email protected]> 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.
On Tue, Nov 06, 2012 at 12:04:02PM +1030, Rusty Russell wrote:
> Josh Boyer <[email protected]> writes:
> > On Mon, Nov 05, 2012 at 12:31:39PM +1030, Rusty Russell wrote:
> >> Josh Boyer <[email protected]> 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
On Tue, Nov 06, 2012 at 07:54:17 -0500,
Josh Boyer <[email protected]> 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.
On Wed, Nov 07, 2012 at 13:21:41 -0600,
Bruno Wolff III <[email protected]> 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.