[PATCH] nfs: Fix potential posix_acl refcnt leak in nfs3_set_acl

[PATCH] nfs: Fix potential posix_acl refcnt leak in nfs3_set_acl

[Andreas Gruenbacher]

nfs3_set_acl keeps track of the acl it allocated locally to determine if an acl
needs to be released at the end.  This results in a memory leak when the
function allocates an acl as well as a default acl.  Fix by releasing acls
that differ from the acl originally passed into nfs3_set_acl.

Fixes: b7fa0554cf1b ("[PATCH] NFS: Add support for NFSv3 ACLs")
Reported-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/nfs/nfs3acl.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
index c5c3fc6e6c60..26c94b32d6f4 100644
--- a/fs/nfs/nfs3acl.c
+++ b/fs/nfs/nfs3acl.c
@@ -253,37 +253,45 @@ int nfs3_proc_setacls(struct inode *inode, struct posix_acl *acl,
 
 int nfs3_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
-	struct posix_acl *alloc = NULL, *dfacl = NULL;
+	struct posix_acl *orig = acl, *dfacl = NULL, *alloc;
 	int status;
 
 	if (S_ISDIR(inode->i_mode)) {
 		switch(type) {
 		case ACL_TYPE_ACCESS:
-			alloc = dfacl = get_acl(inode, ACL_TYPE_DEFAULT);
+			alloc = get_acl(inode, ACL_TYPE_DEFAULT);
 			if (IS_ERR(alloc))
 				goto fail;
+			dfacl = alloc;
 			break;
 
 		case ACL_TYPE_DEFAULT:
-			dfacl = acl;
-			alloc = acl = get_acl(inode, ACL_TYPE_ACCESS);
+			alloc = get_acl(inode, ACL_TYPE_ACCESS);
 			if (IS_ERR(alloc))
 				goto fail;
+			dfacl = acl;
+			acl = alloc;
 			break;
 		}
 	}
 
 	if (acl == NULL) {
-		alloc = acl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
+		alloc = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
 		if (IS_ERR(alloc))
 			goto fail;
+		acl = alloc;
 	}
 	status = __nfs3_proc_setacls(inode, acl, dfacl);
-	posix_acl_release(alloc);
+out:
+	if (acl != orig)
+		posix_acl_release(acl);
+	if (dfacl != orig)
+		posix_acl_release(dfacl);
 	return status;
 
 fail:
-	return PTR_ERR(alloc);
+	status = PTR_ERR(alloc);
+	goto out;
 }
 
 const struct xattr_handler *nfs3_xattr_handlers[] = {

base-commit: ae83d0b416db002fe95601e7f97f64b59514d936
-- 
2.25.3

Re: [PATCH] nfs: Fix potential posix_acl refcnt leak in nfs3_set_acl

[Trond Myklebust]

On Mon, 2020-04-20 at 15:51 +0200, Andreas Gruenbacher wrote:
> nfs3_set_acl keeps track of the acl it allocated locally to determine
> if an acl
> needs to be released at the end.  This results in a memory leak when
> the
> function allocates an acl as well as a default acl.  Fix by releasing
> acls
> that differ from the acl originally passed into nfs3_set_acl.
> 
> Fixes: b7fa0554cf1b ("[PATCH] NFS: Add support for NFSv3 ACLs")
> Reported-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/nfs/nfs3acl.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> index c5c3fc6e6c60..26c94b32d6f4 100644
> --- a/fs/nfs/nfs3acl.c
> +++ b/fs/nfs/nfs3acl.c
> @@ -253,37 +253,45 @@ int nfs3_proc_setacls(struct inode *inode,
> struct posix_acl *acl,
>  
>  int nfs3_set_acl(struct inode *inode, struct posix_acl *acl, int
> type)
>  {
> -	struct posix_acl *alloc = NULL, *dfacl = NULL;
> +	struct posix_acl *orig = acl, *dfacl = NULL, *alloc;
>  	int status;
>  
>  	if (S_ISDIR(inode->i_mode)) {
>  		switch(type) {
>  		case ACL_TYPE_ACCESS:
> -			alloc = dfacl = get_acl(inode,
> ACL_TYPE_DEFAULT);
> +			alloc = get_acl(inode, ACL_TYPE_DEFAULT);
>  			if (IS_ERR(alloc))
>  				goto fail;
> +			dfacl = alloc;
>  			break;
>  
>  		case ACL_TYPE_DEFAULT:
> -			dfacl = acl;
> -			alloc = acl = get_acl(inode, ACL_TYPE_ACCESS);
> +			alloc = get_acl(inode, ACL_TYPE_ACCESS);
>  			if (IS_ERR(alloc))
>  				goto fail;
> +			dfacl = acl;
> +			acl = alloc;
>  			break;
>  		}
>  	}
>  
>  	if (acl == NULL) {
> -		alloc = acl = posix_acl_from_mode(inode->i_mode,
> GFP_KERNEL);
> +		alloc = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
>  		if (IS_ERR(alloc))
>  			goto fail;
> +		acl = alloc;
>  	}
>  	status = __nfs3_proc_setacls(inode, acl, dfacl);
> -	posix_acl_release(alloc);
> +out:
> +	if (acl != orig)
> +		posix_acl_release(acl);
> +	if (dfacl != orig)
> +		posix_acl_release(dfacl);
>  	return status;
>  
>  fail:
> -	return PTR_ERR(alloc);
> +	status = PTR_ERR(alloc);
> +	goto out;
>  }
>  
>  const struct xattr_handler *nfs3_xattr_handlers[] = {
> 
> base-commit: ae83d0b416db002fe95601e7f97f64b59514d936

Thanks Andreas! This one looks good.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH] nfs: Fix potential posix_acl refcnt leak in nfs3_set_acl

[Trond Myklebust]

On Mon, 2020-04-20 at 15:04 +0200, Andreas Gruenbacher wrote:
> On Mon, Apr 20, 2020 at 2:59 PM Trond Myklebust <
> trondmy@hammerspace.com> wrote:
> > Well, that should Oops when either IS_ERR(acl) or IS_ERR(dfacl)
> > triggers, shouldn't it?
> 
> Yes, checks missings. But you get the idea.
> 

Fair enough 🙂

Can I expect an official patch from either you or the original
submitters?

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH] nfs: Fix potential posix_acl refcnt leak in nfs3_set_acl

[Andreas Gruenbacher]

On Mon, Apr 20, 2020 at 2:59 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
> Well, that should Oops when either IS_ERR(acl) or IS_ERR(dfacl)
> triggers, shouldn't it?

Yes, checks missings. But you get the idea.

Thanks,
Andreas

Re: [PATCH] nfs: Fix potential posix_acl refcnt leak in nfs3_set_acl

[Trond Myklebust]

On Mon, 2020-04-20 at 14:51 +0200, Andreas Gruenbacher wrote:
> Am Mo., 20. Apr. 2020 um 14:15 Uhr schrieb Trond Myklebust <
> trondmy@hammerspace.com>:
> > I don't really see any alternative to adding a 'dfalloc' to track
> > the
> > allocated dfacl separately.
> 
> Something like the attached patch should work as well.
> 
> Thanks,
> Andreas
> 
> ---
>  fs/nfs/nfs3acl.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> index c5c3fc6e6c60..f1581f11c220 100644
> --- a/fs/nfs/nfs3acl.c
> +++ b/fs/nfs/nfs3acl.c
> @@ -253,37 +253,41 @@ int nfs3_proc_setacls(struct inode *inode,
> struct posix_acl *acl,
>  
>  int nfs3_set_acl(struct inode *inode, struct posix_acl *acl, int
> type)
>  {
> -	struct posix_acl *alloc = NULL, *dfacl = NULL;
> +	struct posix_acl *orig = acl, *dfacl = NULL;
>  	int status;
>  
>  	if (S_ISDIR(inode->i_mode)) {
>  		switch(type) {
>  		case ACL_TYPE_ACCESS:
> -			alloc = dfacl = get_acl(inode,
> ACL_TYPE_DEFAULT);
> -			if (IS_ERR(alloc))
> -				goto fail;
> +			dfacl = get_acl(inode, ACL_TYPE_DEFAULT);
> +			status = PTR_ERR(dfacl);
> +			if (IS_ERR(dfacl))
> +				goto out;
>  			break;
>  
>  		case ACL_TYPE_DEFAULT:
>  			dfacl = acl;
> -			alloc = acl = get_acl(inode, ACL_TYPE_ACCESS);
> -			if (IS_ERR(alloc))
> -				goto fail;
> +			acl = get_acl(inode, ACL_TYPE_ACCESS);
> +			status = PTR_ERR(acl);
> +			if (IS_ERR(acl))
> +				goto out;
>  			break;
>  		}
>  	}
>  
>  	if (acl == NULL) {
> -		alloc = acl = posix_acl_from_mode(inode->i_mode,
> GFP_KERNEL);
> -		if (IS_ERR(alloc))
> -			goto fail;
> +		acl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
> +		status = PTR_ERR(acl);
> +		if (IS_ERR(acl))
> +			goto out;
>  	}
>  	status = __nfs3_proc_setacls(inode, acl, dfacl);
> -	posix_acl_release(alloc);
> +out:
> +	if (acl != orig)
> +		posix_acl_release(acl);
> +	if (dfacl != orig)
> +		posix_acl_release(dfacl);
>  	return status;
> -
> -fail:
> -	return PTR_ERR(alloc);
>  }
>  
>  const struct xattr_handler *nfs3_xattr_handlers[] = {
> 
> base-commit: ae83d0b416db002fe95601e7f97f64b59514d936

Well, that should Oops when either IS_ERR(acl) or IS_ERR(dfacl)
triggers, shouldn't it? 🙂

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH] nfs: Fix potential posix_acl refcnt leak in nfs3_set_acl

[Andreas Gruenbacher]

Am Mo., 20. Apr. 2020 um 14:15 Uhr schrieb Trond Myklebust <trondmy@hammerspace.com>:
> I don't really see any alternative to adding a 'dfalloc' to track the
> allocated dfacl separately.

Something like the attached patch should work as well.

Thanks,
Andreas

---
 fs/nfs/nfs3acl.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
index c5c3fc6e6c60..f1581f11c220 100644
--- a/fs/nfs/nfs3acl.c
+++ b/fs/nfs/nfs3acl.c
@@ -253,37 +253,41 @@ int nfs3_proc_setacls(struct inode *inode, struct posix_acl *acl,
 
 int nfs3_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
-	struct posix_acl *alloc = NULL, *dfacl = NULL;
+	struct posix_acl *orig = acl, *dfacl = NULL;
 	int status;
 
 	if (S_ISDIR(inode->i_mode)) {
 		switch(type) {
 		case ACL_TYPE_ACCESS:
-			alloc = dfacl = get_acl(inode, ACL_TYPE_DEFAULT);
-			if (IS_ERR(alloc))
-				goto fail;
+			dfacl = get_acl(inode, ACL_TYPE_DEFAULT);
+			status = PTR_ERR(dfacl);
+			if (IS_ERR(dfacl))
+				goto out;
 			break;
 
 		case ACL_TYPE_DEFAULT:
 			dfacl = acl;
-			alloc = acl = get_acl(inode, ACL_TYPE_ACCESS);
-			if (IS_ERR(alloc))
-				goto fail;
+			acl = get_acl(inode, ACL_TYPE_ACCESS);
+			status = PTR_ERR(acl);
+			if (IS_ERR(acl))
+				goto out;
 			break;
 		}
 	}
 
 	if (acl == NULL) {
-		alloc = acl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
-		if (IS_ERR(alloc))
-			goto fail;
+		acl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
+		status = PTR_ERR(acl);
+		if (IS_ERR(acl))
+			goto out;
 	}
 	status = __nfs3_proc_setacls(inode, acl, dfacl);
-	posix_acl_release(alloc);
+out:
+	if (acl != orig)
+		posix_acl_release(acl);
+	if (dfacl != orig)
+		posix_acl_release(dfacl);
 	return status;
-
-fail:
-	return PTR_ERR(alloc);
 }
 
 const struct xattr_handler *nfs3_xattr_handlers[] = {

base-commit: ae83d0b416db002fe95601e7f97f64b59514d936
-- 
2.25.3

Re: [PATCH] nfs: Fix potential posix_acl refcnt leak in nfs3_set_acl

[Trond Myklebust]

On Mon, 2020-04-20 at 13:43 +0800, Xiyu Yang wrote:
> nfs3_set_acl() invokes get_acl(), which returns a local reference of
> the
> posix_acl object to "alloc" with increased refcount.
> 
> When nfs3_set_acl() returns or a new object is assigned to
> "alloc",  the
> original local reference of  "alloc" becomes invalid, so the refcount
> should be decreased to keep refcount balanced.
> 
> The reference counting issue happens in one path of nfs3_set_acl().
> When
> "acl" equals to NULL but "alloc" is not NULL, the function forgets to
> decrease the refcnt increased by get_acl() and causes a refcnt leak.
> 
> Fix this issue by calling posix_acl_release() on this path when
> "alloc"
> is not NULL.
> 
> Fixes: b7fa0554cf1b ("[PATCH] NFS: Add support for NFSv3 ACLs")
> Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
> Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
> ---
>  fs/nfs/nfs3acl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> index c5c3fc6e6c60..b5c41bcca8cf 100644
> --- a/fs/nfs/nfs3acl.c
> +++ b/fs/nfs/nfs3acl.c
> @@ -274,6 +274,8 @@ int nfs3_set_acl(struct inode *inode, struct
> posix_acl *acl, int type)
>  	}
>  
>  	if (acl == NULL) {
> +		if (alloc)
> +			posix_acl_release(alloc);

This will result in 'dfacl' being freed while it is still in use, so
can't be correct either.

>  		alloc = acl = posix_acl_from_mode(inode->i_mode,
> GFP_KERNEL);
>  		if (IS_ERR(alloc))
>  			goto fail;

I don't really see any alternative to adding a 'dfalloc' to track the
allocated dfacl separately.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

[PATCH] nfs: Fix potential posix_acl refcnt leak in nfs3_set_acl

[Xiyu Yang]

nfs3_set_acl() invokes get_acl(), which returns a local reference of the
posix_acl object to "alloc" with increased refcount.

When nfs3_set_acl() returns or a new object is assigned to "alloc",  the
original local reference of  "alloc" becomes invalid, so the refcount
should be decreased to keep refcount balanced.

The reference counting issue happens in one path of nfs3_set_acl(). When
"acl" equals to NULL but "alloc" is not NULL, the function forgets to
decrease the refcnt increased by get_acl() and causes a refcnt leak.

Fix this issue by calling posix_acl_release() on this path when "alloc"
is not NULL.

Fixes: b7fa0554cf1b ("[PATCH] NFS: Add support for NFSv3 ACLs")
Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
---
 fs/nfs/nfs3acl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
index c5c3fc6e6c60..b5c41bcca8cf 100644
--- a/fs/nfs/nfs3acl.c
+++ b/fs/nfs/nfs3acl.c
@@ -274,6 +274,8 @@ int nfs3_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	}
 
 	if (acl == NULL) {
+		if (alloc)
+			posix_acl_release(alloc);
 		alloc = acl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
 		if (IS_ERR(alloc))
 			goto fail;
-- 
2.7.4