[SRU,E/Unstable] UBUNTU: [Packaging] Fix module signing with older modinfo
diff mbox series

Message ID 20191118153917.691813-1-seth.forshee@canonical.com
State New
Headers show
Series
  • [SRU,E/Unstable] UBUNTU: [Packaging] Fix module signing with older modinfo
Related show

Commit Message

Seth Forshee Nov. 18, 2019, 3:39 p.m. UTC
BugLink: https://bugs.launchpad.net/bugs/1852581

Not all versions of modinfo support the signer field;
specifically, the version in boinic does not. This leaves all
modules unsigned in hwe kernels based on eoan and later. Change
the check to look for the magic string at the end of the module,
which does not rely on any external tools being aware of module
signatures.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 debian/rules.d/2-binary-arch.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Thadeu Lima de Souza Cascardo Nov. 18, 2019, 3:58 p.m. UTC | #1
Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Kleber Souza Nov. 19, 2019, 4:59 p.m. UTC | #2
On 18.11.19 16:39, Seth Forshee wrote:
> BugLink: https://bugs.launchpad.net/bugs/1852581
> 
> Not all versions of modinfo support the signer field;
> specifically, the version in boinic does not. This leaves all
> modules unsigned in hwe kernels based on eoan and later. Change
> the check to look for the magic string at the end of the module,
> which does not rely on any external tools being aware of module
> signatures.
> 
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

> ---
>  debian/rules.d/2-binary-arch.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk
> index 050f867060cb..070478e010f7 100644
> --- a/debian/rules.d/2-binary-arch.mk
> +++ b/debian/rules.d/2-binary-arch.mk
> @@ -413,12 +413,12 @@ ifneq ($(skipdbg),true)
>  	  -name '*.ko' | while read path_module ; do \
>  		module="/lib/modules/$${path_module#*/lib/modules/}"; \
>  		if [[ -f "$(dbgpkgdir)/usr/lib/debug/$$module" ]] ; then \
> -			signer=$$(/sbin/modinfo -F signer "$$path_module"); \
> +			signature=$$(tail -c 28 "$$path_module"); \
>  			$(CROSS_COMPILE)objcopy \
>  				--add-gnu-debuglink=$(dbgpkgdir)/usr/lib/debug/$$module \
>  				$$path_module; \
>  			if grep -q CONFIG_MODULE_SIG=y $(builddir)/build-$*/.config && \
> -			   [ -n "$$signer" ]; then \
> +			   [ "$$signature" = "~Module signature appended~" ]; then \
>  				$(builddir)/build-$*/scripts/sign-file $(MODHASHALGO) \
>  					$(MODSECKEY) \
>  					$(MODPUBKEY) \
>
Kleber Souza Nov. 21, 2019, 2:19 p.m. UTC | #3
On 18.11.19 16:39, Seth Forshee wrote:
> BugLink: https://bugs.launchpad.net/bugs/1852581
> 
> Not all versions of modinfo support the signer field;
> specifically, the version in boinic does not. This leaves all
> modules unsigned in hwe kernels based on eoan and later. Change
> the check to look for the magic string at the end of the module,
> which does not rely on any external tools being aware of module
> signatures.
> 
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  debian/rules.d/2-binary-arch.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk
> index 050f867060cb..070478e010f7 100644
> --- a/debian/rules.d/2-binary-arch.mk
> +++ b/debian/rules.d/2-binary-arch.mk
> @@ -413,12 +413,12 @@ ifneq ($(skipdbg),true)
>  	  -name '*.ko' | while read path_module ; do \
>  		module="/lib/modules/$${path_module#*/lib/modules/}"; \
>  		if [[ -f "$(dbgpkgdir)/usr/lib/debug/$$module" ]] ; then \
> -			signer=$$(/sbin/modinfo -F signer "$$path_module"); \
> +			signature=$$(tail -c 28 "$$path_module"); \
>  			$(CROSS_COMPILE)objcopy \
>  				--add-gnu-debuglink=$(dbgpkgdir)/usr/lib/debug/$$module \
>  				$$path_module; \
>  			if grep -q CONFIG_MODULE_SIG=y $(builddir)/build-$*/.config && \
> -			   [ -n "$$signer" ]; then \
> +			   [ "$$signature" = "~Module signature appended~" ]; then \
>  				$(builddir)/build-$*/scripts/sign-file $(MODHASHALGO) \
>  					$(MODSECKEY) \
>  					$(MODPUBKEY) \
> 

Applied to eoan/master-next branch.

