linux-kernel - Re: [PATCH v3 9/9] fs/ntfs3: Show uid/gid always in show

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: <20210829103600.pfwtm5c3sdpcnzlm@pali>
Date:   Sun, 29 Aug 2021 12:36:00 +0200
From:   Pali Rohár <pali@...nel.org>
To:     Kari Argillander <kari.argillander@...il.com>
Cc:     Konstantin Komarov <almaz.alexandrovich@...agon-software.com>,
        ntfs3@...ts.linux.dev, Christoph Hellwig <hch@....de>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        Matthew Wilcox <willy@...radead.org>,
        Christian Brauner <christian.brauner@...ntu.com>
Subject: Re: [PATCH v3 9/9] fs/ntfs3: Show uid/gid always in show_options()

On Sunday 29 August 2021 12:56:14 Kari Argillander wrote:
> Show options should show option according documentation when some value
> is not default or when ever coder wants. Uid/gid are problematic because
> it is hard to know which are defaults. In file system there is many
> different implementation for this problem.
> 
> Some file systems show uid/gid when they are different than root, some
> when user has set them and some show them always. There is also problem
> that what if root uid/gid change. This code just choose to show them
> always. This way we do not need to think this any more.

Hello! IIRC ntfs disk storage supports POSIX permissions and uid/gid for
files and directories. But from ntfs3 documentation it is not clear if
this ntfs3 implementation supports it or not.

Currently ntfs3 documentation says:
> https://github.com/Paragon-Software-Group/linux-ntfs3/blob/master/Documentation/filesystems/ntfs3.rst
> uid= gid= umask= Controls the default permissions for
> files/directories created after the NTFS volume is mounted.
(and looks that rst formatting is broken)

And from this description I'm not really sure what these option
controls.

For example udf filesystem also supports storing POSIX uid/gid and
supports also additional extension per file to "do not store uid" and
"do not store gid". Moreover kernel implementation has mount option
(uid= and gid=) which overrides disk's uid/gid to mount option value.
And also has mount option to allow storing new files "without uid/gid".
So there does not have to be any default for uid=/gid= like it is for
"native" POSIX filesystems (e.g. ext4).

And so interpretation of uid/gid options is not always easy; specially
if filesystem has extensions to POSIX permissions. And NTFS has it too
as it by default has in its storage (only?) NT permissions and NT SIDs.

> Signed-off-by: Kari Argillander <kari.argillander@...il.com>
> ---
>  fs/ntfs3/ntfs_fs.h | 23 ++++++++++-------------
>  fs/ntfs3/super.c   | 12 ++++--------
>  2 files changed, 14 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
> index 5df55bc733bd..a3a7d10de7cb 100644
> --- a/fs/ntfs3/ntfs_fs.h
> +++ b/fs/ntfs3/ntfs_fs.h
> @@ -60,19 +60,16 @@ struct ntfs_mount_options {
>  	u16 fs_fmask_inv;
>  	u16 fs_dmask_inv;
>  
> -	unsigned uid : 1, /* uid was set */
> -		gid : 1, /* gid was set */
> -		fmask : 1, /* fmask was set */
> -		dmask : 1, /*dmask was set*/
> -		sys_immutable : 1, /* immutable system files */
> -		discard : 1, /* issue discard requests on deletions */
> -		sparse : 1, /*create sparse files*/
> -		showmeta : 1, /*show meta files*/
> -		nohidden : 1, /*do not show hidden files*/
> -		force : 1, /*rw mount dirty volume*/
> -		noacs_rules : 1, /*exclude acs rules*/
> -		prealloc : 1 /*preallocate space when file is growing*/
> -		;
> +	unsigned fmask : 1; /* fmask was set */
> +	unsigned dmask : 1; /*dmask was set*/
> +	unsigned sys_immutable : 1; /* immutable system files */
> +	unsigned discard : 1; /* issue discard requests on deletions */
> +	unsigned sparse : 1; /*create sparse files*/
> +	unsigned showmeta : 1; /*show meta files*/
> +	unsigned nohidden : 1; /*do not show hidden files*/
> +	unsigned force : 1; /*rw mount dirty volume*/
> +	unsigned noacs_rules : 1; /*exclude acs rules*/
> +	unsigned prealloc : 1; /*preallocate space when file is growing*/
>  };
>  
>  /* special value to unpack and deallocate*/
> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> index d7408b4f6813..d28fab6c2297 100644
> --- a/fs/ntfs3/super.c
> +++ b/fs/ntfs3/super.c
> @@ -287,13 +287,11 @@ static int ntfs_fs_parse_param(struct fs_context *fc,
>  		opts->fs_uid = make_kuid(current_user_ns(), result.uint_32);
>  		if (!uid_valid(opts->fs_uid))
>  			return invalf(fc, "ntfs3: Invalid value for uid.");
> -		opts->uid = 1;
>  		break;
>  	case Opt_gid:
>  		opts->fs_gid = make_kgid(current_user_ns(), result.uint_32);
>  		if (!gid_valid(opts->fs_gid))
>  			return invalf(fc, "ntfs3: Invalid value for gid.");
> -		opts->gid = 1;
>  		break;
>  	case Opt_umask:
>  		if (result.uint_32 & ~07777)
> @@ -512,12 +510,10 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
>  	struct ntfs_mount_options *opts = sbi->options;
>  	struct user_namespace *user_ns = seq_user_ns(m);
>  
> -	if (opts->uid)
> -		seq_printf(m, ",uid=%u",
> -			   from_kuid_munged(user_ns, opts->fs_uid));
> -	if (opts->gid)
> -		seq_printf(m, ",gid=%u",
> -			   from_kgid_munged(user_ns, opts->fs_gid));
> +	seq_printf(m, ",uid=%u",
> +		  from_kuid_munged(user_ns, opts->fs_uid));
> +	seq_printf(m, ",gid=%u",
> +		  from_kgid_munged(user_ns, opts->fs_gid));
>  	if (opts->fmask)
>  		seq_printf(m, ",fmask=%04o", ~opts->fs_fmask_inv);
>  	if (opts->dmask)
> -- 
> 2.25.1
>