Add whitelist feature - #449
Conversation
|
Hi @paulmenzel a few high-level notes:
The above might be a bit more work than updating the original patch linked in the issue. Although it is doable - just wondering if it will save you some time. Out of curiosity: is there any public reference (mailing list, bugzilla, PR, etc) where the original author submitted the change? If so, please add it as a Thanks o/ |
We could certainly implement all of this and prepare the changes properly for integration into upstream. I'd just like to know beforehand whether the feature would be welcome in principle. It would be a waste to invest time in it if it isn't going to be merged in the end. I was surprised that such an obvious feature doesn't exist, so I'm concerned that there might be fundamental objections to it.
Original author here. This was initially intended for our internal distribution only. Accordingly, there was no public discussion. Here is the original history:
Best
|
I don't think there's a fundamental objection. It makes sense IMO. I was thinking that maybe it would make more sense in the kernel instead of kmod.... but after reading the PRs you linke it seems more a sysadmin conf, so userspace should be ok. |
|
Same here - as mentioned in the issue, I am in favour. One question that immediately comes to mind is should the whitelist be exposed via likmod similar to the original patch or not? Would love to see some arguments why/why not from the authors. |
|
@paulmenzel: I've added commits on top of your PR branch which mangles it into something better integrated into upstream. Can you pull it from add-whitelist-feature and push it here or give me write access to your repository/branch so that I can push to it. I think we should keep the discussion/changes in this PR. The commits are RFC and probably need to be rebased at a later time. Maybe the maintainers here don't want to pollute their history with the diversion over our out-of-tree code evolving into something else and would prefer commits which go directly from their master to the end result. Anyway, we can rebase/rewrite to whatever is wanted later. Also I'm not happy with the logging yet: The specific "not in whitelist" message are probably lost in many environments because of log level LOG_NOTICE and unspecific messages ("could not insert %: EPERM") from modprobe are not really self-explanatory. Upgrading LOG_NOTICE to LOG_ERROR would emit two messages per denied module, which is ugly. I think some basics concepts still need to be discussed, though: In the code change, whitelists are built to match blacklists. But this means, that only toplevel modules are checked, dependencies are not. There is @evelikov , I think I addressed your points in the new commits. I just wasn't sure about "the interaction with soft and weak modules needs to be spelled out". I think hard and soft dependencies are handled the same (maybe wrong, see above) and "weak" dependencies are out of scope. As far as I understand it (but I may by wrong, we don't use that), weak dependency are only used by external tooling, e.g. to decide what goes into a initramfs? When the modules are actually used at a later time, black/whitelists would apply. |
@buczek, awesome that you found time to wok on this.I pushed your changes, and invited you as a collaborator. |
|
AI assistance disclosure: Portions of this patch series (implementation, tests, and documentation) were drafted with AI assistance under my direction and review. All design decisions, verification, and final review were performed by me. |
|
Don't have a strong opinion on the LLM/AI use and in this case there is nothing ground breaking that would warrant licensing and/or other concerns, IMHO. Would be great if @lucas can put together some rough notes in our contributing guide, for posterity sake. Glancing through the patches (proper review will come in a few days), they all seem ok. Some questions come to mind:
I suspect some of these might be covered in the patches already. If so, feel free to ignore ;-) |
I guess you referred to me, even though that's not my @ handle :) "AI is a tool, just like other tools we use. And it's clearly a useful one." - https://lore.kernel.org/linux-media/CAHk-=wi4zC+Ze8e+p3tMv8TtG_80KzsZ1syL9anBtmEh5Z40vg@mail.gmail.com/ |
Yes, the whitelist helpers are direct copies of the blacklist helpers and I've considered deduplication. In the end I decided to avoid changing existing code at this stage. Deduplication can be done with simple and easy to verify separate patches on top of these changes, either in this PR or independently. It's only a minor source code style issue, I guess the compiler would inline the short static functions anyway, thereby removing a "list" argument and using the caller's constants directly. Someone just needs to tell me whether I should do it right away.
We used "lsmod" across all our live machines and created a whitelist for our fleet from that. Then we let it run a few days in "test mode" to see if anything not yet covered is sometimes plugged in dynamically. Then we enabled it. Sysadmins need to be aware of it, because that can be relevant when new hardware is provisioned or kernel is updated. For that reason, a reliable and understandable log is important, see my note about that. In the out-of-tree variant we used syslog() with specific messages but this has been changed now to libkmod standard and I don't even know how other distributions configure their logs.
Yes, of course. Can someone decide, please?
Hmmm. I wouldn't use whitelists in initrd at all. Our usage is kind of a lockdown mode. "no new modules without explicit approval". This is to prevent unprivileged attack code from triggering autoload of kernel modules with vulnerabilities.
The lists don't apply to hard dependencies and I regard weak dependencies as out of scope (see my previous reply). For us, that would definitely be fine. Is somebody requesting a change?
I can only answer for our environment: No, they are not expected to connect their own hardware. Nevertheless most simple hardware like keyboards/mice works with the generic drivers which are already available. If they connect something which would require a new kernel module to be loaded, that would fail and the user would contact our helpdesk for assistance. This is a good thing, because we can review what the user is trying to do and either allow it or otherwise assist in resolving the underlying problem in a correct way. I think a fleet of multi-user systems is uncommon nowadays. But a security-aware self-admin might use "lsmod" on his notebook and generate a whitelist from that with the same intent.
Thanks for your thoughts! |
|
Should we copy the KMOD_PROBE_APPLY_BLACKLIST* API for whitelist? I hesitate, because its somehow strange with KMOD_PROBE_APPLY_BLACKLIST_ALIAS_ONLY defaulted in modprobe.c. |
|
On the soft dependency question, measured on kmod 34.2 in an isolated config dir so it reproduces anywhere: d=$(mktemp -d)
printf 'softdep xfs pre: e1000e\ninstall e1000e /bin/false\n' > "$d/t.conf"
modprobe -C "$d" -n -v xfsFor a module reached through a softdep, the two deny mechanisms behave differently. An The other direction is less obvious. If the requested module itself declares a printf 'install xfs /bin/false\nsoftdep xfs pre: e1000e\n' > "$d/t.conf"
modprobe -C "$d" -n -v xfsThis follows I cannot say much about Since the two directions differ, it would help to state which one |
evelikov
left a comment
evelikov
left a comment
There was a problem hiding this comment.
Was LLM/AI also used to replying to the review questions? They seems overly verbose and somewhat cautions not to make a statement.
Eg.
Yes, the whitelist helpers are direct copies of the blacklist helpers and I've considered deduplication. In the end I decided to avoid changing existing code at this stage. Deduplication can be done with simple and easy to verify separate patches on top of these changes, either in this PR or independently. It's only a minor source code style issue, I guess the compiler would inline the short static functions anyway, thereby removing a "list" argument and using the caller's constants directly.
Someone just needs to tell me whether I should do it right away.
Is usually I've considered it, but decided against for now. Can fixup or send a follow-up PR if people prefer
Let me see if I understood correctly, the usual/recommended workflow is:
- construct a list from lsmod - PR should include a patch to make that process easier
- enable test mode, gather logs/data, analyse and amend whitelist config - love the use of NOTICE (we don't use it yet), but we could use a phrase (like the original patch) to make process easier
- no whitelisting - active or test-mode is present in initrd
- for any issues - driver not loading, need keyboard, etc - sysadmin is involved
Overall I like the idea, although it brings a few questions - not relevant for kmod, but something to consider potentially in your org/setup.
- does the initrd contain full-blown kernel module set - aka people can bypass the whitelist all together, the module isn't listed during test-mode phase.
- in case of keyboard failures (infamous coffee spills), is the recovery process "remove the storage drive" (think soldered ssd/nvme) or there are sufficient backup devices
Wrt weak dependencies - they cannot be out of scope. The way they work is:
- kernel module calls
request_module(foobar), kernel executesmodprobe -q -- foobar - kernel module adds
MODULE_WEAKDEPblurb, to alert userspace - eg. make sure the module is available in initrd/rootfs
So as a whole, I think we don't want/care about the dependency type - hard/soft/weak/etc. Similarly, I don't see the point in adding KMOD_PROBE_APPLY_ - if anything, that would defeat the purpose of the option.
That said, I don't mind being proven wrong 😄
Left a few small inline comments. Let's rebase & squash this up (1-2 patches) and pump the commit message to include, some brief about:
- the why
- how-to-use recommendation
|
|
||
| if (config->whitelist_test_mode) { | ||
| NOTICE(mod->ctx, | ||
| "module '%s' would be denied (test mode active, load permitted)\n", |
There was a problem hiding this comment.
Let's have a clear/unique prefix like the original patch. Eg. whitelist: module %s....
Here and throughout.
|
|
||
| if (!module_whitelist_check(mod)) | ||
| return -EPERM; | ||
|
|
There was a problem hiding this comment.
Curious about the train of thought here - do we want the check prior to any of the others - already_loaded, blacklist - or why not?
There was a problem hiding this comment.
The genesis of this was that we didn't want our kernel to dynamically load modules not explicitly whitelisted by us to reduce attack surface. So the simple logic was that modprobe when used by the kernel to load a module should work like this: "requested module not whitelisted? Reject". Already loaded modules are already loaded at that stage, nothing we can do about that . A module on both, blacklist and whitelist, would not be loaded, which seems to be the better of the two options (less surprising, less change of existing behavior, whitelist required is on top of the existing blacklist feature, this is a security feature so default to the fail-save position to not load a module)
| Adds _modulename_ to the whitelist of modules that are permitted to | ||
| load. This command only has an effect once *whitelist-enable* is | ||
| also given (in this or any other configuration file); by itself, a | ||
| *whitelist* entry is parsed but has no effect. |
There was a problem hiding this comment.
Not strictly related to this PR: Ideally we would have a way to validate the modulename(s) listed in the config. Think typos, people accidentally adding multiple on the same line (space, comma, other-separated), etc.
None of this is a blocker for this feature, but if you feel like sending separate PR that would be really appreciated.
There was a problem hiding this comment.
Well, yes, we had a stricter parsing and error messages and features (dangling comments) in the original code reading /etc/kmod/whitelist.conf. Now the idea was to do proper upstream integration, so we use the existing functions to read modprobe.d files. So the parsing of "whitelist XXX" is now identical to the existing "blacklist XXX".
If this should be approved, I think that should go into a separate PR.
| .config = { | ||
| [TC_UNAME_R] = "3.3.3", | ||
| [TC_ROOTFS] = TESTSUITE_ROOTFS "test-whitelist/deny-all", | ||
| [TC_INIT_MODULE_RETCODES] = "", |
There was a problem hiding this comment.
Do we need the empty TC_INIT_MODULE_RETCODES here?
There was a problem hiding this comment.
I just followed the current code style. See git grep TC_INIT_MODULE_RETCODES.
| [TC_ROOTFS] = TESTSUITE_ROOTFS "test-whitelist/deny-all", | ||
| [TC_INIT_MODULE_RETCODES] = "", | ||
| }, | ||
| .expected_fail = true, |
There was a problem hiding this comment.
Not related to your work: we should get a exit-status field, since expected_fail effectively masks any failure - crash, sanitizers, etc.
| modules are denied. If *whitelist-enable* is given but no | ||
| *whitelist* entries exist, every module is denied. Without | ||
| *whitelist-enable*, no whitelist is enforced regardless of any | ||
| *whitelist* entries present, and all modules may load as usual. |
There was a problem hiding this comment.
Let's shorten this up a bit - my somewhat ADHD brain stopped reading past "configuration file" the first time.
Enables whitelist enforcement, where only modules listed via
*whitelist* commands are permitted to load, and all other
modules are denied.
There was a problem hiding this comment.
Agreed, will shorten that as suggested.
It was not. Natural and limited intelligence and sorry for being too verbose. I used AI as a spell checker, though, because I tend to make a lot of typos, use wrong wording and overlook careless mistakes after editing my text over and over. https://claude.ai/share/7f18f71d-8fdc-43ea-9335-2bdbcf5e46ee
This is just what we did. Not sure if that is the way to go in other environments.
Our initrd doesn't contain any dynamic kernel modules or kmod. All modules required to read the root filesystem are static. So people can't bypass the whitelist by having hardware plugged in during boot. When the system switches to the real rootfs, which provides dynamic kernel modules, the whitelist configuration is there at the same time.
Sorry, I don't get that scenario.
Okay, thanks for the opinion. I need that, because we never managed dependencies manually so I have no experience on my own with all that. We just configure our kernels with a lot of dynamic modules so that we don't need to compile a new kernel or reboot when we want to play with a new feature, then
Thanks! I will look into your comments and rebase. I hope I find time next week. Can't promise, tough. |
|
The builtin logger ( |
I hesitate. One advantage of a separate If you make it tristate, you probably need the same number of lines to remind the admin of the correct setting for test mode.but the required in-line replacement to switch is easier to do wrong: Plus this would clash with kernel modules "enable" or "test" or we'd need to use another keyword... |
Add an optional module-load whitelist, enforced via modprobe.d
directives and parsed by the same libkmod-config.c machinery used
for blacklist, options, softdep, etc. Entries live in the normal
modprobe.d drop-in directories and are merged the same way as every
other directive; there is no separate whitelist config file.
Directives:
whitelist-enable turn on enforcement
whitelist-test-mode audit mode: log what would be denied,
but still permit the load
whitelist <modulename> add an entry (hyphens normalised to
underscores, same as other module names)
Semantics:
- No whitelist-enable anywhere in the merged config: all modules
allowed (opt-in, no behavior change by default)
- whitelist-enable present, no whitelist entries: all modules
denied
- whitelist-enable present with entries: only listed modules
allowed
module_is_whitelisted()/module_whitelist_check() in
libkmod-module.c mirror the existing blacklist handling and are
called from kmod_module_probe_insert_module() after the blacklist
check. Since the kernel always invokes the autoload helper as
"modprobe -q", which silences NOTICE()-level messages, whitelist
denial and test-mode messages are logged via kmod_log() directly so
they stay visible despite -q.
Whitelist state (whitelists list, whitelist_active/
whitelist_test_mode flags) lives in struct kmod_config with the same
per-kmod_ctx lifecycle as blacklist: populated at config load, freed
in kmod_config_free(). Unlike the original out-of-tree
implementation this version has no process-global state, so it is
thread-safe like the rest of libkmod.
Covered by testsuite/test-modprobe.c and documented in
man/modprobe.d.5.scd.
Link: kmod-project#444
9decce7 to
44d2b52
Compare
|
Rebased to upstream/master and squashed into one commit. Changes to
|
Instructions or a script? Where should I put it? |
5 participants
Resolves: #444