Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't brp-strip .ko files #1744

Merged
merged 1 commit into from Jul 12, 2021

Conversation

dmnks
Copy link
Contributor

@dmnks dmnks commented Jul 12, 2021

Otherwise SecureBoot signatures may be stripped too.

We used to exclude shared libraries from this strip as they were
supposed to be covered by another brp script (brp-strip-shared), however
it turned out the latter was never really used, so we removed the
exclusion in commit 0ab151a.

As it turns out, that was a little too ambitious, since we may now
inadvertently strip SecureBoot signatures from kernel modules too,
provided that they're made during the build, prior to the invocation of
brp-strip.

Note that this regression currently does not affect the following two
cases on Fedora/RHEL systems with redhat-rpm-config installed:

  • in-tree modules; these are built from kernel.spec which already
    contains a hack ensuring that module signing only happens after
    any stripping (see %__modsign_install_post in kernel.spec)

  • out-of-tree modules built with debuginfo enabled; this is because
    brp-strip is only called when %debug_package is set to %{nil}

Any other combinations may be affected, depending on the macros and
.spec files used, so let's fix this by effectively "reverting" said
commit for .ko files only.

Fixes: rhbz#1967291

Otherwise SecureBoot signatures may be stripped too.

We used to exclude shared libraries from this strip as they were
supposed to be covered by another brp script (brp-strip-shared), however
it turned out the latter was never really used, so we removed the
exclusion in commit 0ab151a.

As it turns out, that was a little too ambitious, since we may now
inadvertently strip SecureBoot signatures from kernel modules too,
provided that they're made during the build, prior to the invocation of
brp-strip.

Note that this regression currently does *not* affect the following two
cases on Fedora/RHEL systems with redhat-rpm-config installed:

  - in-tree kernel modules; these are built from kernel.spec which
    already contains a hack ensuring that module signing only happens
    *after* any stripping (see %__modsign_install_post in kernel.spec)

  - out-of-tree kernel modules built with debuginfo enabled; this is
    because brp-strip is only called when %debug_package is set to
    %{nil}

Any other combinations may be affected, depending on the macros and
.spec files used, so let's fix this by effectively "reverting" said
commit for .ko files only.

Fixes: rhbz#1967291
@ffesti ffesti merged commit cfdb830 into rpm-software-management:master Jul 12, 2021
@mikhailnov
Copy link
Contributor

Kernel modules may be *.ko.xz, *.ko.zst, *.ko.gz

@mikhailnov
Copy link
Contributor

Ah, sorry, such files probably will not be detected as ELFs

@dmnks
Copy link
Contributor Author

dmnks commented Jul 12, 2021

Yup, that's correct - they would be skipped by the loop anyway (and calling strip(1) on them would have no effect either).

@@ -13,5 +13,5 @@ Darwin*) exit 0 ;;
esac

# Strip ELF binaries
find "$RPM_BUILD_ROOT" -type f \! -regex "${RPM_BUILD_ROOT}/*usr/lib/debug.*" -print0 | \
find "$RPM_BUILD_ROOT" -type f \! -regex "${RPM_BUILD_ROOT}/*usr/lib/debug.*" \! -name "*.ko" -print0 | \
Copy link
Member

Choose a reason for hiding this comment

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

This should cover *.ko.(gz,xz,zst) too, shouldn't it?

Copy link
Member

Choose a reason for hiding this comment

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

blah, I get it now and I didn't see the comments earlier...

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.

None yet

4 participants