LKRG Lockdown

[kcdev809]

Description

This adds some code to bump the module refcount to prevent unloading LKRG. Only makes sense with kernel lockdown enabled and a kernel built with 'CONFIG_MODULE_FORCE_UNLOAD=n' .

How Has This Been Tested?

Module was compiled with changes and behaved as expected during testing.

[solardiz]

Thank you for the suggestion/contribution!

This adds some code to bump the module refcount to prevent unloading LKRG. Only makes sense with kernel lockdown enabled and a kernel built with 'CONFIG_MODULE_FORCE_UNLOAD=n' .

I think this is too much of a special case. If we add unload protection, it should be more general. Also, we need to maintain a consistent security model against a relevant threat model. There's little point in disallowing LKRG unload when we don't also disallow reconfiguration via sysctl that would effectively disable LKRG (while keeping it loaded). Rather, it'd make more sense to have a combined "no unload or configuration weakening" setting that would work for all installs.

So as much as we'd have liked to gain you as a contributor, I think we shouldn't merge this specific change now.

@Adam-pi3 @kerneltoast What do you think?

There are also some minor issues I see in the code, such as wrong indentation. But no point fixing this if we aren't going to merge this in principle.

[kerneltoast]

I don't think this makes much sense given that lockdown only has value when used in conjunction with secure boot. That means you'd need to get lkrg.ko signed by a secure boot CA or use your own secure boot key.

And per the spirit of lockdown, the sysctl knobs would indeed need to be locked down when the lockdown LSM is active. I don't think this effort is worth it right now.

[kcdev809]

Yes, this needs to be combined with further lockdown of certain config values.

[solardiz]

I dislike tying this functionality to lockdown in any way at all. I think we don't have to.

[kcdev809]

Kernel lockdown is a core part of kernel-mode security. Perhaps tying this could be set by a build-time flag?

[solardiz]

I really would rather keep our lockdown feature separate from kernel lockdown, working independently of whether kernel lockdown is supported/enabled. I don't want to clutter the project (and documentation!) with more build-time flags.

[solardiz]

Also, there was the idea of password-protected (we'd keep a hash) unload/reconfiguration. We'll probably want that eventually.

[kcdev809]

Also, there was the idea of password-protected (we'd keep a hash) unload/reconfiguration. We'll probably want that eventually.

That would be a good idea, gives users some more flexibility (fail safe in case of errors).
But on the other topic, I would consider implementing more build-time flags to give flexibility to distro package maintainers.
Tying LKRG lockdown to kernel lockdown may be desired if a distro is meant to have a secure & easy OOBE (especially since nowadays all the big distros have their own code signing CA for module signing, Secure Boot / Microsoft-Signed Shim, IMA).

[Adam-pi3]

@Adam-pi3 @kerneltoast What do you think?

It may have sense if we improve self-defense, hiding and lock sysctl as was mentioned by @solardiz and @kerneltoast. I think our old idea of passowrd protected unload (and sysctl) would likely be a good option to explore

[kcdev809]

I've changed my code to add a separate LKRG lockdown mechanism (optionally auto-invoked by kernel lockdown). Will be easier now to add sysctl protection.

EDIT: not yet added possibility of LKRG_LOCKED_DOWN=0 & LKRG_LOCKDOWN_BY_KERNEL=0 but lock down LKRG via command line

[solardiz]

@kcdev809 Thanks, but we're going to use this PR only as illustration for your suggested functionality and maybe also for how we could check kernel lockdown to affect our own lockdown default. We do not currently intend to merge it. I may make comments on your code, but not for the purpose of you completing this specific PR. Rather, we may revisit this topic separately, and maybe you'll contribute this or something else separately as well.

If you have time now, I'd appreciate your help troubleshooting the unrelated issue you reported (#440). This is also a good way for us to get started working on the project together. Thank you!

[solardiz]

I think __ro_after_init is ineffective for modules, isn't it? We use our own usually-read-only page instead.

[solardiz]

I don't see how you're able to call kallsyms_lookup_name directly, whereas we have to locate it and go via a function pointer? Also, your call via fn is expected to fail on kernels with CET IBT (on supporting Intel CPUs and not in a VM) or KCFI - something we worked around with extra magic for such calls we make in 1.0.

[solardiz]

I just let the CI jobs run here, and we got "17 failing, 1 neutral, 7 successful". This is not to get this PR completed and passing all checks - I think what we maybe actually implement later would be quite different. Rather, it's to confirm/show that a few of the things you do here are not portable enough, to help you with possible other changes.

I think the "1 neutral" is irrelevant to this PR's changes - looks like something changed on GitHub's end (since our previous PR) in how the lack of results from CodeQL is handled.

[kcdev809]

Thank you for the comments on the code. I will update the code with fixes/corrections and some other stuff soon, to use personally, and for reference.

Also, calling kernel lockdown and secure boot ‘edge cases’ is absurd, and not very smart! They’re upstream, maintained, and exist for a reason. Recreating LKRG-only lockdown is weaker, adds complexity, and drifts from mainline Linux. IMO password protection is similarly a dead end: it adds bloat & overhead, and doesn’t meaningfully improve security. Refusing to merge a patch that uses existing kernel mechanisms on principle goes against LKRG’s goal of actually improving security.

[solardiz]

@kcdev809 There may be a misunderstanding here. Let me clarify a few things:

We're not "refusing to merge a patch [...] on principle". Rather, we're debating what exact changes of this sort and when we'd want to merge. We'd like to have a reasonably general solution that would provide a consistent security model and address a certain threat model, and would be easy to describe in documentation, and that we wouldn't make incompatible changes to shortly. Having just released 1.0 and considering certain other changes and cleanups, I am not sure we're ready to add this at this time, especially without agreement on the specifics of what we want here.

This sort of ideas aren't new to LKRG, in fact back when we released version 0.0 in 2018 Adam already had an "experimental" branch that tried to implement some kind of lockdown, or BSD securelevel on steroids as I recall referring to it. We ended up not making this functionality standard in LKRG yet. Now with the kernel standardizing on lockdown, it certainly makes sense for us to see whether we should leverage that. So I do see value in this PR, even if not merged - as feature request and illustration. Thank you very much for this!

Possible password protection would indeed not "improve security" compared to full unconditional lockdown, but it may be a reasonable middle point between no and full lockdown, allowing more systems/sysadmins to use the feature. We should indeed also have a full lockdown option for those who dare.