linux-kernel - Re: [PATCH] debugfs: fix mount options not being applied

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025-08-14.1755150554-popular-erased-gallons-heroism-gRtAbX@cyphar.com>
Date: Thu, 14 Aug 2025 19:05:55 +1000
From: Aleksa Sarai <cyphar@...har.com>
To: Eric Sandeen <sandeen@...hat.com>
Cc: Charalampos Mitrodimas <charmitro@...teo.net>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Danilo Krummrich <dakr@...nel.org>, Christian Brauner <brauner@...nel.org>, 
	David Howells <dhowells@...hat.com>, linux-kernel@...r.kernel.org, 
	linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] debugfs: fix mount options not being applied

On 2025-08-05, Eric Sandeen <sandeen@...hat.com> wrote:
> On 8/4/25 12:22 PM, Eric Sandeen wrote:
> > On 8/4/25 9:30 AM, Charalampos Mitrodimas wrote:
> >> Mount options (uid, gid, mode) are silently ignored when debugfs is
> >> mounted. This is a regression introduced during the conversion to the
> >> new mount API.
> >>
> >> When the mount API conversion was done, the line that sets
> >> sb->s_fs_info to the parsed options was removed. This causes
> >> debugfs_apply_options() to operate on a NULL pointer.
> >>
> >> As an example, with the bug the "mode" mount option is ignored:
> >>
> >>   $ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test
> >>   $ mount | grep debugfs_test
> >>   debugfs on /tmp/debugfs_test type debugfs (rw,relatime)
> >>   $ ls -ld /tmp/debugfs_test
> >>   drwx------ 25 root root 0 Aug  4 14:16 /tmp/debugfs_test
> > 
> > Argh. So, this looks a lot like the issue that got fixed for tracefs in:
> > 
> > e4d32142d1de tracing: Fix tracefs mount options
> > 
> > Let me look at this; tracefs & debugfs are quite similar, so perhaps
> > keeping the fix consistent would make sense as well but I'll dig
> > into it a bit more.
> 
> So, yes - a fix following the pattern of e4d32142d1de does seem to resolve
> this issue.
> 
> However, I think we might be playing whack-a-mole here (fixing one fs at a time,
> when the problem is systemic) among filesystems that use get_tree_single()
> and have configurable options. For example, pstore:
> 
> # umount /sys/fs/pstore 
> 
> # mount -t pstore -o kmsg_bytes=65536 none /sys/fs/pstore
> # mount | grep pstore
> none on /sys/fs/pstore type pstore (rw,relatime,seclabel)
> 
> # mount -o remount,kmsg_bytes=65536 /sys/fs/pstore
> # mount | grep pstore
> none on /sys/fs/pstore type pstore (rw,relatime,seclabel,kmsg_bytes=65536)
> #

Isn't this just a standard consequence of the classic "ignore mount
flags if we are reusing a superblock" behaviour? Not doing this can lead
to us silently clearing security-related flags ("acl" is the common
example used) and was the main reason for FSCONFIG_CMD_CREATE_EXCL.

Maybe for some filesystems (like debugfs), it makes sense to permit a
mount operation to silently reconfigure existing mounts, but this should
be an opt-in knob per-filesystem.

Also, if we plan to do this then you almost certainly want to have
fs_context track which set of parameters were set and then only
reconfigure those parameters *which were set*. At the moment,
fs_context_for_reconfigure() works around this by having the current
sb_flags and other configuration be loaded via init_fs_context(), but if
you do an auto-reconfigure with an fs_context created for mounting then
you won't inherit _any_ of the old mount options. This could lead to a
situation like:

  % mount -t pstore -o ro /sys/fs/pstore
  % mount -t pstore -o kmsg_bytes=65536 /tmp
  % # /sys/fs/pstore is now rw.

Which is really not ideal, as it would make it incredibly fragile for
anyone to try to mount these filesystems without breaking other mounts
on the system.

If fs_context tracked which parameters were configured and only applied
the set ones, at least you would avoid unintentionally unsetting
parameters of the original mount.

FWIW, cgroupv1 has a warning when this situation happens (see the
pr_warn() in cgroup1_root_to_use()). I always wondered why this wasn't
done on the VFS level, as a warning is probably enough to alert admins
about this behaviour without resorting to implicitly changing the mount
options of existing mounts.

> I think gadgetfs most likely has the same problem but I'm not yet sure
> how to test that.
> 
> I have no real objection to merging your patch, though I like the
> consistency of following e4d32142d1de a bit more. But I think we should
> find a graceful solution so that any filesystem using get_tree_single
> can avoid this pitfall, if possible.
> 
> -Eric
> 
> 

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/

Download attachment "signature.asc" of type "application/pgp-signature" (266 bytes)