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

Alyssa Ross
2025-11-30T17:25:18Z

[-- 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 --]