lsm: Use walk API with noxdev for recursive SELinux relabeling by jeckersb · Pull Request #2045 · bootc-dev/bootc

jeckersb · 2026-03-04T22:02:24Z

Rewrite ensure_dir_labeled_recurse to use the cap_std_ext walk API
with noxdev instead of manually recursing through directories. This
prevents the recursive labeling from crossing mount point boundaries,
avoiding failures when pseudo-filesystems like sysfs are mounted
under the target root.

Assisted-by: OpenCode (claude-opus-4-6)
Signed-off-by: John Eckersberg jeckersb@redhat.com

gemini-code-assist

Code Review

This pull request refactors ensure_dir_labeled_recurse to use the cap_std_ext walk API with noxdev, which is a good improvement to prevent crossing mount point boundaries during recursive SELinux relabeling. However, a critical security vulnerability has been introduced: the use of ControlFlow::Break(()) prematurely terminates the SELinux relabeling process, leaving the filesystem in an inconsistently labeled state. Additionally, the logic for counting relabeled items no longer includes the root directory, leading to an inaccurate count, and the handling of skipped items incorrectly terminates the entire walk instead of just skipping the specified entry.

gemini-code-assist · 2026-03-04T22:10:07Z

crates/lib/src/lsm.rs

+            if let Some((skip_dev, skip_ino)) = skip {
+                if (metadata.dev(), metadata.ino()) == (skip_dev, skip_ino) {
+                    tracing::debug!("Skipping dev={skip_dev} inode={skip_ino}");
+                    return Ok(ControlFlow::Break(()));


The use of ControlFlow::Break(()) here is a significant security regression. It prematurely terminates the entire directory traversal, leaving subsequent files and directories without correct SELinux labels if a skipped entry is found. This is a regression from the previous implementation that would only skip that single entry. You should use ControlFlow::Continue(()) to skip only the current item and continue the walk. Note that if the skipped entry is a directory, walk() will still descend into it, which might be an unintended side effect if skip can be a directory.

Suggested change

return Ok(ControlFlow::Break(()));

return Ok(ControlFlow::Continue(()));

Hm really we should do both; Break() for skipped directories and Continue() for skipped non-directories.

Wait no I think Gemini is wrong here, it probably just didn't load the docs, but see https://docs.rs/cap-std-ext/latest/cap_std_ext/dirext/trait.CapStdExtDirExt.html#tymethod.walk

Break is documented to be used for exactly this purpose.

This is a problem with the Gemini review, my understand is what happens on the backend is it might clone the git repo, but it doesn't usually proactively try to load content from outside of that, and so it just works off ambient knowledge, and in this particular case I think it was just guessing about semantics of cap-std-ext.

It's for reasons like this that honestly (as good as it is sometimes) I want to try to go to a place where AI reviews are triggered and owned by humans and we've tweaked them to ensure they include relevant context by default and crucially are running in our default devcontainer so they can actually test things and not just guess.

Well I think it's mostly correct though. Usually you'd skip a directory so that should be fine to Break, but if you happen to skip a non-directory using Break is going to skip not just the skipped entry but also its siblings which is not what we want.

We know what we passed in is a directory though. How about a check to that effect + comment here?

In the "big picture" outside of here, yes we only skip directories, but as far as this little bit of API goes we only have the (dev_t, ino64_t) pair so in theory you could skip regular files for some reason. I'm going to push a new version that checks for both and chooses accordingly but if I'm misunderstanding something we can change it again.

On a related note I have some code laying around in a branch or stash somewhere that changes the skip from Option<(libc::dev_t, libc::ino64_t)> to &[(libc::dev_t, libc::ino64_t)] since as we untangle all of this I think if we're going to label the entire physical root we're going to need to skip composefs/objects (after labeling them usr_t prior) and also the ostree deployment root(s), so it's going to need to take multiple things to skip.

Why would we skip composefs/objects? I think the label on them can just be usr_t too.

We do probably do need to skip the ostree deployment roots, but again I think ostree nowadays is already setting labels for those regardless, and THIS code should be doing "only set label if none exists" right?

Ah that's true, I was thinking after we labeled the composefs objects we would need to be careful not to re-label them in the bulk relabel, but yeah this code skips any labeled files, even if they have the "wrong" label per the policy.

crates/lib/src/lsm.rs

jeckersb · 2026-03-04T23:00:57Z

crates/lib/src/lsm.rs

+                        return Ok(ControlFlow::Break(()));
+                    } else {
+                        return Ok(ControlFlow::Continue(()));
+                    }


If this is just wrong let me know but that's how I understand it to work from the docs.

jeckersb · 2026-03-04T23:01:18Z

/gemini review

gemini-code-assist

Code Review

The pull request refactors the ensure_dir_labeled_recurse function to utilize the cap_std_ext::dirext::walk API with the noxdev option. This change correctly prevents recursive SELinux relabeling from traversing across mount point boundaries, improving robustness, especially when dealing with pseudo-filesystems like sysfs. However, this introduces a significant bug in the directory skipping logic where using ControlFlow::Break will cause the entire walk to terminate prematurely, potentially leaving many files unlabeled. Additionally, the new implementation follows symlinks at the top level, which is a change in behavior from the previous manual recursion and could lead to unintended relabeling.

crates/lib/src/lsm.rs

jeckersb · 2026-03-04T23:59:03Z

error: Installing to filesystem: Creating ostree deployment: Recursive SELinux relabeling of boot: Unexpected SELinuxLabelState::Unsupported during walk at boot/efi

🤨

jeckersb · 2026-03-05T14:18:25Z

error: Installing to filesystem: Creating ostree deployment: Recursive SELinux relabeling of boot: Unexpected SELinuxLabelState::Unsupported during walk at boot/efi

🤨

I think this is because using noxdev with the walk will prevent the traversal into /boot/efi, but we still try to label it as part of iterating through the entries of /boot.

cgwalters · 2026-03-05T14:32:58Z

Hmm yes interesting I would offhand say this is a bug in .noxdev() in walk it should also skip the mountpoint dir entirely.
But we could also work around it here by replicating the openat-noxdev logic for dirs here...

jeckersb · 2026-03-05T14:41:17Z

My current plan is... if we get Unsupported, check whether it's a mounpoint, and if so skip it.

One minor problem though is if we encounter a mountpoint for a filesystem that does support labels, we will label the root of that filesystem but not traverse any further. To avoid that we would need to check every directory we encounter to see if it's a mountpoint before labeling, which seems rather expensive.

jeckersb · 2026-03-05T14:54:27Z

As an aside it's mildly annoying that this seems to pass in all of our tests other than Test install since that one is more bespoke to the CI runner and is not easily reproducible locally with just like everything else.

jeckersb · 2026-03-05T14:57:01Z

My current plan is... if we get Unsupported, check whether it's a mounpoint, and if so skip it.

Added this for now just to see if it passes or fails in some other way

cgwalters · 2026-03-05T16:11:07Z

As an aside it's mildly annoying that this seems to pass in all of our tests other than Test install since that one is more bespoke to the CI runner and is not easily reproducible locally with just like everything else.

Yeah 🙁 - I think we can try hard cutting those over to a tmt flow too.

That said we can optimize this a bit: the install tests don't actually need a full bcvk libvirt they just need bcvk ephemeral which would be cheaper.

jeckersb · 2026-03-05T17:03:40Z

Ok updated with temporary cargo patch to point at coreos/cap-std-ext#88, so now we call .skip_mountpoints() and remove the logic here that previously checked for mountpoints.

jeckersb · 2026-03-05T17:06:20Z

Also updated the docstring on ensure_dir_labeled_recurse to mention skip_mountpoints.

jeckersb · 2026-03-05T19:44:27Z

Ok this looks pretty good now to me, the only failure was a network flake:

content: error: Installing to disk: Creating ostree deployment: Preparing import: Fetching manifest: failed to invoke method GetManifest: reading manifest sha256:12b0be0890a35baac8bdec7157918cf2dc6aa7ee28cc64ffd2e0cb682250d789 in quay.io/centos-bootc/centos-bootc: received unexpected HTTP status: 500 Internal Server Error

…relabeling Rewrite ensure_dir_labeled_recurse to use the cap_std_ext walk API with noxdev and skip_mountpoints instead of manually recursing through directories. This prevents the recursive labeling from crossing mount point boundaries, avoiding failures when pseudo-filesystems like sysfs are mounted under the target root. Assisted-by: OpenCode (claude-opus-4-6) Signed-off-by: John Eckersberg <jeckersb@redhat.com>

jeckersb · 2026-03-06T16:09:53Z

Updated to use the 5.1.1 release of cap-std-ext, assuming CI doesn't blow up this should be good now.

cgwalters · 2026-03-06T16:14:56Z

crates/lib/src/lsm.rs

+        .noxdev()
+        .skip_mountpoints()


This is obviously fine but don't we only need the skip_mountpoints? Conceptually it's a more restrictive noxdev I believe

Yes that's true, it doesn't hurt anything like that but I also don't see how you could hit the noxdev case because you would hit the mountpoint case first.

bootc-bot bot requested a review from gursewak1997 March 4, 2026 22:02

cgwalters previously approved these changes Mar 4, 2026

View reviewed changes

cgwalters enabled auto-merge (rebase) March 4, 2026 22:05

gemini-code-assist bot reviewed Mar 4, 2026

View reviewed changes

jeckersb disabled auto-merge March 4, 2026 22:28

jeckersb dismissed cgwalters’s stale review via d9a6064 March 4, 2026 22:57

jeckersb force-pushed the label-noxdev branch from 176bffb to d9a6064 Compare March 4, 2026 22:57

jeckersb commented Mar 4, 2026

View reviewed changes

gemini-code-assist bot reviewed Mar 4, 2026

View reviewed changes

crates/lib/src/lsm.rs Show resolved Hide resolved

crates/lib/src/lsm.rs Show resolved Hide resolved

cgwalters marked this pull request as draft March 5, 2026 12:30

jeckersb force-pushed the label-noxdev branch from d9a6064 to 5d423b6 Compare March 5, 2026 14:56

jeckersb mentioned this pull request Mar 5, 2026

Walk with noxdev will visit (but not traverse) mountpoints coreos/cap-std-ext#87

Closed

jeckersb mentioned this pull request Mar 5, 2026

Add WalkConfiguration::skip_mountpoints() coreos/cap-std-ext#88

Merged

jeckersb force-pushed the label-noxdev branch 2 times, most recently from 355684a to 423fe26 Compare March 5, 2026 17:02

jeckersb force-pushed the label-noxdev branch from 423fe26 to faa7f21 Compare March 5, 2026 17:05

jeckersb mentioned this pull request Mar 5, 2026

Release 5.1.0 coreos/cap-std-ext#89

Merged

jeckersb force-pushed the label-noxdev branch from faa7f21 to c76e936 Compare March 6, 2026 16:08

jeckersb marked this pull request as ready for review March 6, 2026 16:10

cgwalters approved these changes Mar 6, 2026

View reviewed changes

cgwalters enabled auto-merge (rebase) March 6, 2026 16:16

jeckersb mentioned this pull request Mar 6, 2026

Enable enforcing SELinux for sealed images #2035

Merged

cgwalters merged commit b5173fe into bootc-dev:main Mar 6, 2026
47 checks passed

	return Ok(ControlFlow::Break(()));
	return Ok(ControlFlow::Continue(()));

Conversation

jeckersb commented Mar 4, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 4, 2026

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeckersb commented Mar 4, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

jeckersb commented Mar 4, 2026

Uh oh!

jeckersb commented Mar 5, 2026

Uh oh!

cgwalters commented Mar 5, 2026

Uh oh!

jeckersb commented Mar 5, 2026

Uh oh!

jeckersb commented Mar 5, 2026

Uh oh!

jeckersb commented Mar 5, 2026

Uh oh!

cgwalters commented Mar 5, 2026

Uh oh!

jeckersb commented Mar 5, 2026

Uh oh!

jeckersb commented Mar 5, 2026

Uh oh!

jeckersb commented Mar 5, 2026

Uh oh!

jeckersb commented Mar 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants