Re: [PATCH v3 1/3] tools/mount-flatpak: init

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 4408 bytes --]

Demi Marie Obenour <demiobenour@gmail.com> writes:

> On 11/29/25 14:00, Alyssa Ross wrote:
>> Demi Marie Obenour <demiobenour@gmail.com> writes:
>> 
>>> On 11/27/25 15:23, Alyssa Ross wrote:
>>>> +fn run(mut args: ArgsOs) -> Result<(), String> {
>>>> +    let Some(user_data_path) = args.next().map(PathBuf::from) else {
>>>> +        ex_usage();
>>>> +    };
>>>
>>> This should be an absolute path, as the working directory isn't one
>>> that the user should be caring about.
>> 
>> I don't see any reason to restrict the user like that.
>
> The advantage is better error messages.  A relative path will result
> in a confusing "does not exist" error at best.
>
> Definitely should not block committing this.

From what I can tell mount-flatpak works fine with relative paths — are
you talking about the cd in run-flatpak?  I don't think it makes sense
for one program to reject valid inputs that might be invalid to a higher
layer that runs it, but we could consider moving the cd into
mount-flatpak by adding yet another argument for the destination
directory.

>>>> +    let target_installation_dir = open_tree(
>>>> +        CWD,
>>>> +        "flatpak",
>>>> +        OpenTreeFlags::OPEN_TREE_CLONE
>>>> +            | OpenTreeFlags::OPEN_TREE_CLOEXEC
>>>> +            | OpenTreeFlags::AT_RECURSIVE,
>>>
>>> Missing AT_SYMLINK_NOFOLLOW unless it is is implied.
>> 
>> We rely on nothing malicious being able to mess with the target
>> directory while we're running, and we do not create flatpak as a
>> symlink.
>
> Still good practice anyway, but not a blocker.

Will add.

>>>> +    let config_source = open_tree(
>>>> +        CWD,
>>>> +        config_source_path,
>>>> +        OpenTreeFlags::OPEN_TREE_CLONE | OpenTreeFlags::OPEN_TREE_CLOEXEC,
>>>> +    )
>>>> +    .map_err(|e| format!("opening {config_source_path}: {e}"))?;
>>>> +    move_mount(
>>>> +        config_source,
>>>> +        "",
>>>> +        config_target,
>>>> +        "",
>>>> +        MoveMountFlags::MOVE_MOUNT_F_EMPTY_PATH | MoveMountFlags::MOVE_MOUNT_T_EMPTY_PATH,
>>>> +    )
>>>> +    .map_err(|e| format!("mounting config: {e}"))?;
>>>> +
>>>> +    let mut attr = libc::mount_attr {
>>>> +        attr_clr: libc::MOUNT_ATTR_NOSYMFOLLOW,
>>>> +        attr_set: libc::MOUNT_ATTR_RDONLY | libc::MOUNT_ATTR_NODEV,
>>>> +        propagation: 0,
>>>> +        userns_fd: 0,
>>>> +    };
>>>
>>> Propagation should likely be set to MS_SLAVE.  (Yeah, I know,
>>> terrible name.)  attr_set should also include MOUNT_ATTR_NOEXEC as
>>> well as the host has no business executing anything there.
>> 
>> The host has no business executing anything at all from the whole user
>> data partition, so this is not the place to set that.  There's no reason
>> this mount _in particular_ needs the flag set that doesn't apply to the
>> whole partition this mount's flags are inherited from.
>
> Good point, but I still think the propagation needs to be set to
> MS_SLAVE.  Right now, mount flags are set manually by the user.

Will do.

>>>> diff --git a/tools/mount-flatpak/src/metadata.rs b/tools/mount-flatpak/src/metadata.rs
>>>> new file mode 100644
>>>> index 0000000..dfc05b1
>>>> --- /dev/null
>>>> +++ b/tools/mount-flatpak/src/metadata.rs
>>>> @@ -0,0 +1,19 @@
>>>> +// SPDX-License-Identifier: EUPL-1.2+
>>>> +// SPDX-FileCopyrightText: 2025 Alyssa Ross <hi@alyssa.is>
>>>> +
>>>> +use std::fs::File;
>>>> +use std::io::read_to_string;
>>>> +
>>>> +use crate::keyfile::parse;
>>>> +
>>>> +pub fn extract_runtime(mut metadata: File) -> Result<String, String> {
>>>> +    let metadata = read_to_string(&mut metadata).map_err(|e| e.to_string())?;
>>>
>>> Missing limit on the size of the file to prevent unbounded memory allocation.
>> 
>> There are lots of ways to cause unbounded host memory allocation.
>> Modifying everything to limit size is not feasible.  The only way to
>> prevent this is to run this stuff in a VM-scoped cgroup.
> It's easy to run the VM's services in a cgroup, but running commands
> like this in the cgroup is harder.  Also, this command is much easier
> to make robust against problems like that.
>
> Should not block commit though.

I don't see why it needs to be.  The cgroup will have to be created by
run-flatpak, so why wouldn't it be able to run mount-flatpak in that
cgroup?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]