Repository navigation
capi: Check expected_fsverity_digest and fix lcfs_mount_image() - #401
Conversation
libcomposefs is built on its own (`make install-capi`, which the RPM spec's `make install` runs), so it never gets the kernel compat features cfsctl and composefs-setup-root enable by default. Without them, lcfs_mount_image() passes the detached EROFS mount to overlayfs by fd, which kernels before 6.15 reject with EBADF. ostree-prepare-root on CentOS Stream 10 (6.12) fails to mount the deployment's composefs image that way. The C library works on older kernels at runtime; give the Rust one the same features and defaults as the other binaries. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
With object directories, lcfs_mount_fd() mounted the EROFS image and
then passed that mount to composefs_fsmount(), which expects the image
file and mounts it again. The second mount gets a directory as its
source and fails with ENOTBLK, so every composefs mount with a
basedir failed. ostree-prepare-root hits this on boot ("composefs:
failed to mount: Block device required") and drops to the emergency
shell.
Only mount the EROFS image ourselves when there's no overlay on top.
Generated-by: AI
Signed-off-by: Colin Walters <walters@verbum.org>
f1752b5 to
468778e
Compare
| /// An empty string parses to an empty digest, which then never matches. | ||
| /// That's deliberately stricter than C, which treats an empty digest as | ||
| /// no digest and mounts without checking. | ||
| fn parse_hex_digest(hex: &[u8]) -> Option<Vec<u8>> { |
There was a problem hiding this comment.
No don't invent your own hex parser we should use https://docs.rs/hex/latest/hex/fn.decode.html
There was a problem hiding this comment.
Done: parse_hex_digest() is gone and validate_options() uses hex::decode (hex 0.4, as the other crates declare it), mapping any decode error to EINVAL; odd length and + are still rejected, and a short digest still fails with EWRONGVERITY. Squashed into "capi: Check expected_fsverity_digest in lcfs_mount_fd()" (was 23fdafd, now 3338f44).
Changed commits lost your Signed-off-by for now (the rewrite makes the bot the committer); it will be restored with the new sign-off tool. just base_image=...centos-bootc:stream10 test-integration-vm capi --test-threads=1 (4 passed), cargo test -p composefs-capi, clippy -D warnings and fmt pass on a 16-core RHEL 10 devspace.
Generated-by: https://github.com/cgwalters/#llms
| if !options.is_null() { | ||
| let opts = &*options; | ||
| // validate_options() checked that both or neither are set. | ||
| if !opts.upperdir.is_null() { |
There was a problem hiding this comment.
Not new but we should use Option<NonNull> https://doc.rust-lang.org/stable/std/ptr/struct.NonNull.html#method.new
There was a problem hiding this comment.
Done, as a separate prep commit "capi: Use Option for the lcfs_mount_fd() options" (ee7198d): lcfs_mount_fd() converts the pointer once with NonNull::new(options).map(|p| p.as_ref()) and the rest uses if let Some(opts); validate_options() now takes Option<&LcfsMountOptions>. The later commits changed only to fit it: 23fdafd -> 3338f44, 1d177f4 -> 0cd328a, 468778e -> a692d1e (the first two, 70efbf3 and 637bb78, are unchanged). Those lost your Signed-off-by for now; it will be restored with the new sign-off tool.
Generated-by: https://github.com/cgwalters/#llms
468778e to
a692d1e
Compare
| } | ||
|
|
||
| // SAFETY: the caller passes either null or valid mount options. | ||
| let options = NonNull::new(options).map(|p| unsafe { p.as_ref() }); |
There was a problem hiding this comment.
Ah I think the correct thing here is to use Option<NonNull> in the parameter declaration right? Research this
There was a problem hiding this comment.
Yes. I went with Option<&LcfsMountOptions> rather than Option<NonNull<_>>, for both lcfs_mount_fd() and lcfs_mount_image(). From the std::option "Representation" docs:
Rust guarantees to optimize the following types
Tsuch thatOption<T>has the same size, alignment, and function call ABI asT.
The list covers &U, &mut U and ptr::NonNull<U> (for U: Sized), and None is all zeroes, so null. The Nomicon calls this the "nullable pointer optimization" and says "unlike other optimizations it is guaranteed to apply to eligible types."
Option<NonNull<_>> would still need an unsafe as_ref() in the body. Option<&_> doesn't, and it borrows only for the length of the call, which is all we need since the options are only read. The header is hand-written (no cbindgen) and its prototypes don't change.
The change is in 6c55f45, which still has no sign-off. Its subject still says Option<NonNull> because bot-git rework pairs commits by subject, so renaming it would drop your sign-off from the three commits after it. I'll reword it when you do bot-pr signoff if you want. 9df092f ("Check expected_fsverity_digest") also lost its NonNull conversion in lcfs_mount_image() and kept your sign-off, as did the rest, whose content is unchanged. fmt, clippy, cargo test -p composefs-capi and the capi VM integration tests pass on a devspace.
Generated-by: https://github.com/cgwalters/#llms
a692d1e to
668789f
Compare
lcfs_mount_fd() ignored upperdir and workdir, so a caller asking for a writable composefs (ostree does, for a transient root) got a read-only mount without its upper layer. Pass them to the overlayfs mount, and like C make the mount writable unless LCFS_MOUNT_FLAGS_READONLY is set. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
Nothing tested mounting through the C API: the capi CI job runs the C test suite, which doesn't mount anything. So lcfs_mount_image() failed for every image with an object directory, and ignored expected_fsverity_digest, without any test noticing. The test image now also builds our libcomposefs and lcfs-mount-test, a small C program that mounts an image the way ostree-prepare-root does. The tests mount an image from a verity-enabled repository and read back an inline and an external file, with and without the correct digest; check the errors for a wrong digest, an unparseable one and an image without fs-verity; and check that an upper layer is writable, or read-only with LCFS_MOUNT_FLAGS_READONLY. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
668789f to
c8345cb
Compare
alexlarsson
left a comment
alexlarsson
left a comment
There was a problem hiding this comment.
This looks good to me. It needs a sign-off, and @cgwalters requested some changes, but I think they are all done now.
|
@cgwalters-bot you can add my DCO |
|
@cgwalters I can only add your sign-off on an approving review of the current head (c8345cb), not on a comment. Approve the PR and the bot adds the DCO sign-off and pushes. Generated-by: https://github.com/cgwalters/#llms |
|
@cgwalters-bot Can you add it now? |
The options pointer was null-checked and dereferenced by hand at each use. Option<&T> has the same ABI as a nullable pointer (see the "Representation" section of the std::option docs), so declaring the parameter that way lets the compiler do the null check, with no unsafe conversion, and the borrow lasts exactly as long as the call. The C prototypes in lcfs-mount.h are unchanged. Prep for the option handling added by the following commits. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
lcfs_mount_fd() ignored expected_fsverity_digest and mounted whatever image it was given. The C library measures the image's fs-verity digest and refuses to mount it on a mismatch, and callers rely on that: ostree-prepare-root passes the digest of the deployment's composefs image, so with our library its verification silently didn't happen. Check it the way C does: the kernel's sha256 fs-verity measurement of the image must equal the expected digest, failing with EWRONGVERITY (EILSEQ) if it doesn't and ENOVERITY (ENOTTY) if the image has no fs-verity. Also validate the options up front as C does, so an unparseable digest, unknown flags, an upperdir without a workdir (or the reverse) and IDMAP without an fd fail with EINVAL before anything is opened or mounted. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
c8345cb to
5dbc8e1
Compare
|
Done: cgwalters' sign-off was added on his approval and pushed as 5dbc8e1, with no content change. Generated-by: https://github.com/cgwalters/#llms |
3 participants
Security:
lcfs_mount_fd()never readexpected_fsverity_digest. The C library measures the image's fs-verity digest and refuses to mount on a mismatch (lcfs-mount.clcfs_validate_verity_fd). ostree-prepare-root passes that digest for the deployment's composefs image, so on the Rust library its verification silently didn't happen. This PR checks the digest as C does:EWRONGVERITY(EILSEQ) on a mismatch,ENOVERITY(ENOTTY) when the image has no fs-verity, andEINVALfor an unparseable digest. The other options are validated up front, as in C.It also fixes
lcfs_mount_image()/lcfs_mount_fd()with object directories, which failed for every image:pre-6.15compat code, and 6.12 kernels rejected the mount with EBADF. It now enables it by default, like cfsctl and composefs-setup-root.Also:
upperdir/workdirare now passed through (ostree uses them for a transient root), and the mount is writable unlessLCFS_MOUNT_FLAGS_READONLYis set, as in C.New privileged integration tests run
lcfs-mount-test, a small C program built into the test image against our libcomposefs. It mounts an image from a verity-enabled repository the way ostree-prepare-root does and reads back an inline and an external file, with and without the correct digest. The tests also cover wrong, unparseable and+-prefixed digests, an image without verity, and an upper layer that is writable, or read-only withLCFS_MOUNT_FLAGS_READONLY. An empty digest never matches, which is deliberately stricter than C (C skips the check).Tested on a RHEL 10 devspace (6.12):
just test-integration-vm capi --test-threads=1(CentOS Stream 10 VMs): 4/4 pass. The same tests as root on the host pass against the system C libcomposefs too, so the errors match C. On main, all 4 fail.cargo test -p composefs-capi,just test-capi, clippy-D warningsandcargo fmt --checkpass.Not done here, follow-ups:
image_mountdiris still ignored. On pre-6.15 kernels, the temporary EROFS mount goes into atempfiledirectory instead of the caller's path (ostree passes/run/ostree/.private/cfsroot-lower). Fixing that needs acomposefs::mount::MountOptionsfield threaded intoprepare_mount().n_objdirs == 0still mounts the bare EROFS image, where C returns EINVAL..fileredirect bug inconvert.rsis separate. The ostree test in bootc: Test ostree against the Rust libcomposefs cgwalters-forge/composefs-rs#4 still fails on it.Related: #323
Generated-by: https://github.com/cgwalters/#llms