Thanks,
Kleber
Kleber Souza Nov. 22, 2019, 10:36 a.m. UTC | #4
On 18.11.19 16:39, Seth Forshee wrote:
> BugLink: https://bugs.launchpad.net/bugs/1852581
> 
> Not all versions of modinfo support the signer field;
> specifically, the version in boinic does not. This leaves all
> modules unsigned in hwe kernels based on eoan and later. Change
> the check to look for the magic string at the end of the module,
> which does not rely on any external tools being aware of module
> signatures.
> 
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  debian/rules.d/2-binary-arch.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk
> index 050f867060cb..070478e010f7 100644
> --- a/debian/rules.d/2-binary-arch.mk
> +++ b/debian/rules.d/2-binary-arch.mk
> @@ -413,12 +413,12 @@ ifneq ($(skipdbg),true)
>  	  -name '*.ko' | while read path_module ; do \
>  		module="/lib/modules/$${path_module#*/lib/modules/}"; \
>  		if [[ -f "$(dbgpkgdir)/usr/lib/debug/$$module" ]] ; then \
> -			signer=$$(/sbin/modinfo -F signer "$$path_module"); \
> +			signature=$$(tail -c 28 "$$path_module"); \

Hi Seth,

With this patch, we get the following warnings when building the virtual box
modules during the package build:

