diff mbox series

[v2,1/2] vfs: block chmod of symlinks

Message ID 20200916002253.GP3265@brightrain.aerifal.cx
State New
Headers show
Series changes for addding fchmodat2 syscall | expand

Commit Message

Rich Felker Sept. 16, 2020, 12:22 a.m. UTC
It was discovered while implementing userspace emulation of fchmodat
AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise
it's not possible to target symlinks with chmod operations) that some
filesystems erroneously allow access mode of symlinks to be changed,
but return failure with EOPNOTSUPP (see glibc issue #14578 and commit
a492b1e5ef). This inconsistency is non-conforming and wrong, and the
consensus seems to be that it was unintentional to allow link modes to
be changed in the first place.

Signed-off-by: Rich Felker <dalias@libc.org>
---
 fs/open.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Greg Kroah-Hartman Sept. 16, 2020, 6:18 a.m. UTC | #1
On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote:
> It was discovered while implementing userspace emulation of fchmodat
> AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise
> it's not possible to target symlinks with chmod operations) that some
> filesystems erroneously allow access mode of symlinks to be changed,
> but return failure with EOPNOTSUPP (see glibc issue #14578 and commit
> a492b1e5ef). This inconsistency is non-conforming and wrong, and the
> consensus seems to be that it was unintentional to allow link modes to
> be changed in the first place.
> 
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
>  fs/open.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 9af548fb841b..cdb7964aaa6e 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode)
>  	struct iattr newattrs;
>  	int error;
>  
> +	/* 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. */
> +	if (S_ISLNK(inode->i_mode))
> +		return -EOPNOTSUPP;

I still fail to understand why these "buggy" filesystems can not be
fixed.  Why are you papering over a filesystem-specific-bug with this
core kernel change that we will forever have to keep?

thanks,

greg k-h
Christoph Hellwig Sept. 16, 2020, 6:23 a.m. UTC | #2
On Wed, Sep 16, 2020 at 08:18:15AM +0200, Greg KH wrote:
> On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote:
> > It was discovered while implementing userspace emulation of fchmodat
> > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise
> > it's not possible to target symlinks with chmod operations) that some
> > filesystems erroneously allow access mode of symlinks to be changed,
> > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit
> > a492b1e5ef). This inconsistency is non-conforming and wrong, and the
> > consensus seems to be that it was unintentional to allow link modes to
> > be changed in the first place.
> > 
> > Signed-off-by: Rich Felker <dalias@libc.org>
> > ---
> >  fs/open.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/fs/open.c b/fs/open.c
> > index 9af548fb841b..cdb7964aaa6e 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode)
> >  	struct iattr newattrs;
> >  	int error;
> >  
> > +	/* 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. */
> > +	if (S_ISLNK(inode->i_mode))
> > +		return -EOPNOTSUPP;
> 
> I still fail to understand why these "buggy" filesystems can not be
> fixed.  Why are you papering over a filesystem-specific-bug with this
> core kernel change that we will forever have to keep?

Because checking this once in the VFS is much saner than trying to
patch up a gazillion file systems.
Christoph Hellwig Sept. 16, 2020, 6:25 a.m. UTC | #3
On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote:
> It was discovered while implementing userspace emulation of fchmodat
> AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise
> it's not possible to target symlinks with chmod operations) that some
> filesystems erroneously allow access mode of symlinks to be changed,
> but return failure with EOPNOTSUPP (see glibc issue #14578 and commit
> a492b1e5ef). This inconsistency is non-conforming and wrong, and the
> consensus seems to be that it was unintentional to allow link modes to
> be changed in the first place.
> 
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
>  fs/open.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 9af548fb841b..cdb7964aaa6e 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode)
>  	struct iattr newattrs;
>  	int error;
>  
> +	/* 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. */
> +	if (S_ISLNK(inode->i_mode))
> +		return -EOPNOTSUPP;

Our usualy place for this would be setattr_prepare.  Also the comment
style is off, and I don't think we should talk about buggy file systems
here, but a policy to not allow the chmod.  I also suspect the right
error value is EINVAL - EOPNOTSUPP isn't really used in normal posix
file system interfaces.
Rich Felker Sept. 16, 2020, 3:36 p.m. UTC | #4
On Wed, Sep 16, 2020 at 08:18:15AM +0200, Greg KH wrote:
> On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote:
> > It was discovered while implementing userspace emulation of fchmodat
> > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise
> > it's not possible to target symlinks with chmod operations) that some
> > filesystems erroneously allow access mode of symlinks to be changed,
> > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit
> > a492b1e5ef). This inconsistency is non-conforming and wrong, and the
> > consensus seems to be that it was unintentional to allow link modes to
> > be changed in the first place.
> > 
> > Signed-off-by: Rich Felker <dalias@libc.org>
> > ---
> >  fs/open.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/fs/open.c b/fs/open.c
> > index 9af548fb841b..cdb7964aaa6e 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode)
> >  	struct iattr newattrs;
> >  	int error;
> >  
> > +	/* 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. */
> > +	if (S_ISLNK(inode->i_mode))
> > +		return -EOPNOTSUPP;
> 
> I still fail to understand why these "buggy" filesystems can not be
> fixed.  Why are you papering over a filesystem-specific-bug with this

Because that's what Christoph wanted, and it seems exposure of the
vector for applying chmod to symlinks was unintentional to begin with.
I have no preference how this is fixed as long as breakage is not
exposed to userspace via the new fchmodat2 syscall (since a broken
syscall would be worse than not having it at all).

> core kernel change that we will forever have to keep?

There's no fundamental reason it would have to be kept forever. The
contract remains "either it works and reports success, or it makes no
change and reports EOPNOTSUPP". It just can't do both.

Rich
Rich Felker Sept. 16, 2020, 3:41 p.m. UTC | #5
On Wed, Sep 16, 2020 at 07:25:53AM +0100, Christoph Hellwig wrote:
> On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote:
> > It was discovered while implementing userspace emulation of fchmodat
> > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise
> > it's not possible to target symlinks with chmod operations) that some
> > filesystems erroneously allow access mode of symlinks to be changed,
> > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit
> > a492b1e5ef). This inconsistency is non-conforming and wrong, and the
> > consensus seems to be that it was unintentional to allow link modes to
> > be changed in the first place.
> > 
> > Signed-off-by: Rich Felker <dalias@libc.org>
> > ---
> >  fs/open.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/fs/open.c b/fs/open.c
> > index 9af548fb841b..cdb7964aaa6e 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode)
> >  	struct iattr newattrs;
> >  	int error;
> >  
> > +	/* 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. */
> > +	if (S_ISLNK(inode->i_mode))
> > +		return -EOPNOTSUPP;
> 
> Our usualy place for this would be setattr_prepare.  Also the comment
> style is off, and I don't think we should talk about buggy file systems
> here, but a policy to not allow the chmod.  I also suspect the right
> error value is EINVAL - EOPNOTSUPP isn't really used in normal posix
> file system interfaces.

EINVAL is non-conforming here. POSIX defines it as unsupported mode or
flag. Lack of support for setting an access mode on symlinks is a
distinct failure reason, and POSIX does not allow overloading error
codes like this.

Even if it were permitted, it would be bad to do this because it would
make it impossible for the application to tell whether the cause of
failure is an invalid mode or that the filesystem/implementation
doesn't support modes on symlinks. This matters because one is usually
a fatal error and the other is a condition to ignore. Moreover, the
affected applications (e.g. coreutils, tar, etc.) already accept
EOPNOTSUPP here from libc. Defining a new error would break them
unless libc translated whatever the syscall returns to the expected
EOPNOTSUPP.

Rich
Al Viro Sept. 17, 2020, 4:07 a.m. UTC | #6
On Wed, Sep 16, 2020 at 07:25:53AM +0100, Christoph Hellwig wrote:
> On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote:
> > It was discovered while implementing userspace emulation of fchmodat
> > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise
> > it's not possible to target symlinks with chmod operations) that some
> > filesystems erroneously allow access mode of symlinks to be changed,
> > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit
> > a492b1e5ef). This inconsistency is non-conforming and wrong, and the
> > consensus seems to be that it was unintentional to allow link modes to
> > be changed in the first place.
> > 
> > Signed-off-by: Rich Felker <dalias@libc.org>
> > ---
> >  fs/open.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/fs/open.c b/fs/open.c
> > index 9af548fb841b..cdb7964aaa6e 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode)
> >  	struct iattr newattrs;
> >  	int error;
> >  
> > +	/* 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. */
> > +	if (S_ISLNK(inode->i_mode))
> > +		return -EOPNOTSUPP;
> 
> Our usualy place for this would be setattr_prepare.  Also the comment
> style is off, and I don't think we should talk about buggy file systems
> here, but a policy to not allow the chmod.  I also suspect the right
> error value is EINVAL - EOPNOTSUPP isn't really used in normal posix
> file system interfaces.

Er...   Wasn't that an ACL-related crap?  XFS calling posix_acl_chmod()
after it has committed to i_mode change, propagating the error to
caller of ->notify_change(), IIRC...

Put it another way, why do we want
        if (!inode->i_op->set_acl)
                return -EOPNOTSUPP;
in posix_acl_chmod(), when we have
        if (!IS_POSIXACL(inode))
                return 0;
right next to it?  If nothing else, make that
	if (!IS_POSIXACL(inode) || !inode->i_op->get_acl)
		return 0;	// piss off - nothing to adjust here
Al Viro Sept. 17, 2020, 4:15 a.m. UTC | #7
On Thu, Sep 17, 2020 at 05:07:15AM +0100, Al Viro wrote:
> On Wed, Sep 16, 2020 at 07:25:53AM +0100, Christoph Hellwig wrote:
> > On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote:
> > > It was discovered while implementing userspace emulation of fchmodat
> > > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise
> > > it's not possible to target symlinks with chmod operations) that some
> > > filesystems erroneously allow access mode of symlinks to be changed,
> > > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit
> > > a492b1e5ef). This inconsistency is non-conforming and wrong, and the
> > > consensus seems to be that it was unintentional to allow link modes to
> > > be changed in the first place.
> > > 
> > > Signed-off-by: Rich Felker <dalias@libc.org>
> > > ---
> > >  fs/open.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/fs/open.c b/fs/open.c
> > > index 9af548fb841b..cdb7964aaa6e 100644
> > > --- a/fs/open.c
> > > +++ b/fs/open.c
> > > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode)
> > >  	struct iattr newattrs;
> > >  	int error;
> > >  
> > > +	/* 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. */
> > > +	if (S_ISLNK(inode->i_mode))
> > > +		return -EOPNOTSUPP;
> > 
> > Our usualy place for this would be setattr_prepare.  Also the comment
> > style is off, and I don't think we should talk about buggy file systems
> > here, but a policy to not allow the chmod.  I also suspect the right
> > error value is EINVAL - EOPNOTSUPP isn't really used in normal posix
> > file system interfaces.
> 
> Er...   Wasn't that an ACL-related crap?  XFS calling posix_acl_chmod()
> after it has committed to i_mode change, propagating the error to
> caller of ->notify_change(), IIRC...
> 
> Put it another way, why do we want
>         if (!inode->i_op->set_acl)
>                 return -EOPNOTSUPP;
> in posix_acl_chmod(), when we have
>         if (!IS_POSIXACL(inode))
>                 return 0;
> right next to it?  If nothing else, make that
> 	if (!IS_POSIXACL(inode) || !inode->i_op->get_acl)
> 		return 0;	// piss off - nothing to adjust here

Arrgh...  That'd break shmem and similar filesystems...  Still, it
feels like we should _not_ bother in cases when there's no ACL
for that sucker; after all, if get_acl() returns NULL, we quietly
return 0 and that's it.

How about something like this instead?

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 95882b3f5f62..2339160fabab 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -559,8 +559,6 @@ posix_acl_chmod(struct inode *inode, umode_t mode)
 
 	if (!IS_POSIXACL(inode))
 		return 0;
-	if (!inode->i_op->set_acl)
-		return -EOPNOTSUPP;
 
 	acl = get_acl(inode, ACL_TYPE_ACCESS);
 	if (IS_ERR_OR_NULL(acl)) {
@@ -569,6 +567,10 @@ posix_acl_chmod(struct inode *inode, umode_t mode)
 		return PTR_ERR(acl);
 	}
 
+	if (!inode->i_op->set_acl) {
+		posix_acl_release(acl);
+		return -EOPNOTSUPP;
+	}
 	ret = __posix_acl_chmod(&acl, GFP_KERNEL, mode);
 	if (ret)
 		return ret;
