linux-kernel - Re: [PATCH] do_open(): Fix O_DIRECTORY

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230320115153.7n5cq4wl2hmcbndf@wittgenstein>
Date:   Mon, 20 Mar 2023 12:51:53 +0100
From:   Christian Brauner <brauner@...nel.org>
To:     Pedro Falcato <pedro.falcato@...il.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] do_open(): Fix O_DIRECTORY | O_CREAT behavior

On Mon, Mar 20, 2023 at 07:14:42AM +0000, Pedro Falcato wrote:
> On Linux, open(O_DIRECTORY | O_CREAT) has historically meant "open
> directory or create a regular file". This has remained mostly true,
> except open(O_DIR | O_CREAT) has started returning an error *while
> creating the file*. Restore the old behavior.
> 
> Signed-off-by: Pedro Falcato <pedro.falcato@...il.com>
> ---
> I did not explicitly add a Fixes: tag because I was unable to bisect this locally,

This dates back to the lookup rework done for v5.7. Specifically, this
was introduced by:

Fixes: 973d4b73fbaf ("do_last(): rejoin the common path even earlier in FMODE_{OPENED,CREATED} case")

> but it seems to me that this was introduced in the path walking refactoring done in early 2020.
> Al, if you have a rough idea of what may have added this bug, feel free to add a Fixes.
> 
> This should also probably get CC'd to stable, but I'll leave this to your criteria.
>  fs/namei.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index edfedfbccae..7b26db2f0f8 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3540,8 +3540,18 @@ static int do_open(struct nameidata *nd,
>  		if (unlikely(error))
>  			return error;
>  	}
> -	if ((nd->flags & LOOKUP_DIRECTORY) && !d_can_lookup(nd->path.dentry))
> -		return -ENOTDIR;
> +
> +	if ((open_flag & (O_DIRECTORY | O_CREAT)) != (O_DIRECTORY | O_CREAT) ||
> +	    !(file->f_mode & FMODE_CREATED)) {
> +		/* O_DIRECTORY | O_CREAT has the strange property of being the
> +		 * only open(O_DIRECTORY) lookup that can create and return a
> +		 * regular file *if we indeed did create*. Because of this,
> +		 * only return -ENOTDIR if we're not O_DIR | O_CREAT or if we
> +		 * did not create a file.
> +		 */

So before we continue down that road should we maybe treat this as a
chance to fix the old bug? Because this behavior of returning -ENOTDIR
has existed ever since v5.7 now. Since that time we had three LTS
releases all returning ENOTDIR even if the file was created.

So yeah, we could return to the old behavior. But we could also see this
as a chance to get rid of the bug. IOW, fail right at O_DIRECTORY | O_CREAT
with ENOTDIR and not even create the file anymore. No one has really
noticed this from 5.7 until now and afaict this has been a bug ever
since the dark ages and is mentioned as a bug on man 2 openat.