[...]
signing vboxguest.ko
II: dkms-build override dkms-build--virtualbox-guest found, executing
II: dkms-build build virtualbox-guest complete
# Add .gnu_debuglink sections to each stripped .ko
# pointing to unstripped verson
find /<<PKGBUILDDIR>>/debian/linux-modules-5.3.0-23-generic \
  /<<PKGBUILDDIR>>/debian/linux-modules-extra-5.3.0-23-generic \
  -name '*.ko' | while read path_module ; do \
        module="/lib/modules/${path_module#*/lib/modules/}"; \
        if [[ -f "/<<PKGBUILDDIR>>/debian/linux-image-unsigned-5.3.0-23-generic-dbgsym/usr/lib/debug
/$module" ]] ; then \
                signature=$(tail -c 28 "$path_module"); \
                objcopy \
                        --add-gnu-debuglink=/<<PKGBUILDDIR>>/debian/linux-image-unsigned-5.3.0-23-ge
neric-dbgsym/usr/lib/debug/$module \
                        $path_module; \
                if grep -q CONFIG_MODULE_SIG=y /<<PKGBUILDDIR>>/debian/build/build-generic/.config &
& \
                   [ "$signature" = "~Module signature appended~" ]; then \
                        /<<PKGBUILDDIR>>/debian/build/build-generic/scripts/sign-file sha512 \
                                /<<PKGBUILDDIR>>/debian/build/build-generic/certs/signing_key.pem \
                                /<<PKGBUILDDIR>>/debian/build/build-generic/certs/signing_key.x509 \
                                $path_module; \
                fi; \
        else \
                echo "WARNING: Missing debug symbols for module '$module'."; \
        fi; \
done
WARNING: Missing debug symbols for module '/lib/modules/5.3.0-23-generic/kernel/virtualbox-guest/vbo
xsf.ko'.
WARNING: Missing debug symbols for module '/lib/modules/5.3.0-23-generic/kernel/virtualbox-guest/vbo
xguest.ko'.
/bin/bash: line 5: warning: command substitution: ignored null byte in input
/bin/bash: line 5: warning: command substitution: ignored null byte in input
/bin/bash: line 5: warning: command substitution: ignored null byte in input
[...]

For the first one I'm not sure there's anything we can do, but the second one repeats
hundreds of times and I suspect it's cause by the return of the 'tail' command.



Kleber



>  			$(CROSS_COMPILE)objcopy \
>  				--add-gnu-debuglink=$(dbgpkgdir)/usr/lib/debug/$$module \
>  				$$path_module; \
>  			if grep -q CONFIG_MODULE_SIG=y $(builddir)/build-$*/.config && \
> -			   [ -n "$$signer" ]; then \
> +			   [ "$$signature" = "~Module signature appended~" ]; then \
>  				$(builddir)/build-$*/scripts/sign-file $(MODHASHALGO) \
>  					$(MODSECKEY) \
>  					$(MODPUBKEY) \
>
Seth Forshee Nov. 22, 2019, 3:24 p.m. UTC | #5
On Fri, Nov 22, 2019 at 11:36:45AM +0100, Kleber Souza wrote:
> On 18.11.19 16:39, Seth Forshee wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1852581
> > 
> > Not all versions of modinfo support the signer field;
> > specifically, the version in boinic does not. This leaves all
> > modules unsigned in hwe kernels based on eoan and later. Change
> > the check to look for the magic string at the end of the module,
> > which does not rely on any external tools being aware of module
> > signatures.
> > 
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> >  debian/rules.d/2-binary-arch.mk | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk
> > index 050f867060cb..070478e010f7 100644
> > --- a/debian/rules.d/2-binary-arch.mk
> > +++ b/debian/rules.d/2-binary-arch.mk
> > @@ -413,12 +413,12 @@ ifneq ($(skipdbg),true)
> >  	  -name '*.ko' | while read path_module ; do \
> >  		module="/lib/modules/$${path_module#*/lib/modules/}"; \
> >  		if [[ -f "$(dbgpkgdir)/usr/lib/debug/$$module" ]] ; then \
> > -			signer=$$(/sbin/modinfo -F signer "$$path_module"); \
> > +			signature=$$(tail -c 28 "$$path_module"); \
> 
> Hi Seth,
> 
> With this patch, we get the following warnings when building the virtual box
> modules during the package build:
> 
> [...]
> signing vboxguest.ko
> II: dkms-build override dkms-build--virtualbox-guest found, executing
> II: dkms-build build virtualbox-guest complete
> # Add .gnu_debuglink sections to each stripped .ko
> # pointing to unstripped verson
> find /<<PKGBUILDDIR>>/debian/linux-modules-5.3.0-23-generic \
>   /<<PKGBUILDDIR>>/debian/linux-modules-extra-5.3.0-23-generic \
>   -name '*.ko' | while read path_module ; do \
>         module="/lib/modules/${path_module#*/lib/modules/}"; \
>         if [[ -f "/<<PKGBUILDDIR>>/debian/linux-image-unsigned-5.3.0-23-generic-dbgsym/usr/lib/debug
> /$module" ]] ; then \
>                 signature=$(tail -c 28 "$path_module"); \
>                 objcopy \
>                         --add-gnu-debuglink=/<<PKGBUILDDIR>>/debian/linux-image-unsigned-5.3.0-23-ge
> neric-dbgsym/usr/lib/debug/$module \
>                         $path_module; \
>                 if grep -q CONFIG_MODULE_SIG=y /<<PKGBUILDDIR>>/debian/build/build-generic/.config &
> & \
>                    [ "$signature" = "~Module signature appended~" ]; then \
>                         /<<PKGBUILDDIR>>/debian/build/build-generic/scripts/sign-file sha512 \
>                                 /<<PKGBUILDDIR>>/debian/build/build-generic/certs/signing_key.pem \
>                                 /<<PKGBUILDDIR>>/debian/build/build-generic/certs/signing_key.x509 \
>                                 $path_module; \
>                 fi; \
>         else \
>                 echo "WARNING: Missing debug symbols for module '$module'."; \
>         fi; \
> done
> WARNING: Missing debug symbols for module '/lib/modules/5.3.0-23-generic/kernel/virtualbox-guest/vbo
> xsf.ko'.
> WARNING: Missing debug symbols for module '/lib/modules/5.3.0-23-generic/kernel/virtualbox-guest/vbo
> xguest.ko'.
> /bin/bash: line 5: warning: command substitution: ignored null byte in input
> /bin/bash: line 5: warning: command substitution: ignored null byte in input
> /bin/bash: line 5: warning: command substitution: ignored null byte in input
> [...]
> 
> For the first one I'm not sure there's anything we can do,

That warning is printed by debian/rules.d/2-binary-arch.mk, but isn't a
result of this patch. But of course if there's no utility to having it
we could delete it.

> but the second one repeats
> hundreds of times and I suspect it's cause by the return of the 'tail' command.

Yes, that seems to be the case, when we feed it modules without
signatures. Every solution to this problem seems to have some kind of
gotcha. I'll take another crack at it.

Seth

Patch
diff mbox series

diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk
index 050f867060cb..070478e010f7 100644
--- a/debian/rules.d/2-binary-arch.mk
+++ b/debian/rules.d/2-binary-arch.mk
@@ -413,12 +413,12 @@  ifneq ($(skipdbg),true)
 	  -name '*.ko' | while read path_module ; do \
 		module="/lib/modules/$${path_module#*/lib/modules/}"; \
 		if [[ -f "$(dbgpkgdir)/usr/lib/debug/$$module" ]] ; then \
-			signer=$$(/sbin/modinfo -F signer "$$path_module"); \
+			signature=$$(tail -c 28 "$$path_module"); \
 			$(CROSS_COMPILE)objcopy \
 				--add-gnu-debuglink=$(dbgpkgdir)/usr/lib/debug/$$module \
 				$$path_module; \
 			if grep -q CONFIG_MODULE_SIG=y $(builddir)/build-$*/.config && \
-			   [ -n "$$signer" ]; then \
+			   [ "$$signature" = "~Module signature appended~" ]; then \
 				$(builddir)/build-$*/scripts/sign-file $(MODHASHALGO) \
 					$(MODSECKEY) \
 					$(MODPUBKEY) \