[PATCH] ext4: check CAP_SYS_ADMIN in ext4_ioctl_get_tune_sb()

[PATCH] ext4: check CAP_SYS_ADMIN in ext4_ioctl_get_tune_sb()

[Guanglei Zhu]

ext4_ioctl_set_tune_sb() requires CAP_SYS_ADMIN before changing the
superblock tunables, but the GET side has no permission check.  Any
user who can open a file on the filesystem read-only can read the
whole tune struct, including the 64-byte mount_opts string that
mount(2) stored in the superblock.

Require CAP_SYS_ADMIN on the GET side as well.

Fixes: 04a91570ac67 ("ext4: implemet new ioctls to set and get superblock parameters")
Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
Verified in a QEMU guest with the ext4 image on a virtio disk: a
process demoted to uid 65534 reads the tune struct of a read-only
opened file unpatched, and gets -EPERM with this patch while root
is unaffected.

 fs/ext4/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index c8387e6a2..d7125a8be 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1284,6 +1284,9 @@ static int ext4_ioctl_get_tune_sb(struct ext4_sb_info *sbi,
 	struct ext4_tune_sb_params ret;
 	struct ext4_super_block *es = sbi->s_es;
 
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
 	memset(&ret, 0, sizeof(ret));
 	ret.set_flags = TUNE_OPS_SUPPORTED;
 	ret.errors_behavior = le16_to_cpu(es->s_errors);
-- 
2.43.0

Re: [PATCH] ext4: check CAP_SYS_ADMIN in ext4_ioctl_get_tune_sb()

[sashiko-bot]

> ext4_ioctl_set_tune_sb() requires CAP_SYS_ADMIN before changing the
> superblock tunables, but the GET side has no permission check.  Any
> user who can open a file on the filesystem read-only can read the
> whole tune struct, including the 64-byte mount_opts string that
> mount(2) stored in the superblock.
> 
> Require CAP_SYS_ADMIN on the GET side as well.
> 
> Fixes: 04a91570ac67 ("ext4: implemet new ioctls to set and get superblock parameters")
> Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012060844.836001-1-zhugl3@xiaopeng.com?part=1

Re: [PATCH] ext4: check CAP_SYS_ADMIN in ext4_ioctl_get_tune_sb()

[Jan Kara]

On Mon 12-10-26 14:08:44, Guanglei Zhu wrote:
> ext4_ioctl_set_tune_sb() requires CAP_SYS_ADMIN before changing the
> superblock tunables, but the GET side has no permission check.  Any
> user who can open a file on the filesystem read-only can read the
> whole tune struct, including the 64-byte mount_opts string that
> mount(2) stored in the superblock.
> 
> Require CAP_SYS_ADMIN on the GET side as well.
> 
> Fixes: 04a91570ac67 ("ext4: implemet new ioctls to set and get superblock parameters")
> Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>

So I suspect not requiring CAP_SYS_ADMIN for reading ext4 sb parameters was
a deliberate design choice (Ted can confirm). You speak about the mount
string and frankly, I don't see what's sensitive about the mount options
that a user shouldn't see them.

								Honza

> ---
> Verified in a QEMU guest with the ext4 image on a virtio disk: a
> process demoted to uid 65534 reads the tune struct of a read-only
> opened file unpatched, and gets -EPERM with this patch while root
> is unaffected.
> 
>  fs/ext4/ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index c8387e6a2..d7125a8be 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1284,6 +1284,9 @@ static int ext4_ioctl_get_tune_sb(struct ext4_sb_info *sbi,
>  	struct ext4_tune_sb_params ret;
>  	struct ext4_super_block *es = sbi->s_es;
>  
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
>  	memset(&ret, 0, sizeof(ret));
>  	ret.set_flags = TUNE_OPS_SUPPORTED;
>  	ret.errors_behavior = le16_to_cpu(es->s_errors);
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: check CAP_SYS_ADMIN in ext4_ioctl_get_tune_sb()

[Guanglei Zhu]

I checked the original series (20250916-tune2fs-v2): neither the
commit message of 04a91570ac67 nor the discussion around it says
anything about a permission policy for the GET side - the only
statement is that the ioctls allow parameters to be changed while
the file system is mounted, without needing write access to the
block device.

You are probably right that reading these parameters is not
sensitive on its own: most of them are already visible through
/proc/mounts or statfs(), and the superblock is readable by anyone
with read access to the block device.  The argument of my patch was
the asymmetry with the SET side rather than an actual leak.

I'm happy to drop this patch if Ted confirms that the missing check
was deliberate.

Re: [PATCH] ext4: check CAP_SYS_ADMIN in ext4_ioctl_get_tune_sb()

[Theodore Tso]

On Tue, Oct 13, 2026 at 10:01:45AM -0500, Guanglei Zhu wrote:
> 
> You are probably right that reading these parameters is not
> sensitive on its own: most of them are already visible through
> /proc/mounts or statfs(), and the superblock is readable by anyone
> with read access to the block device.  The argument of my patch was
> the asymmetry with the SET side rather than an actual leak.
> 
> I'm happy to drop this patch if Ted confirms that the missing check
> was deliberate.

Yes, it's deliberate, precisely because the parameters are visible in
/proc/mounts and in /proc/fs/ext4/<block>/options.  (The latter shows
*all* of the mount parameters currently active, whereas the former
show those mount options which differ from the defaults.)

There should be nothing in the file system parameters which are
secret; things like keying information for fscrypt are not passed in
the mount options, for Very Good Reasons; instead we use the Linux Key
Management facility[1].  As far as I'm concerned, storing passwords in
the mount options ala cifs[2] is an anti-pattern.

[1]  https://man7.org/linux/man-pages/man7/keyrings.7.html
[2]  https://man7.org/linux/man-pages/man8/mount.cifs.8.html

     	   	 	       	      	       - Ted

Re: [PATCH] ext4: check CAP_SYS_ADMIN in ext4_ioctl_get_tune_sb()

[Guanglei Zhu]

Thanks for confirming, and for the pointer to
/proc/fs/ext4/<block>/options - I wasn't aware of that interface.

Agreed, this was not a real leak.  Dropping the patch.