linux-kernel - Re: [PATCH] tracefs: avoid setting i

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: <20230816155245.6ead4384@gandalf.local.home>
Date:   Wed, 16 Aug 2023 15:52:45 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Sishuai Gong <sishuai.system@...il.com>
Cc:     mhiramat@...nel.org, linux-kernel@...r.kernel.org,
        linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH] tracefs: avoid setting i_mode to a temp value

On Thu, 10 Aug 2023 20:59:26 -0400
Sishuai Gong <sishuai.system@...il.com> wrote:

> Right now inode->i_mode is updated twice to reach the desired value
> in tracefs_apply_options(). Because there is no lock protecting the two
> writes, other threads might read the intermediate value of inode->i_mode.
> 
> Thread-1			Thread-2
> // tracefs_apply_options()	//e.g., acl_permission_check
> inode->i_mode &= ~S_IALLUGO;
> 				unsigned int mode = inode->i_mode;
> inode->i_mode |= opts->mode;
> 
> I think there is no need to introduce a lock but it is better to
> only update inode->i_mode ONCE, so the readers will either see the old
> or latest value, rather than an intermediate/temporary value.
> 
> Signed-off-by: Sishuai Gong <sishuai.system@...il.com>
> ---
>  fs/tracefs/inode.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> index 57ac8aa4a724..dca84ebb62fa 100644
> --- a/fs/tracefs/inode.c
> +++ b/fs/tracefs/inode.c
> @@ -297,8 +297,7 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
>  	 */
>  
>  	if (!remount || opts->opts & BIT(Opt_mode)) {
> -		inode->i_mode &= ~S_IALLUGO;
> -		inode->i_mode |= opts->mode;
> +		inode->i_mode = (inode->i_mode & ~S_IALLUGO) | opts->mode;

You do realize that the compiler could decide to keep the original logic
even with this change? If it was crucial that the compiler did not, you
would need to have:

	if (!remount || opts->opts & BIT(Opt_mode)) {
		umode_t tmp = READ_ONCE(inode->i_mode);

		tmp &= ~S_IALLUGO
		tmp |= opts->mode;
		WRITE_ONCE(inode->i_mode, tmp);
	}		

And if you notice the !remount flag, this is only preformed when the file
system is actually mounted. Are the files visible before then?

Can you produce this race?

-- Steve



>  	}
>  
>  	if (!remount || opts->opts & BIT(Opt_uid))