Rich Felker Sept. 17, 2020, 6:42 p.m. UTC | #8
On Thu, Sep 17, 2020 at 05:15:03AM +0100, Al Viro wrote:
> On Thu, Sep 17, 2020 at 05:07:15AM +0100, Al Viro wrote:
> > On Wed, Sep 16, 2020 at 07:25:53AM +0100, Christoph Hellwig wrote:
> > > On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote:
> > > > It was discovered while implementing userspace emulation of fchmodat
> > > > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise
> > > > it's not possible to target symlinks with chmod operations) that some
> > > > filesystems erroneously allow access mode of symlinks to be changed,
> > > > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit
> > > > a492b1e5ef). This inconsistency is non-conforming and wrong, and the
> > > > consensus seems to be that it was unintentional to allow link modes to
> > > > be changed in the first place.
> > > > 
> > > > Signed-off-by: Rich Felker <dalias@libc.org>
> > > > ---
> > > >  fs/open.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/fs/open.c b/fs/open.c
> > > > index 9af548fb841b..cdb7964aaa6e 100644
> > > > --- a/fs/open.c
> > > > +++ b/fs/open.c
> > > > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode)
> > > >  	struct iattr newattrs;
> > > >  	int error;
> > > >  
> > > > +	/* 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. */
> > > > +	if (S_ISLNK(inode->i_mode))
> > > > +		return -EOPNOTSUPP;
> > > 
> > > Our usualy place for this would be setattr_prepare.  Also the comment
> > > style is off, and I don't think we should talk about buggy file systems
> > > here, but a policy to not allow the chmod.  I also suspect the right
> > > error value is EINVAL - EOPNOTSUPP isn't really used in normal posix
> > > file system interfaces.
> > 
> > Er...   Wasn't that an ACL-related crap?  XFS calling posix_acl_chmod()
> > after it has committed to i_mode change, propagating the error to
> > caller of ->notify_change(), IIRC...
> > 
> > Put it another way, why do we want
> >         if (!inode->i_op->set_acl)
> >                 return -EOPNOTSUPP;
> > in posix_acl_chmod(), when we have
> >         if (!IS_POSIXACL(inode))
> >                 return 0;
> > right next to it?  If nothing else, make that
> > 	if (!IS_POSIXACL(inode) || !inode->i_op->get_acl)
> > 		return 0;	// piss off - nothing to adjust here
> 
> Arrgh...  That'd break shmem and similar filesystems...  Still, it
> feels like we should _not_ bother in cases when there's no ACL
> for that sucker; after all, if get_acl() returns NULL, we quietly
> return 0 and that's it.
> 
> How about something like this instead?
> 
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 95882b3f5f62..2339160fabab 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -559,8 +559,6 @@ posix_acl_chmod(struct inode *inode, umode_t mode)
>  
>  	if (!IS_POSIXACL(inode))
>  		return 0;
> -	if (!inode->i_op->set_acl)
> -		return -EOPNOTSUPP;
>  
>  	acl = get_acl(inode, ACL_TYPE_ACCESS);
>  	if (IS_ERR_OR_NULL(acl)) {
> @@ -569,6 +567,10 @@ posix_acl_chmod(struct inode *inode, umode_t mode)
>  		return PTR_ERR(acl);
>  	}
>  
> +	if (!inode->i_op->set_acl) {
> +		posix_acl_release(acl);
> +		return -EOPNOTSUPP;
> +	}
>  	ret = __posix_acl_chmod(&acl, GFP_KERNEL, mode);
>  	if (ret)
>  		return ret;

Does this make chmod of links behave consistently (either succeeding
with return value 0, or returning -EOPNOTSUPP without doing anything)
for all filesystems? I'm fine with (and would probably prefer) this
fix if it's a complete one. If this goes in I think my patch 1/2 can
just be dropped and patch 2/2 behaves as intended; does that sound
correct to you?

Rich
Christoph Hellwig Sept. 29, 2020, 5:49 p.m. UTC | #9
On Thu, Sep 17, 2020 at 05:15:03AM +0100, Al Viro wrote:
> Arrgh...  That'd break shmem and similar filesystems...  Still, it
> feels like we should _not_ bother in cases when there's no ACL
> for that sucker; after all, if get_acl() returns NULL, we quietly
> return 0 and that's it.
> 
> How about something like this instead?

Do you plan to turn this into a submission?

Rich, can you share your original reproducer?  I would be really
helpful to have it wired up in xfstests as a regression tests.
diff mbox series

Patch

diff --git a/fs/open.c b/fs/open.c
index 9af548fb841b..cdb7964aaa6e 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -570,6 +570,12 @@  int chmod_common(const struct path *path, umode_t mode)
 	struct iattr newattrs;
 	int error;
 
+	/* 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. */
+	if (S_ISLNK(inode->i_mode))
+		return -EOPNOTSUPP;
+
 	error = mnt_want_write(path->mnt);
 	if (error)
 		return error;