Skip to content

Conversation

@eric-ch
Copy link
Contributor

@eric-ch eric-ch commented Apr 13, 2021

Module signing and sstate mirroring can trigger failures (see OXT-1523) either at build-time (pseudo database corruption) or runtime (modules signed with a different key than the one embedded in the kernel binary). This changeset should rebuild the kernel and re-sign out-of-tree modules should the signing key change, whether or not the key is provided outside of the tree or the default generated one.

It should be recommended to use KERNEL_MODULE_SIG_CERT/KERNEL_MODULE_SIG_KEY locally. This will inevitably trigger kernel rebuilds initially without being able to use an sstate as the private key (albeit test ones), should not be shared like that. Kernel build times are not that bad though compared to webkitgtk and other UI components.

Copy link
Contributor

@jandryuk jandryuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ -z "${KERNEL_MODULE_SIG_CERT}" ] && [ -n "${KERNEL_MODULE_SIG_KEY}" ] is invalid, so maybe bbfatal on that?

Embedding a CERT but doing offline signing is valid, but that would break do_image_qa signature checking. Maybe that should be mode should be explicit instead of implicit from SIG_KEY bring unset. i.e. introduce KERNEL_MODULE_OFFLINE_SIGNING? That would then control do_sign_modules and do_image_qa?


# Set KERNEL_MODULE_SIG_KEY in local.conf to the filepath of a private key
# for signing kernel modules. If unset, signing can be done offline.
# for signing kernel modules.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can still be valid to have KERNEL_MODULE_SIG_KEY unset for offline signing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discarded that use-case to unblock the rest, I'll amend and add the bbfatal. KERNEL_MODULE_OFFLINE_SIGNING sounds fine, it will require changes in #1395 for do_image_qa.

@eric-ch
Copy link
Contributor Author

eric-ch commented Apr 28, 2021

While there are still things up for discussion, it sounds like 39a7b5f is not controversial, so I'll split it out of this PR and submit it separately to unbreak some trees using key/cert.

Using certain types of certificates rather than others also requires changes to the kernel configuration (e.g, default_md = sha256, or openssl req -sha256 requires CONFIG_CRYPTO_SHA256=y). So I think this should be refactored to guaranty that these conditions are met. This would currently require some sed invocation against the defconfig. Should #1383 be considered, it would only be a KERNEL_FEATURES with the relevant .scc.

@eric-ch
Copy link
Contributor Author

eric-ch commented Apr 29, 2021

  • Rebased on layer: run sign_module in fakeroot #1405
  • Allow offline signing, add a bbnote if that is detected
  • out-of-tree modules needed STAGING_KERNEL_BUILDDIR (at least with externalsrc) and not B
  • re-work the tests to error accurately when given an invalid configuration using out-of-tree certificates

Copy link
Contributor

@jandryuk jandryuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you

Eric Chanudet added 4 commits April 29, 2021 14:01
If module signing is enabled and key/cert are provided, add the
necessary dependencies between do_configure/do_sign_module to depend on
the out-of-tree key/cert.

If module signing is enabled and only cert is provided, add the
necessary dependency to do_configure, do_sign_module will not do
anything, offline signing is left to be done outside of OE.

If module signing is disabled, nothing is done, no dependency is added.

If module signing is enabled, but a certificate is not provided, error
out, this inevitably ends up re-generating a throw-away signing key when
out-of-tree modules dependencies are resolved (do_shared_workdir does
not store state, including signing keys).

Signed-off-by: Eric Chanudet <chanudete@ainfosec.com>
xarg can log to stdout the command it then runs. This is helpful to
figure out what was used in the event something goes wrong.

Signed-off-by: Eric Chanudet <chanudete@ainfosec.com>
No functional change.
It helps text editor color things right and seems to be the idiomatic
way from other recipes.

Signed-off-by: Eric Chanudet <chanudete@ainfosec.com>
out-of-tree modules will not have .config in ${B}.
Check the defconfig in STAGING_KERNEL_BUILDDIR instead.

Add a bbnote, failing this test is not fatal if signing was disabled for
a kernel that inherited this class. More correctly this class should not
be inherited if the build kernel/modules are not intended to support
module signing.

Signed-off-by: Eric Chanudet <chanudete@ainfosec.com>
@eric-ch
Copy link
Contributor Author

eric-ch commented Apr 29, 2021

Rebase for history coherency. No change.

return
fi
if [ -z "${KERNEL_MODULE_SIG_CERT}" ]; then
bbfatal "Kernel module signing should only be used when setting \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something to note here. The bbfatal message is printed and then bitbake prints the contents of log.do_configure, which is quite long with all the make oldconfig output. All you see at the end of it is:

make[1]: Entering directory '.../build'
  GEN     Makefile
# 
# No change to .config
# 
make[1]: Leaving directory '.../build'
make: Leaving directory '/home/build/openxt/build-201218/tmp-glibc/work-shared/usbvm/kernel-source'
WARNING: .../temp/run.do_configure.7382:1 exit 1 from 'exit 1'

ERROR: Logfile of failure stored in: .../temp/log.do_configure.7382

If you look back far enough you see the message. It's on line 2299 of the 4596 line log.do_configure. Not a blocker, but something I noticed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just bberror and exit 1 I guess.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That didn't change the output :/

@jandryuk
Copy link
Contributor

Will merge soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants