[BUG] With size > XATTR_SIZE_MAX, getxattr(2) always returns E2BIG

[BUG] With size > XATTR_SIZE_MAX, getxattr(2) always returns E2BIG

[Andreas Gruenbacher]

[-- Attachment #1.1: Type: text/plain, Size: 205 bytes --]

Hello,

here is a fix for the getxattr and listxattr syscall. Explanation in the
patch. Could you please apply? Thanks.

Regards,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX AG

[-- Attachment #1.2: big-xattr.diff --]
[-- Type: text/plain, Size: 5027 bytes --]

The getxattr (listxattr) syscall returns E2BIG if the buffer
passed to them is bigger than XATTR_SIZE_MAX (XATTR_LIST_MAX),
no matter what buffer size is actually required. Here is a
fix. It also removes the xattr_alloc and xattr_free functions
which are not of much use anymore.

Index: linux-2.6.1/fs/xattr.c
===================================================================
--- linux-2.6.1.orig/fs/xattr.c
+++ linux-2.6.1/fs/xattr.c
@@ -16,40 +16,6 @@
 #include <asm/uaccess.h>
 
 /*
- * Extended attribute memory allocation wrappers, originally
- * based on the Intermezzo PRESTO_ALLOC/PRESTO_FREE macros.
- * Values larger than a page are uncommon - extended attributes
- * are supposed to be small chunks of metadata, and it is quite
- * unusual to have very many extended attributes, so lists tend
- * to be quite short as well.  The 64K upper limit is derived
- * from the extended attribute size limit used by XFS.
- * Intentionally allow zero @size for value/list size requests.
- */
-static void *
-xattr_alloc(size_t size, size_t limit)
-{
-	void *ptr;
-
-	if (size > limit)
-		return ERR_PTR(-E2BIG);
-
-	if (!size)	/* size request, no buffer is needed */
-		return NULL;
-
-	ptr = kmalloc((unsigned long) size, GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
-	return ptr;
-}
-
-static void
-xattr_free(void *ptr, size_t size)
-{
-	if (size)	/* for a size request, no buffer was needed */
-		kfree(ptr);
-}
-
-/*
  * Extended attribute SET operations
  */
 static long
@@ -57,7 +23,7 @@ setxattr(struct dentry *d, char __user *
 	 size_t size, int flags)
 {
 	int error;
-	void *kvalue;
+	void *kvalue = NULL;
 	char kname[XATTR_NAME_MAX + 1];
 
 	if (flags & ~(XATTR_CREATE|XATTR_REPLACE))
@@ -69,13 +35,16 @@ setxattr(struct dentry *d, char __user *
 	if (error < 0)
 		return error;
 
-	kvalue = xattr_alloc(size, XATTR_SIZE_MAX);
-	if (IS_ERR(kvalue))
-		return PTR_ERR(kvalue);
-
-	if (size > 0 && copy_from_user(kvalue, value, size)) {
-		xattr_free(kvalue, size);
-		return -EFAULT;
+	if (size) {
+		if (size > XATTR_SIZE_MAX)
+			return -E2BIG;
+		kvalue = kmalloc(size, GFP_KERNEL);
+		if (!kvalue)
+			return -ENOMEM;
+		if (copy_from_user(kvalue, value, size)) {
+			kfree(kvalue);
+			return -EFAULT;
+		}
 	}
 
 	error = -EOPNOTSUPP;
@@ -90,7 +59,8 @@ setxattr(struct dentry *d, char __user *
 out:
 		up(&d->d_inode->i_sem);
 	}
-	xattr_free(kvalue, size);
+	if (kvalue)
+		kfree(kvalue);
 	return error;
 }
 
@@ -146,7 +116,7 @@ static ssize_t
 getxattr(struct dentry *d, char __user *name, void __user *value, size_t size)
 {
 	ssize_t error;
-	void *kvalue;
+	void *kvalue = NULL;
 	char kname[XATTR_NAME_MAX + 1];
 
 	error = strncpy_from_user(kname, name, sizeof(kname));
@@ -155,9 +125,13 @@ getxattr(struct dentry *d, char __user *
 	if (error < 0)
 		return error;
 
-	kvalue = xattr_alloc(size, XATTR_SIZE_MAX);
-	if (IS_ERR(kvalue))
-		return PTR_ERR(kvalue);
+	if (size) {
+		if (size > XATTR_SIZE_MAX)
+			size = XATTR_SIZE_MAX;
+		kvalue = kmalloc(size, GFP_KERNEL);
+		if (!kvalue)
+			return -ENOMEM;
+	}
 
 	error = -EOPNOTSUPP;
 	if (d->d_inode->i_op && d->d_inode->i_op->getxattr) {
@@ -165,13 +139,18 @@ getxattr(struct dentry *d, char __user *
 		if (error)
 			goto out;
 		error = d->d_inode->i_op->getxattr(d, kname, kvalue, size);
+		if (error > 0) {
+			if (copy_to_user(value, kvalue, error))
+				error = -EFAULT;
+		} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
+			/* The file system tried to returned a value bigger
+			   than XATTR_SIZE_MAX bytes. Not possible. */
+			error = -E2BIG;
+		}
 	}
-
-	if (kvalue && error > 0)
-		if (copy_to_user(value, kvalue, error))
-			error = -EFAULT;
 out:
-	xattr_free(kvalue, size);
+	if (kvalue)
+		kfree(kvalue);
 	return error;
 }
 
@@ -226,11 +205,15 @@ static ssize_t
 listxattr(struct dentry *d, char __user *list, size_t size)
 {
 	ssize_t error;
-	char *klist;
+	char *klist = NULL;
 
-	klist = (char *)xattr_alloc(size, XATTR_LIST_MAX);
-	if (IS_ERR(klist))
-		return PTR_ERR(klist);
+	if (size) {
+		if (size > XATTR_LIST_MAX)
+			size = XATTR_LIST_MAX;
+		klist = kmalloc(size, GFP_KERNEL);
+		if (!klist)
+			return -ENOMEM;
+	}
 
 	error = -EOPNOTSUPP;
 	if (d->d_inode->i_op && d->d_inode->i_op->listxattr) {
@@ -238,13 +221,18 @@ listxattr(struct dentry *d, char __user 
 		if (error)
 			goto out;
 		error = d->d_inode->i_op->listxattr(d, klist, size);
+		if (error > 0) {
+			if (copy_to_user(list, klist, error))
+				error = -EFAULT;
+		} else if (error == -ERANGE && size >= XATTR_LIST_MAX) {
+			/* The file system tried to returned a list bigger
+			   than XATTR_LIST_MAX bytes. Not possible. */
+			error = -E2BIG;
+		}
 	}
-
-	if (klist && error > 0)
-		if (copy_to_user(list, klist, error))
-			error = -EFAULT;
 out:
-	xattr_free(klist, size);
+	if (klist)
+		kfree(klist);
 	return error;
 }
 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [BUG] With size > XATTR_SIZE_MAX, getxattr(2) always returns E2BIG

[Nathan Scott]

On Tue, Feb 03, 2004 at 01:52:19PM +0100, Andreas Gruenbacher wrote:
> Hello,
> 
> here is a fix for the getxattr and listxattr syscall. Explanation in the
> patch. Could you please apply? Thanks.

Our regression tests tripped a couple of problems with this;
here's a patch on top of yours (which is now 2.6.3-rc1).

cheers.

-- 
Nathan


--- /usr/tmp/TmpDir.13927-0/fs/xattr.c_1.2	2004-02-09 11:42:31.000000000 +1100
+++ fs/xattr.c	2004-02-09 11:37:39.747592560 +1100
@@ -139,7 +139,7 @@
 		if (error)
 			goto out;
 		error = d->d_inode->i_op->getxattr(d, kname, kvalue, size);
-		if (error > 0) {
+		if (size && error > 0) {
 			if (copy_to_user(value, kvalue, error))
 				error = -EFAULT;
 		} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
@@ -221,7 +221,7 @@
 		if (error)
 			goto out;
 		error = d->d_inode->i_op->listxattr(d, klist, size);
-		if (error > 0) {
+		if (size && error > 0) {
 			if (copy_to_user(list, klist, error))
 				error = -EFAULT;
 		} else if (error == -ERANGE && size >= XATTR_LIST_MAX) {

Re: [BUG] With size > XATTR_SIZE_MAX, getxattr(2) always returns E2BIG

[Andreas Gruenbacher]

On Mon, 2004-02-09 at 02:26, Nathan Scott wrote:
> On Tue, Feb 03, 2004 at 01:52:19PM +0100, Andreas Gruenbacher wrote:
> > Hello,
> > 
> > here is a fix for the getxattr and listxattr syscall. Explanation in the
> > patch. Could you please apply? Thanks.
> 
> Our regression tests tripped a couple of problems with this;
> here's a patch on top of yours (which is now 2.6.3-rc1).

Indeed, I got that case wrong. The patch looks correct. Thanks!

I would move the missing test right before the copy_to_user as below,
which is slightly better.


Index: linux-2.6.3-rc1.orig/fs/xattr.c
===================================================================
--- linux-2.6.3-rc1.orig.orig/fs/xattr.c
+++ linux-2.6.3-rc1.orig/fs/xattr.c
@@ -140,7 +140,7 @@ getxattr(struct dentry *d, char __user *
 			goto out;
 		error = d->d_inode->i_op->getxattr(d, kname, kvalue, size);
 		if (error > 0) {
-			if (copy_to_user(value, kvalue, error))
+			if (size && copy_to_user(value, kvalue, error))
 				error = -EFAULT;
 		} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
 			/* The file system tried to returned a value bigger
@@ -222,7 +222,7 @@ listxattr(struct dentry *d, char __user 
 			goto out;
 		error = d->d_inode->i_op->listxattr(d, klist, size);
 		if (error > 0) {
-			if (copy_to_user(list, klist, error))
+			if (size && copy_to_user(list, klist, error))
 				error = -EFAULT;
 		} else if (error == -ERANGE && size >= XATTR_LIST_MAX) {
 			/* The file system tried to returned a list bigger

> cheers.
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX AG