[PATCH] fuse: mark all aliases of an inode stale in unlink

[PATCH] fuse: mark all aliases of an inode stale in unlink

[Anand Avati]

Problem:
	create("filename") => 0
	link("filename", "linkname") => 0
	unlink("filename") => 0
	link("linkname", "filename") => -ENOENT ### BUG ###

This is a test case link section of the Poix compliance test suite from
ntfs-3g project.

Cause:
	fuse_unlink() clears i_nlink of the inode. If the inode has
hardlinks (and hence aliases) they kept fresh (within the entry
timeout bounds.) A link() call happening right after unlink now checks
for i_nlink == 0 (since patch aae8a97d3ec30788790d1720b71d76fd8eb44b73)
and fails the syscall for security reasons.

Fix:
	Instead of marking the just-unlinked entry as stale, mark all
the aliases as stale so that a revalidation of the alias is done during
path resolution.

Signed-off-by: Anand Avati <avati@redhat.com>
---
 fs/fuse/dir.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 2066328..880e9dd 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -103,6 +103,25 @@ void fuse_invalidate_entry_cache(struct dentry *entry)
 }
 
 /*
+ * Mark all dentry aliases of an inode as stale. It is necessary to mark
+ * all aliases as stale because a link() on an inode performed right
+ * after an unlink on one of the hardlinks will pick the inode without
+ * a revalidate and find i_nlink == 0. This ends up getting checked as
+ * a security vulnerability and fail -ENOENT. Marking all aliases as stale
+ * will instead force lookup the inode freshly at the time of link()
+ */
+void fuse_invalidate_all_aliases(struct inode *inode)
+{
+	struct dentry *alias;
+
+	spin_lock(&inode->i_lock);
+	list_for_each_entry(alias, &inode->i_dentry, d_alias) {
+		fuse_invalidate_entry_cache(alias);
+	}
+	spin_unlock(&inode->i_lock);
+}
+
+/*
  * Same as fuse_invalidate_entry_cache(), but also try to remove the
  * dentry from the hash
  */
@@ -653,7 +672,7 @@ static int fuse_unlink(struct inode *dir, struct dentry *entry)
 		clear_nlink(inode);
 		fuse_invalidate_attr(inode);
 		fuse_invalidate_attr(dir);
-		fuse_invalidate_entry_cache(entry);
+		fuse_invalidate_all_aliases(inode);
 	} else if (err == -EINTR)
 		fuse_invalidate_entry(entry);
 	return err;
-- 
1.7.6.4


-- 
"It's easier said than done."

... and if you don't believe it, try proving that it's easier done than
said, and you'll see that "it's easier said that `it's easier done than
said' than it is done", which really proves that "it's easier said than
done".

[PATCH] fuse: mark all aliases of an inode stale in unlink

[Anand Avati]

Problem:
	create("filename") => 0
	link("filename", "linkname") => 0
	unlink("filename") => 0
	link("linkname", "filename") => -ENOENT ### BUG ###

This is a test case link section of the Poix compliance test suite from
ntfs-3g project.

Cause:
	fuse_unlink() clears i_nlink of the inode. If the inode has
hardlinks (and hence aliases) they kept fresh (within the entry
timeout bounds.) A link() call happening right after unlink now checks
for i_nlink == 0 (since patch aae8a97d3ec30788790d1720b71d76fd8eb44b73)
and fails the syscall for security reasons.

Fix:
	Instead of marking the just-unlinked entry as stale, mark all
the aliases as stale so that a revalidation of the alias is done during
path resolution.

Signed-off-by: Anand Avati <avati@redhat.com>
---
 fs/fuse/dir.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 2066328..880e9dd 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -103,6 +103,25 @@ void fuse_invalidate_entry_cache(struct dentry *entry)
 }
 
 /*
+ * Mark all dentry aliases of an inode as stale. It is necessary to mark
+ * all aliases as stale because a link() on an inode performed right
+ * after an unlink on one of the hardlinks will pick the inode without
+ * a revalidate and find i_nlink == 0. This ends up getting checked as
+ * a security vulnerability and fail -ENOENT. Marking all aliases as stale
+ * will instead force lookup the inode freshly at the time of link()
+ */
+void fuse_invalidate_all_aliases(struct inode *inode)
+{
+	struct dentry *alias;
+
+	spin_lock(&inode->i_lock);
+	list_for_each_entry(alias, &inode->i_dentry, d_alias) {
+		fuse_invalidate_entry_cache(alias);
+	}
+	spin_unlock(&inode->i_lock);
+}
+
+/*
  * Same as fuse_invalidate_entry_cache(), but also try to remove the
  * dentry from the hash
  */
@@ -653,7 +672,7 @@ static int fuse_unlink(struct inode *dir, struct dentry *entry)
 		clear_nlink(inode);
 		fuse_invalidate_attr(inode);
 		fuse_invalidate_attr(dir);
-		fuse_invalidate_entry_cache(entry);
+		fuse_invalidate_all_aliases(inode);
 	} else if (err == -EINTR)
 		fuse_invalidate_entry(entry);
 	return err;
-- 
1.7.6.4

Re: [PATCH] fuse: mark all aliases of an inode stale in unlink

[Miklos Szeredi]

Anand Avati <avati@redhat.com> writes:

> Problem:
> 	create("filename") => 0
> 	link("filename", "linkname") => 0
> 	unlink("filename") => 0
> 	link("linkname", "filename") => -ENOENT ### BUG ###
>
> This is a test case link section of the Poix compliance test suite from
> ntfs-3g project.
>
> Cause:
> 	fuse_unlink() clears i_nlink of the inode. If the inode has
> hardlinks (and hence aliases) they kept fresh (within the entry
> timeout bounds.) A link() call happening right after unlink now checks
> for i_nlink == 0 (since patch aae8a97d3ec30788790d1720b71d76fd8eb44b73)
> and fails the syscall for security reasons.
>
> Fix:
> 	Instead of marking the just-unlinked entry as stale, mark all
> the aliases as stale so that a revalidation of the alias is done during
> path resolution.

This is only a partial fix: the entry may still be referenced and used:
linkat(..., AT_EMPTY_PATH).  There are also several other places where
i_nlink is cheked without refreshing the attributes.

So how about changing that clear_nlink() to drop_nlink()?

I don't quite understand what that comment above clear_nlink() is saying
but I suspect it comes from before ->drop_inode was used.  Now the inode
will go away if no dentry is referencing it regardless of the value of
i_nlink.

Untested patch below.  Does that work for you?

Thanks,
Miklos


diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 2066328..517a7f5 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -645,12 +645,7 @@ static int fuse_unlink(struct inode *dir, struct dentry *entry)
 	if (!err) {
 		struct inode *inode = entry->d_inode;
 
-		/*
-		 * Set nlink to zero so the inode can be cleared, if the inode
-		 * does have more links this will be discovered at the next
-		 * lookup/getattr.
-		 */
-		clear_nlink(inode);
+		drop_nlink(inode);
 		fuse_invalidate_attr(inode);
 		fuse_invalidate_attr(dir);
 		fuse_invalidate_entry_cache(entry);

Re: [PATCH] fuse: mark all aliases of an inode stale in unlink

[Anand Avati]

On 01/20/2012 08:10 PM, Miklos Szeredi wrote:

> Untested patch below.  Does that work for you?
>
> Thanks,
> Miklos
>
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 2066328..517a7f5 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -645,12 +645,7 @@ static int fuse_unlink(struct inode *dir, struct dentry *entry)
>   	if (!err) {
>   		struct inode *inode = entry->d_inode;
>
> -		/*
> -		 * Set nlink to zero so the inode can be cleared, if the inode
> -		 * does have more links this will be discovered at the next
> -		 * lookup/getattr.
> -		 */
> -		clear_nlink(inode);
> +		drop_nlink(inode);
>   		fuse_invalidate_attr(inode);
>   		fuse_invalidate_attr(dir);
>   		fuse_invalidate_entry_cache(entry);

That will work for me!

Thanks,
Avati