imp: add bpf device containment by garlick · Pull Request #228 · flux-framework/flux-security

garlick · 2026-04-22T17:34:42Z

Problem: systemd running as the flux user cannot enforce DeviceAllow / DevicePolicy because it lacks appropriate privilege. The IMP does have that privilege so we can implement it there.

See the included design spec for design decisions made while building this.

I'm posting this early for quick feedback on the design doc. The code at this point is first draft, completely untested.

Fixes #226

codecov · 2026-04-22T17:37:08Z

Codecov Report

❌ Patch coverage is 66.02564% with 106 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.75%. Comparing base (485fe2d) to head (c211a90).
⚠️ Report is 14 commits behind head on master.

Files with missing lines	Patch %	Lines
src/imp/cgroup_device.c	8.53%	75 Missing ⚠️
src/imp/exec/device.c	88.42%	25 Missing ⚠️
src/imp/exec/exec.c	57.14%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
- Coverage   84.78%   83.75%   -1.04%     
==========================================
  Files          38       40       +2     
  Lines        5154     5466     +312     
==========================================
+ Hits         4370     4578     +208     
- Misses        784      888     +104

Files with missing lines	Coverage Δ
src/imp/exec/exec.c	`76.75% <57.14%> (-1.61%)`	⬇️
src/imp/exec/device.c	`88.42% <88.42%> (ø)`
src/imp/cgroup_device.c	`8.53% <8.53%> (ø)`

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

grondo

Wow! It is really nice to have some clear design docs up front!
I've made a few comments inline.

grondo · 2026-04-23T01:14:59Z

+numeric form.  If ``DeviceAllow`` is present, access is restricted to the
+listed devices; if absent, all devices are accessible.  For example:


Might want to call out here that baseline devices (i.e. those that would be included in the systemd DevicePolicy=closed policy) should be included in the DeviceAllow input to the IMP. I guess the imp helper will need to hard code these.

Yeah that was the plan (easier to add/remove things there!) I'll add a note.

grondo · 2026-04-23T01:15:03Z

+        { "type": "c", "major": 195, "minor": 0, "access": "rw" },
+        { "type": "c", "major": 195, "minor": -1, "access": "rw" }
+      ]
+    }


RFC 15 specifies that the only allowed keys in the input to the IMP are J and options:
https://flux-framework.readthedocs.io/projects/flux-rfc/en/latest/spec_15.html#input-to-the-imp
and the Device Containment specification needs an update to indicate the new DeviceAllow structure.

Oh, duh, maybe this should be under options.

grondo · 2026-04-23T01:22:19Z

+**Macro parameter naming.**  The BPF instruction-building macros avoid
+parameter names ``off`` and ``imm`` because the C preprocessor
+substitutes macro parameters before the compiler sees struct field
+names.  A parameter named ``off`` would be expanded inside ``.off =
+(off)`` designators, producing invalid syntax.  The parameters are


This one needs more explanation I think. What are poff and pimm? Is this detail really necessary in this doc?

This quirk probably belongs in an inline comment, if at all.

grondo · 2026-04-23T01:23:43Z

+Each allow entry emits 10 instructions without a minor check, or 11
+with one; the jump offsets within each entry are computed accordingly.


If there's a reason the number of instructions needs to be explicit it would be interesting to know that here.

Naa, useless detail, will drop.

garlick · 2026-04-26T16:57:09Z

This is probably ready for another look. It now reads DevicePolicy and DeviceAllow from the options dict in more or less the same form as the systemd properties of the same name. In addition to the sharness tests, I've poked at it a bit on my test system and it seems to do what's advertised.

grondo

Excellent! This LGTM! A few comments inline. We should probably get the testenv images updated so we don't need a future PR to revert the manual dependency install.

grondo · 2026-04-26T17:48:00Z

+     bionic*|focal*|jammy*|noble*|bookworm*) \
+         apt-get update && apt-get install -y linux-libc-dev ;; \
+     el*) yum install -y kernel-headers ;; \
+     fedora*) dnf install -y libubsan kernel-headers ;; \
+     alpine*) apk add --no-cache linux-headers ;; \
+     *) echo "Unknown BASE_IMAGE=$BASE_IMAGE" >&2 && exit 1 ;; \


We should just go ahead and get the testenv docker containers updated over in flux-core (just a simple PR), then when that's merged this commit can be dropped.

grondo · 2026-04-26T18:04:37Z

+    if (count < 0 || count > INT_MAX) {
+        errno = ERANGE;
+        return -1;
+    }


I might be more paranoid in the count check in decode here and use a more reasonable number than INT_MAX. Without this an attacker could set device.count to a large enough number to cause DoS by having the privileged process allocate a huge amount of memory.

grondo · 2026-04-26T18:15:56Z

+    if (sscanf (s, "%c:%d:%d:%3s", &type, &major, &minor, access) != 4
+        || (type != 'c' && type != 'b')) {
+        errno = EINVAL;
+        return -1;
+    }


type is validated here, should this call access_is_valid (access) as well?

Problem: linux/bpf.h is now required for device containment, but configure doesn't check for it, so if it is missing, compilation fails. Add an AC_CHECK_HEADER check so configure fails fast if not found.

Problem: man page link for bpf(2) is broken. Core uses man7.org, which has this page, while we use linux.die.net, which doesn't. Just switch to man7.org.

Problem: there is no documentation describing how BPF device containment works in the IMP for developers. Add a device-containment.rst page to the internals guide covering the data flow from helper JSON through privsep to BPF attachment, the BPF program structure, and key design decisions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Problem: the new guide docs are not spell checked. Update spell check glob to include guide/*.

Problem: trivial macros like streq() and ARRAY_SIZE() that we use in flux-core are useful for code clarity, but flux-security doesn't have them. Rather than vendor, just rewrite the following from scratch and add to macros.h: streq(), strstarts(), ARRAY_SIZE(), ERRNO_SAFE_WRAP().

Problem: the IMP has no way to receive DeviceAllow and DevicePolicy values from the FLUX_IMP_EXEC_HELPER for use in device containment. Add src/imp/exec/device.h and device.c with struct device_allow and struct device_allow_entry representing a resolved device policy, along with device_allow_encode() and device_allow_decode() to transfer the data across the privsep boundary via the kv struct. Each entry holds a resolved type, major, minor (-1 for wildcard), and access string. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Problem: device containment data has no path from the unprivileged child to the privileged parent. Add struct device_allow *da to struct imp_exec. In imp_exec_put_kv(), encode it into the kv if present. In imp_exec_init_kv(), decode it from the kv, treating absence as no containment. Free it in imp_exec_destroy(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Problem: the IMP has no way to read device containment policy from the FLUX_IMP_EXEC_HELPER JSON output. Add device_allow_from_options() to parse DevicePolicy and DeviceAllow from input options dict to a struct device_allow. Call it from imp_exec_init_stream(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Problem: there is no code to enforce device policy on the job cgroup. Add cgroup_device_apply() which builds a BPF cgroup device filter program from a struct device_allow list, loads it into the kernel via bpf(2), and attaches it to the job cgroup fd. The BPF program allows device accesses matching any entry in the list and denies all others. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Problem: device containment policy received from the helper is never applied to the job cgroup. Call cgroup_device_apply() with the job cgroup and decoded device policy after PAM setup and before fork, so the BPF program is in place before any job processes are created. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Problem: No integration test exercises the device containment code path through the full IMP exec flow. Add one. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Problem: the code for creating the device_allow object and passing it to the privileged portion section of the IMP has no unit tests. Add some. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

grondo

Re-approving since previous approval was cleared.

garlick · 2026-04-27T22:49:43Z

Thanks! setting mwp

mergify · 2026-04-27T22:50:29Z

garlick force-pushed the device_contain branch 4 times, most recently from 60903e3 to 0675382 Compare April 23, 2026 01:00

grondo reviewed Apr 23, 2026

View reviewed changes

garlick force-pushed the device_contain branch 2 times, most recently from 8a62a35 to 234af77 Compare April 23, 2026 03:36

garlick mentioned this pull request Apr 24, 2026

add device containment support to IMP exec helper flux-framework/flux-core#7568

Merged

garlick force-pushed the device_contain branch 8 times, most recently from 69157d5 to 23dde97 Compare April 26, 2026 16:44

garlick changed the title ~~WIP: imp: add bpf device containment~~ imp: add bpf device containment Apr 26, 2026

grondo previously approved these changes Apr 26, 2026

View reviewed changes

garlick and others added 10 commits April 26, 2026 11:47

configure: require linux/bpf.h

2e61e57

Problem: linux/bpf.h is now required for device containment, but configure doesn't check for it, so if it is missing, compilation fails. Add an AC_CHECK_HEADER check so configure fails fast if not found.

doc: fix linux man page URLs to use man7.org

69b4c96

Problem: man page link for bpf(2) is broken. Core uses man7.org, which has this page, while we use linux.die.net, which doesn't. Just switch to man7.org.

doc: add guide/*.rst to spellcheck

f47d3c7

Problem: the new guide docs are not spell checked. Update spell check glob to include guide/*.

garlick and others added 2 commits April 26, 2026 12:39

testsuite: add device containment sharness tests

6d056b7

Problem: No integration test exercises the device containment code path through the full IMP exec flow. Add one. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

testsuite: add device_allow unit tests

c211a90

Problem: the code for creating the device_allow object and passing it to the privileged portion section of the IMP has no unit tests. Add some. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

garlick dismissed grondo’s stale review via c211a90 April 27, 2026 01:28

garlick force-pushed the device_contain branch from 23dde97 to c211a90 Compare April 27, 2026 01:28

grondo approved these changes Apr 27, 2026

View reviewed changes

garlick added the merge-when-passing label Apr 27, 2026

mergify Bot added the queued label Apr 27, 2026

mergify Bot merged commit 3b9c239 into flux-framework:master Apr 27, 2026
55 of 60 checks passed

mergify Bot removed the queued label Apr 27, 2026

garlick deleted the device_contain branch April 27, 2026 22:58

		numeric form. If ``DeviceAllow`` is present, access is restricted to the
		listed devices; if absent, all devices are accessible. For example:

		Each allow entry emits 10 instructions without a minor check, or 11
		with one; the jump offsets within each entry are computed accordingly.

Uh oh!

Conversation

garlick commented Apr 22, 2026

Uh oh!

codecov Bot commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

grondo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garlick commented Apr 26, 2026

Uh oh!

grondo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grondo left a comment

Choose a reason for hiding this comment

Uh oh!

garlick commented Apr 27, 2026

Uh oh!

mergify Bot commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Apr 22, 2026 •

edited

Loading

mergify Bot commented Apr 27, 2026 •

edited

Loading