Skip to content

Comments

Fuse mount: make auto_unmount compatible with suid/dev mount options#762

Merged
Nikratio merged 10 commits intolibfuse:masterfrom
matthiasgoergens:matthias/fix-dev-suid-2
Apr 12, 2023
Merged

Fuse mount: make auto_unmount compatible with suid/dev mount options#762
Nikratio merged 10 commits intolibfuse:masterfrom
matthiasgoergens:matthias/fix-dev-suid-2

Conversation

@matthiasgoergens
Copy link
Contributor

When you run as root, fuse normally does not call fusermount but uses
the mount system call directly. When you specify auto_unmount, it goes
through fusermount instead. However, fusermount is a setuid binary that
is normally called by regular users, so it cannot in general accept suid
or dev options.

In this patch, we split up how fuse mounts as root when auto_unmount is specified.

First, we mount using system calls directly, then we reach out to fusermount to set up auto_unmount only (with no actual mounting done in fusermount).

Fixes #148

@matthiasgoergens
Copy link
Contributor Author

Btw, I'm a bit worried about race conditions for auto_unmount in general. After all, auto_unmount identifies the mountpoint by path only (both in my new usage, and in established usage). Couldn't something happen in between check and unmount?

@bsbernd
Copy link
Collaborator

bsbernd commented Apr 4, 2023

Yeah, I had scanned through your patches and the code and also seen the race.

@Nikratio
Copy link
Contributor

Nikratio commented Apr 4, 2023

Thanks for the patch. I am generally not a big fan of auto_unmount, and I don't think I would add this feature today (but it existing we're unfortunately stuck with it).

I've just pushed a commit that explains the drawbacks in more detail: #763

@matthiasgoergens matthiasgoergens force-pushed the matthias/fix-dev-suid-2 branch from b035cc2 to cce9598 Compare April 6, 2023 04:01
@Nikratio
Copy link
Contributor

Nikratio commented Apr 6, 2023

Could you also add a unit tests that checks if this works?

@matthiasgoergens
Copy link
Contributor Author

Yes, working on it.

I discovered a few minor issues when looking at the tests. You've already seen the PRs.

@matthiasgoergens
Copy link
Contributor Author

@Nikratio I added a test.

@Nikratio
Copy link
Contributor

Looks like the test discovered some memory leaks.

@matthiasgoergens
Copy link
Contributor Author

Yes, I'll check that. Perhaps it's one I just introduced, even.

@matthiasgoergens
Copy link
Contributor Author

@Nikratio I found the memory leak and fixed it. Weirdly enough, it's in fuse_opt_parse which should have already been suppressed.

More detail: when fuse_opt_parse handles

	{ "source=%s",
	  offsetof(struct lo_data, source), 0 },

in passthrough_ll.c it allocates a brand-new string for us. And that's the one we have to free properly or get scolded for leaking.


} else {
lo.source = "/";
lo.source = strdup("/");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The true-branch of this if gives us a malloc-ed string. So for symmetry, we make this one a malloced on, too. So we can free without a care further down.

> When you run as root, fuse normally does not call fusermount but uses
> the mount system call directly. When you specify auto_unmount, it goes
> through fusermount instead. However, fusermount is a setuid binary that
> is normally called by regular users, so it cannot in general accept suid
> or dev options.

In this patch, we split up how fuse mounts as root when `auto_unmount`
is specified.

First, we mount using system calls directly, then we reach out to
fusermount to set up auto_unmount only (with no actual mounting done in
fusermount).

Fixes libfuse#148
@Nikratio Nikratio force-pushed the matthias/fix-dev-suid-2 branch from 50bdd1d to d0d85c7 Compare April 11, 2023 11:29
@matthiasgoergens
Copy link
Contributor Author

@Nikratio Does this one need more changes?

Split test into root vs non-root, so that the output of pytest
makes clear what functionality is being tested.
@Nikratio Nikratio merged commit 7297044 into libfuse:master Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FUSE mount ignores suid/dev mount options when auto_unmount is specified

3 participants