[PATCH] vfs: check ->get_link return value

[PATCH] vfs: check ->get_link return value

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Teach callers of inode->i_op->get_link in the vfs code to check for a
NULL return value and return an error status instead of blindly
dereferencing the returned NULL pointer.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/namei.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index 0cab6494978c..0744ab981fa0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4737,6 +4737,8 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
 		if (IS_ERR(link))
 			return PTR_ERR(link);
 	}
+	if (!link)
+		return -EUCLEAN;
 	res = readlink_copy(buffer, buflen, link);
 	do_delayed_call(&done);
 	return res;
@@ -4763,6 +4765,8 @@ const char *vfs_get_link(struct dentry *dentry, struct delayed_call *done)
 		res = ERR_PTR(security_inode_readlink(dentry));
 		if (!res)
 			res = inode->i_op->get_link(dentry, inode, done);
+		if (!res)
+			return ERR_PTR(-EUCLEAN);
 	}
 	return res;
 }

Re: [PATCH] vfs: check ->get_link return value

[Dave Chinner]

On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Teach callers of inode->i_op->get_link in the vfs code to check for a
> NULL return value and return an error status instead of blindly
> dereferencing the returned NULL pointer.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/namei.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 0cab6494978c..0744ab981fa0 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4737,6 +4737,8 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
>  		if (IS_ERR(link))
>  			return PTR_ERR(link);
>  	}
> +	if (!link)
> +		return -EUCLEAN;

If we are going to start returning this as a filesystem corruption
error from the VFS, can we please start with adding

#define EFSCORRUPTED	EUCLEAN

into one of the global error definition headers? The code makes much
more sense when it uses EFSCORRUPTED....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] vfs: check ->get_link return value

[Darrick J. Wong]

On Tue, Oct 02, 2018 at 09:21:28AM +1000, Dave Chinner wrote:
> On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Teach callers of inode->i_op->get_link in the vfs code to check for a
> > NULL return value and return an error status instead of blindly
> > dereferencing the returned NULL pointer.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/namei.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 0cab6494978c..0744ab981fa0 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -4737,6 +4737,8 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
> >  		if (IS_ERR(link))
> >  			return PTR_ERR(link);
> >  	}
> > +	if (!link)
> > +		return -EUCLEAN;
> 
> If we are going to start returning this as a filesystem corruption
> error from the VFS, can we please start with adding
> 
> #define EFSCORRUPTED	EUCLEAN
> 
> into one of the global error definition headers? The code makes much
> more sense when it uses EFSCORRUPTED....

Ok, which header file?  include/uapi/asm-generic/errno.h ?

We'll finally be able to get rid of the redundant definitions in
ext[24] too.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH] vfs: check ->get_link return value

[Matthew Wilcox]

On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Teach callers of inode->i_op->get_link in the vfs code to check for a
> NULL return value and return an error status instead of blindly
> dereferencing the returned NULL pointer.

Is that better than having the get_link method return ERR_PTR(-EUCLEAN) itself?

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/namei.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 0cab6494978c..0744ab981fa0 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4737,6 +4737,8 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
>  		if (IS_ERR(link))
>  			return PTR_ERR(link);
>  	}
> +	if (!link)
> +		return -EUCLEAN;
>  	res = readlink_copy(buffer, buflen, link);
>  	do_delayed_call(&done);
>  	return res;
> @@ -4763,6 +4765,8 @@ const char *vfs_get_link(struct dentry *dentry, struct delayed_call *done)
>  		res = ERR_PTR(security_inode_readlink(dentry));
>  		if (!res)
>  			res = inode->i_op->get_link(dentry, inode, done);
> +		if (!res)
> +			return ERR_PTR(-EUCLEAN);
>  	}
>  	return res;
>  }

Re: [PATCH] vfs: check ->get_link return value

[Dave Chinner]

On Mon, Oct 01, 2018 at 04:33:36PM -0700, Darrick J. Wong wrote:
> On Tue, Oct 02, 2018 at 09:21:28AM +1000, Dave Chinner wrote:
> > On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Teach callers of inode->i_op->get_link in the vfs code to check for a
> > > NULL return value and return an error status instead of blindly
> > > dereferencing the returned NULL pointer.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  fs/namei.c |    4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index 0cab6494978c..0744ab981fa0 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -4737,6 +4737,8 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
> > >  		if (IS_ERR(link))
> > >  			return PTR_ERR(link);
> > >  	}
> > > +	if (!link)
> > > +		return -EUCLEAN;
> > 
> > If we are going to start returning this as a filesystem corruption
> > error from the VFS, can we please start with adding
> > 
> > #define EFSCORRUPTED	EUCLEAN
> > 
> > into one of the global error definition headers? The code makes much
> > more sense when it uses EFSCORRUPTED....
> 
> Ok, which header file?  include/uapi/asm-generic/errno.h ?

Seems reasonable, because that's where EUCLEAN is defined.

> We'll finally be able to get rid of the redundant definitions in
> ext[24] too.

And filesystems like btrfs can be converted from EUCLEAN to
EFSCORRUPTED, too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] vfs: check ->get_link return value

[Darrick J. Wong]

On Mon, Oct 01, 2018 at 04:52:51PM -0700, Matthew Wilcox wrote:
> On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Teach callers of inode->i_op->get_link in the vfs code to check for a
> > NULL return value and return an error status instead of blindly
> > dereferencing the returned NULL pointer.
> 
> Is that better than having the get_link method return ERR_PTR(-EUCLEAN) itself?

get_link doesn't need the EFSCORRUPTED return; all two of its callers
handle null pointer returns correctly and they don't return the ->get_link
return value directly to userspace.

It's just these two functions below whose callers assume they have to
deal an error pointer or that it's totally safe to dereference it.

--D

> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/namei.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 0cab6494978c..0744ab981fa0 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -4737,6 +4737,8 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
> >  		if (IS_ERR(link))
> >  			return PTR_ERR(link);
> >  	}
> > +	if (!link)
> > +		return -EUCLEAN;
> >  	res = readlink_copy(buffer, buflen, link);
> >  	do_delayed_call(&done);
> >  	return res;
> > @@ -4763,6 +4765,8 @@ const char *vfs_get_link(struct dentry *dentry, struct delayed_call *done)
> >  		res = ERR_PTR(security_inode_readlink(dentry));
> >  		if (!res)
> >  			res = inode->i_op->get_link(dentry, inode, done);
> > +		if (!res)
> > +			return ERR_PTR(-EUCLEAN);
> >  	}
> >  	return res;
> >  }

Re: [PATCH] vfs: check ->get_link return value

[Al Viro]

On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Teach callers of inode->i_op->get_link in the vfs code to check for a
> NULL return value and return an error status instead of blindly
> dereferencing the returned NULL pointer.

IDGI.  If you want it to fail with -EUCLEAN, then by all means return
it as you would any other error.

I've no problem with "fs image is fucked, return an error".  However,
"fs driver is fucked, paper over that if we'd caught one of the
symptoms" is a different story.

NAK in that form.  If we have that happen, let the damn thing oops.
Quietly papering over bugs like that is just plain wrong.

Re: [PATCH] vfs: check ->get_link return value

[Al Viro]

On Mon, Oct 01, 2018 at 05:23:32PM -0700, Darrick J. Wong wrote:

> get_link doesn't need the EFSCORRUPTED return; all two of its callers
> handle null pointer returns correctly and they don't return the ->get_link
> return value directly to userspace.
> 
> It's just these two functions below whose callers assume they have to
> deal an error pointer or that it's totally safe to dereference it.

No.  The only case when ->get_link() has any business returning NULL is
when it has done nd_jump_link().  Those should never come without explicit
->readlink() and they should never be fed to vfs_get_link(), so they are
not a problem; anything else is a filesystem driver bug, plain and simple.

Check for NULL in fs/namei.c:get_link() is *NOT* "defensive programming"
bullshit; there it can legitimately happen (aforementioned procfs-style
symlinks).  Note that there it is not an error at all.

Current calling conventions are:
	* return ERR_PTR(-E...) on error
	* return pointer to symlink body to be traversed on success
	* return NULL when ->get_link() instances has jumped to destination
on its own and there's no "symlink body" to be traversed.  For such symlinks
we obviously need an explicit ->readlink() (for whatever string we want
readlink(2) to return).  These should not be occur on anything NFS-exported
or on overlayfs layers, since neither NFSD nor overlayfs don't know what
to do with such.

What you are proposing is a weird change along the lines of "if you
accidentally return NULL it will be treated as empty body, except when it
occurs on NFS exports or overlayfs layers; in such cases it will be
interpreted as fs corruption.  $DEITY help you if real procfs-style
symlink hits one of those, since nd_jump_link() called by those will
oops in such conditions".

As a mitigation strategy it sucks.  As part of calling conventions it's
confusing and AFAICS absolutely pointless.

NAK.  And I'm really curious - what has lead to that?  Because procfs-style
symlink in such conditions would have oopsed before it returned from
->get_link()...

Re: [PATCH] vfs: check ->get_link return value

[Darrick J. Wong]

On Tue, Oct 02, 2018 at 02:31:06AM +0100, Al Viro wrote:
> On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Teach callers of inode->i_op->get_link in the vfs code to check for a
> > NULL return value and return an error status instead of blindly
> > dereferencing the returned NULL pointer.
> 
> IDGI.  If you want it to fail with -EUCLEAN, then by all means return
> it as you would any other error.
> 
> I've no problem with "fs image is fucked, return an error".  However,
> "fs driver is fucked, paper over that if we'd caught one of the
> symptoms" is a different story.

This whole thread got started from a suggestion Christoph made about a
patch I had to fix the XFS side to return an error instead of a null
pointer:

https://www.spinics.net/lists/linux-xfs/msg21372.html

I hadn't thought it was all /that/ necessary to fix the vfs since there
are plenty of other places where the vfs assumes the fs knows what it's
doing and sneezes hard if not...

> NAK in that form.  If we have that happen, let the damn thing oops.
> Quietly papering over bugs like that is just plain wrong.

Sounds good to me!  Patch withdrawn. :)

--D

Re: [PATCH] vfs: check ->get_link return value

[Al Viro]

On Mon, Oct 01, 2018 at 07:07:12PM -0700, Darrick J. Wong wrote:
> On Tue, Oct 02, 2018 at 02:31:06AM +0100, Al Viro wrote:
> > On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Teach callers of inode->i_op->get_link in the vfs code to check for a
> > > NULL return value and return an error status instead of blindly
> > > dereferencing the returned NULL pointer.
> > 
> > IDGI.  If you want it to fail with -EUCLEAN, then by all means return
> > it as you would any other error.
> > 
> > I've no problem with "fs image is fucked, return an error".  However,
> > "fs driver is fucked, paper over that if we'd caught one of the
> > symptoms" is a different story.
> 
> This whole thread got started from a suggestion Christoph made about a
> patch I had to fix the XFS side to return an error instead of a null
> pointer:
> 
> https://www.spinics.net/lists/linux-xfs/msg21372.html

Ugh...  What should happen for that to trigger?  If anything, I would rather
validate that somewhere around xfs_setup_iops() *AND* set ->i_link at the
same time, killing the whole xfs_vn_get_link_inline() thing (just use
simple_get_link() instead)...

See another reply for the reasons why such mitigation makes no sense.