linux-kernel - RE: [PATCH] vfs: add fchmodat2 syscall

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: <1111806ca0344527a8855616e46346c5@AcuMS.aculab.com>
Date:   Thu, 10 Sep 2020 15:23:40 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Rich Felker' <dalias@...c.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>
CC:     Alexander Viro <viro@...iv.linux.org.uk>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] vfs: add fchmodat2 syscall

From: Rich Felker
> Sent: 10 September 2020 15:24
...
> index 9af548fb841b..570a21f4d81e 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -610,15 +610,30 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode)
>  	return err;
>  }
> 
> -static int do_fchmodat(int dfd, const char __user *filename, umode_t mode)
> +static int do_fchmodat(int dfd, const char __user *filename, umode_t mode, int flags)
>  {
>  	struct path path;
>  	int error;
>  	unsigned int lookup_flags = LOOKUP_FOLLOW;
> +
> +	if (flags & AT_SYMLINK_NOFOLLOW)
> +		lookup_flags &= ~LOOKUP_FOLLOW;
> +	if (flags & ~AT_SYMLINK_NOFOLLOW)
> +		return -EINVAL;

I think I'd swap over those two tests.
So unsupported flags are clearly errored.

>  retry:
>  	error = user_path_at(dfd, filename, lookup_flags, &path);
>  	if (!error) {
> -		error = chmod_common(&path, mode);
> +		/* Block chmod from getting to fs layer. Ideally the
> +		 * fs would either allow it or fail with EOPNOTSUPP,
> +		 * but some are buggy and return an error but change
> +		 * the mode, which is non-conforming and wrong.
> +		 * Userspace emulation of AT_SYMLINK_NOFOLLOW in
> +		 * glibc and musl blocked it too, for same reason. */
> +		if (S_ISLNK(path.dentry->d_inode->i_mode)
> +		    && (flags & AT_SYMLINK_NOFOLLOW))
> +			error = -EOPNOTSUPP;

Again swap the order of the tests. I think it reads better as:
		if ((flags & AT_SYMLINK_NOFOLLOW)
		    && S_ISLNK(path.dentry->d_inode->i_mode))
			error = -EOPNOTSUPP;
As well as saving a few clock cycles.

> +		else
> +			error = chmod_common(&path, mode);
>  		path_put(&path);
>  		if (retry_estale(error, lookup_flags)) {
>  			lookup_flags |= LOOKUP_REVAL;
...

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)