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@xxxxxxxxxxxx>

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@xxxxxxxx>
SUSE Labs, CR