linux-kernel - Re: [PATCH] vfs: Don't exchange filenames unconditionally.

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: <20140918112206.GA26866@mguzik>
Date:	Thu, 18 Sep 2014 13:22:07 +0200
From:	Mateusz Guzik <mguzik@...hat.com>
To:	Mikhail Efremov <sem@...linux.org>
Cc:	linux-kernel@...r.kernel.org, Miklos Szeredi <mszeredi@...e.cz>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] vfs: Don't exchange filenames unconditionally.

On Thu, Sep 18, 2014 at 02:55:00PM +0400, Mikhail Efremov wrote:
> Only exchange source and destination filenames
> if flags contain RENAME_EXCHANGE.

But names are still exchanged without RENAME_EXCHANGE, provided they are
long enough.

da1ce0670c14 introduced this behaviour also for short names and this is
what you are reverting here.

That said, I think the patch is fine as an interim change, but should
note facts mentioned above.

For more info please see https://lkml.org/lkml/2014/9/7/6

I'll be posting a RFC patch for actual problem next week.

> In case if executable file was running and replaced by
> other file /proc/PID/exe should still show correct file name,
> not the old name of the file by which it was replaced.
> 




> The scenario when this bug manifests itself was like this:
> * ALT Linux uses rpm and start-stop-daemon;
> * during a package upgrade rpm creates a temporary file
>   for an executable to rename it upon successful unpacking;
> * start-stop-daemon is run subsequently and it obtains
>   the (nonexistant) temporary filename via /proc/PID/exe
>   thus failing to identify the running process.
> 
> Cc: Miklos Szeredi <mszeredi@...e.cz>
> Cc: Alexander Viro <viro@...iv.linux.org.uk>
> Cc: linux-fsdevel@...r.kernel.org
> 
> Fixes: da1ce0670c14 "vfs: add cross-rename"
> Signed-off-by: Mikhail Efremov <sem@...linux.org>
> ---
>  fs/dcache.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 7a5b514..3218570 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2372,7 +2372,8 @@ void dentry_update_name_case(struct dentry *dentry, struct qstr *name)
>  }
>  EXPORT_SYMBOL(dentry_update_name_case);
>  
> -static void switch_names(struct dentry *dentry, struct dentry *target)
> +static void switch_names(struct dentry *dentry, struct dentry *target,
> +			 bool exchange)
>  {
>  	if (dname_external(target)) {
>  		if (dname_external(dentry)) {
> @@ -2404,11 +2405,21 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
>  			/*
>  			 * Both are internal.
>  			 */
> -			unsigned int i;
> -			BUILD_BUG_ON(!IS_ALIGNED(DNAME_INLINE_LEN, sizeof(long)));
> -			for (i = 0; i < DNAME_INLINE_LEN / sizeof(long); i++) {
> -				swap(((long *) &dentry->d_iname)[i],
> -				     ((long *) &target->d_iname)[i]);
> +			if (exchange) {
> +				unsigned int i;
> +
> +				BUILD_BUG_ON(!IS_ALIGNED(DNAME_INLINE_LEN,
> +							 sizeof(long)));
> +				for (i = 0; i < DNAME_INLINE_LEN / sizeof(long);
> +									i++) {
> +					swap(((long *) &dentry->d_iname)[i],
> +					     ((long *) &target->d_iname)[i]);
> +				}
> +			} else {
> +				memcpy(dentry->d_iname, target->d_name.name,
> +						target->d_name.len + 1);
> +				dentry->d_name.len = target->d_name.len;
> +				return;
>  			}
>  		}
>  	}
> @@ -2510,7 +2521,7 @@ static void __d_move(struct dentry *dentry, struct dentry *target,
>  	list_del(&target->d_u.d_child);
>  
>  	/* Switch the names.. */
> -	switch_names(dentry, target);
> +	switch_names(dentry, target, exchange);
>  	swap(dentry->d_name.hash, target->d_name.hash);
>  
>  	/* ... and switch the parents */
> @@ -2649,7 +2660,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
>  
>  	dparent = dentry->d_parent;
>  
> -	switch_names(dentry, anon);
> +	switch_names(dentry, anon, false);
>  	swap(dentry->d_name.hash, anon->d_name.hash);
>  
>  	dentry->d_parent = dentry;
> -- 
> 1.8.5.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Mateusz Guzik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/