[PATCH] nfsd: Fix memory leak in nfsd_getxattr (& usage in _get_posix_acl)

[Krishna Kumar <krkumar2@xxxxxxxxxx>]

"J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote on 10/21/2008 02:32:59 AM:

> Is the current code really a problem?
> 
> Possibly more interesting to look into:
> 
>    - Can nfsd_getxattr actually return NULL?

You are right. I think it can return 0, eg any of the filesystem getxattr's.

>    - Why do we need to return ENODATA in that case?
>      (nfsd_get_posix_acl, for example, seems to just end up
>      returning -EINVAL in that case.)

Did you mean NULL rather than EINVAL (posix_acl_from_xattr returns NULL before
EINVAL)?

Another thing that seems slightly wrong. Both of the following functions:
nfsd_get_posix_acl()
{
	...
	size = nfsd_getxattr(fhp->fh_dentry, name, &value);
	if (size < 0)
		return ERR_PTR(size);
	...
}
_get_posix_acl(struct dentry *dentry, char *key)
{
	...
	buflen = nfsd_getxattr(dentry, key, &buf);
	if (!buflen)
		buflen = -ENODATA;
	if (buflen <= 0)
		return ERR_PTR(buflen);
	...
}
call nfsd_getxattr which can allocate memory but still return < 0. Function
allocates buf and calls vfs_getxattr() which fails and leaks memory. The best
place to fix this is in nfsd_getxattr in case vfs_getxattr() returns < 0

Does that sound correct? Patch for this follows...

thanks,

- KK

From: Krishna Kumar <krkumar2@xxxxxxxxxx>

1. Fix a memory leak in nfsd_getxattr. nfsd_getxattr should free up memory
	that it allocated if vfs_getxattr fails.

2. Change _get_posix_acl to return NULL on buflen == 0.

Signed-off-by: Krishna Kumar <krkumar2@xxxxxxxxxx>
---

 fs/nfsd/vfs.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff -ruNp org/fs/nfsd/vfs.c new/fs/nfsd/vfs.c
--- org/fs/nfsd/vfs.c	2008-10-21 16:05:41.000000000 +0530
+++ new/fs/nfsd/vfs.c	2008-10-21 15:57:30.000000000 +0530
@@ -411,6 +411,7 @@ out_nfserr:
 static ssize_t nfsd_getxattr(struct dentry *dentry, char *key, void **buf)
 {
 	ssize_t buflen;
+	ssize_t ret;
 
 	buflen = vfs_getxattr(dentry, key, NULL, 0);
 	if (buflen <= 0)
@@ -420,7 +421,10 @@ static ssize_t nfsd_getxattr(struct dent
 	if (!*buf)
 		return -ENOMEM;
 
-	return vfs_getxattr(dentry, key, *buf, buflen);
+	ret = vfs_getxattr(dentry, key, *buf, buflen);
+	if (ret < 0)
+		kfree(*buf);
+	return ret;
 }
 #endif
 
@@ -499,13 +503,11 @@ static struct posix_acl *
 _get_posix_acl(struct dentry *dentry, char *key)
 {
 	void *buf = NULL;
-	struct posix_acl *pacl = NULL;
+	struct posix_acl *pacl;
 	int buflen;
 
 	buflen = nfsd_getxattr(dentry, key, &buf);
-	if (!buflen)
-		buflen = -ENODATA;
-	if (buflen <= 0)
+	if (buflen < 0)
 		return ERR_PTR(buflen);
 
 	pacl = posix_acl_from_xattr(buf, buflen);
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html