vfs: check ->get_link return value

PATCH In review Darrick J. Wong 4 participants 10 messages Oct 2, 2018 VFS

lore

Share & tools

lore

Darrick J. Wong

author

2018-10-02 05:25 UTC #0 lore raw

PATCH 1/1 vfs: check ->get_link return value

From: Darrick J. Wong <[email protected]>

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 <[email protected]>

---
 fs/namei.c |    4 ++++
 1 file changed, 4 insertions(+)

fs/namei.c +4 -0

index 0cab6494978c..0744ab981fa0 100644
@@ -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;
 }

Dave Chinner

2018-10-02 06:01 UTC #1 lore raw

Quoted 16 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>
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 <[email protected]>
---
 fs/namei.c |    4 ++++
 1 file changed, 4 insertions(+)
fs/namei.c +0 -0
index 0cab6494978c..0744ab981fa0 100644

fs/namei.c int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen) · line 4741

Quoted 5 lines

fs/namei.c +2 -0

@@ -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.

Signature · 2 lines

Dave Chinner
[email protected]

Darrick J. Wong

author

2018-10-02 06:14 UTC #2 lore raw

Quoted 26 lines · Dave Chinner

Quoted 16 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
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 <[email protected]>
---
 fs/namei.c |    4 ++++
 1 file changed, 4 insertions(+)
fs/namei.c +0 -0
index 0cab6494978c..0744ab981fa0 100644
fs/namei.c int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen) · line 4741
Quoted 5 lines
fs/namei.c +2 -0
@@ -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

Quoted 4 lines

#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

Quoted 6 lines

Cheers,
Dave.
Signature · 2 lines
Dave Chinner
[email protected]

Dave Chinner

2018-10-02 06:33 UTC #3 lore raw

Quoted 27 lines · Darrick J. Wong

Quoted 26 lines · Dave Chinner
Quoted 16 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
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 <[email protected]>
---
 fs/namei.c |    4 ++++
 1 file changed, 4 insertions(+)
fs/namei.c +0 -0
index 0cab6494978c..0744ab981fa0 100644
fs/namei.c int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen) · line 4741
Quoted 5 lines
fs/namei.c +2 -0
@@ -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

Quoted 6 lines

Quoted 4 lines
#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.

Quoted 2 lines

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.

Signature · 2 lines

Dave Chinner
[email protected]

Matthew Wilcox

2018-10-02 06:33 UTC #4 lore raw

In reply to Darrick J. Wong

Quoted 5 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>
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?

Quoted 27 lines

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/namei.c |    4 ++++
 1 file changed, 4 insertions(+)

fs/namei.c +4 -0

index 0cab6494978c..0744ab981fa0 100644
@@ -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;
 }

Darrick J. Wong

author

2018-10-02 07:04 UTC #5 lore raw

Quoted 8 lines · Matthew Wilcox

Quoted 5 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
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

Quoted 27 lines

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/namei.c |    4 ++++
 1 file changed, 4 insertions(+)

fs/namei.c +4 -0

index 0cab6494978c..0744ab981fa0 100644
@@ -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;
 }

Al Viro

maintainer

2018-10-02 08:35 UTC #6 lore raw

Quoted 6 lines · Darrick J. Wong

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()...

Al Viro

maintainer

2018-10-02 08:11 UTC #7 lore raw

In reply to Darrick J. Wong

Quoted 5 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>
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.

Darrick J. Wong

author

2018-10-02 08:48 UTC #8 lore raw

Quoted 7 lines · Al Viro

Quoted 5 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
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.

Quoted 6 lines

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...

Quoted 2 lines

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

Al Viro

maintainer

2018-10-02 09:27 UTC #9 lore raw

Quoted 15 lines · Darrick J. Wong

Quoted 7 lines · Al Viro
Quoted 5 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
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.
Quoted 6 lines
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.

Quoted 5 lines

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.