RE: [PATCH] backout the xattr override access checks flag

RE: [PATCH] backout the xattr override access checks flag

[Luka Renko]

On Friday, February 21, 2003 16:59 Christoph Hellwig wrote:
> On Fri, Feb 21, 2003 at 01:50:33PM +0100, Luka Renko wrote:
> > I would agree with Andreas that temporarily raising 
> capabilities for 
> > process by kernel module is probably not a good thing. I 
> think having 
> > the flag (that cannot be passed from user space) is better.
> 
> So could you please explain why this is better in your eyes? 

I have concerns with kernel module temporarly changing capabilities of a
user process, however I am not sure if this is really the problem. I was
thinking in terms of SMP/preempt (EA calls can/will go to sleep) and
considering that this might be a security problem, however since we are
changing it per process it should probably be OK.
If just EA calls have mode to allow "kernel context" access it for sure does
not have as wide impact as temporary change of process capabilities... But I
agree with you that it is "yet-another-special-case"...

Regards,
Luka

Re: [PATCH] backout the xattr override access checks flag

[Christoph Hellwig]

On Sun, Feb 23, 2003 at 08:28:23PM +0100, Luka Renko wrote:
> I have concerns with kernel module temporarly changing capabilities of a
> user process, however I am not sure if this is really the problem. I was
> thinking in terms of SMP/preempt (EA calls can/will go to sleep) and
> considering that this might be a security problem, however since we are
> changing it per process it should probably be OK.

Right.  And we're already doing similar things, see sys_access().

Re: [PATCH] backout the xattr override access checks flag

[Andreas Gruenbacher]

Hello,

On Sunday 23 February 2003 20:36, Christoph Hellwig wrote:
> On Sun, Feb 23, 2003 at 08:28:23PM +0100, Luka Renko wrote:
> > I have concerns with kernel module temporarly changing capabilities of a
> > user process, however I am not sure if this is really the problem. I was
> > thinking in terms of SMP/preempt (EA calls can/will go to sleep) and
> > considering that this might be a security problem, however since we are
> > changing it per process it should probably be OK.
>
> Right.  And we're already doing similar things, see sys_access().

Okay, this seems sufficient confirmation that it is not dangerous to 
temporarily raise capabilities in a task, so we can get rid of the 
XATTR_KERNEL_CONTEXT flag. Instead, the HSM module should raise a capability 
before invoking the xattr inode operations.

-- Andreas.

RE: [PATCH] backout the xattr override access checks flag

[Luka Renko]

On Friday, February 21, 2003 11:20 Andreas Gruenbacher wrote:
> > This check backs out the XATTR_KERNEL_CONTEXT that tells 
> filesystems 
> > to ignore the lack of capabilities of the caller that went 
> in in the 
> > last week (through akpm..).
> >
> > It's a cludgy interface design to have flags that change access 
> > checks, and there's a much easier way by raising the 
> capabilities in 
> > the process that actually needs this in kernel mode
> 
> Thanks, I'm not fully convinced that raising capabilities 
> temporarily in the 
> kernel is a good thing to do, but it surely obsoletes the 
> XATTR_KERNEL_CONTEXT flag.

I would agree with Andreas that temporarily raising capabilities for process
by kernel module is probably not a good thing. I think having the flag (that
cannot be passed from user space) is better.

> 
> > (not that such code even exists yet).
> 
> There is an HSM project for which this feature has been 
> added. I think they 
> are using a loadable module.

Any HSM kernel module is potential user of this. The project I am working is
currently doing capability/permission modifications to allow writing EAs in
some cases.

This "feature" was discussed some time ago on acl-devel mailing list:
http://acl.bestbits.at/pipermail/acl-devel/2002-November/001263.html

There is also related discussion about trusted attributes:
http://acl.bestbits.at/pipermail/acl-devel/2002-November/001266.html
http://acl.bestbits.at/pipermail/acl-devel/2002-November/001286.html

Regards,
Luka

Re: [PATCH] backout the xattr override access checks flag

[Christoph Hellwig]

On Fri, Feb 21, 2003 at 01:50:33PM +0100, Luka Renko wrote:
> I would agree with Andreas that temporarily raising capabilities for process
> by kernel module is probably not a good thing. I think having the flag (that
> cannot be passed from user space) is better.

So could you please explain why this is better in your eyes? 

> Any HSM kernel module is potential user of this. The project I am working is
> currently doing capability/permission modifications to allow writing EAs in
> some cases.

SGI's dmapi implementation doesn't need it.. (it uses the proper credentials
passing in XFS instead, something we should have at the VFS level but
unfortunately 2.5 once again didn't get this)

[PATCH] backout the xattr override access checks flag

[Christoph Hellwig]

This check backs out the XATTR_KERNEL_CONTEXT that tells filesystems
to ignore the lack of capabilities of the caller that went in in the
last week (through akpm..).

It's a cludgy interface design to have flags that change access checks,
and there's a much easier way by raising the capabilities in the process
that actually needs this in kernel mode (not that such code even exists
yet).

Something even better would probably be to move out the xattr access checks
to common code..


--- 1.12/fs/xattr.c	Sat Feb 15 03:24:02 2003
+++ edited/fs/xattr.c	Thu Feb 20 19:27:15 2003
@@ -160,7 +160,7 @@
 		if (error)
 			goto out;
 		down(&d->d_inode->i_sem);
-		error = d->d_inode->i_op->getxattr(d, kname, kvalue, size, 0);
+		error = d->d_inode->i_op->getxattr(d, kname, kvalue, size);
 		up(&d->d_inode->i_sem);
 	}
 
@@ -233,7 +233,7 @@
 		if (error)
 			goto out;
 		down(&d->d_inode->i_sem);
-		error = d->d_inode->i_op->listxattr(d, klist, size, 0);
+		error = d->d_inode->i_op->listxattr(d, klist, size);
 		up(&d->d_inode->i_sem);
 	}
 
@@ -308,7 +308,7 @@
 		if (error)
 			goto out;
 		down(&d->d_inode->i_sem);
-		error = d->d_inode->i_op->removexattr(d, kname, 0);
+		error = d->d_inode->i_op->removexattr(d, kname);
 		up(&d->d_inode->i_sem);
 	}
 out:
--- 1.4/fs/ext2/acl.c	Sat Feb 15 03:24:02 2003
+++ edited/fs/ext2/acl.c	Thu Feb 20 19:27:14 2003
@@ -419,7 +419,7 @@
  */
 static size_t
 ext2_xattr_list_acl_access(char *list, struct inode *inode,
-			   const char *name, int name_len, int flags)
+			   const char *name, int name_len)
 {
 	const size_t size = sizeof(XATTR_NAME_ACL_ACCESS);
 
@@ -432,7 +432,7 @@
 
 static size_t
 ext2_xattr_list_acl_default(char *list, struct inode *inode,
-			    const char *name, int name_len, int flags)
+			    const char *name, int name_len)
 {
 	const size_t size = sizeof(XATTR_NAME_ACL_DEFAULT);
 
@@ -465,7 +465,7 @@
 
 static int
 ext2_xattr_get_acl_access(struct inode *inode, const char *name,
-			  void *buffer, size_t size, int flags)
+			  void *buffer, size_t size)
 {
 	if (strcmp(name, "") != 0)
 		return -EINVAL;
@@ -474,7 +474,7 @@
 
 static int
 ext2_xattr_get_acl_default(struct inode *inode, const char *name,
-			   void *buffer, size_t size, int flags)
+			   void *buffer, size_t size)
 {
 	if (strcmp(name, "") != 0)
 		return -EINVAL;
@@ -482,8 +482,7 @@
 }
 
 static int
-ext2_xattr_set_acl(struct inode *inode, int type, const void *value,
-		   size_t size)
+ext2_xattr_set_acl(struct inode *inode, int type, const void *value, size_t size)
 {
 	struct posix_acl *acl;
 	int error;
===== fs/ext2/xattr.c 1.8 vs edited =====
--- 1.8/fs/ext2/xattr.c	Sat Feb 15 03:24:04 2003
+++ edited/fs/ext2/xattr.c	Thu Feb 20 19:27:15 2003
@@ -199,7 +199,7 @@
  */
 ssize_t
 ext2_getxattr(struct dentry *dentry, const char *name,
-	      void *buffer, size_t size, int flags)
+	      void *buffer, size_t size)
 {
 	struct ext2_xattr_handler *handler;
 	struct inode *inode = dentry->d_inode;
@@ -207,7 +207,7 @@
 	handler = ext2_xattr_resolve_name(&name);
 	if (!handler)
 		return -EOPNOTSUPP;
-	return handler->get(inode, name, buffer, size, flags);
+	return handler->get(inode, name, buffer, size);
 }
 
 /*
@@ -217,9 +217,9 @@
  * BKL held [before 2.5.x]
  */
 ssize_t
-ext2_listxattr(struct dentry *dentry, char *buffer, size_t size, int flags)
+ext2_listxattr(struct dentry *dentry, char *buffer, size_t size)
 {
-	return ext2_xattr_list(dentry->d_inode, buffer, size, flags);
+	return ext2_xattr_list(dentry->d_inode, buffer, size);
 }
 
 /*
@@ -250,7 +250,7 @@
  * BKL held [before 2.5.x]
  */
 int
-ext2_removexattr(struct dentry *dentry, const char *name, int flags)
+ext2_removexattr(struct dentry *dentry, const char *name)
 {
 	struct ext2_xattr_handler *handler;
 	struct inode *inode = dentry->d_inode;
@@ -258,7 +258,7 @@
 	handler = ext2_xattr_resolve_name(&name);
 	if (!handler)
 		return -EOPNOTSUPP;
-	return handler->set(inode, name, NULL, 0, flags | XATTR_REPLACE);
+	return handler->set(inode, name, NULL, 0, XATTR_REPLACE);
 }
 
 /*
@@ -371,8 +371,7 @@
  * used / required on success.
  */
 int
-ext2_xattr_list(struct inode *inode, char *buffer, size_t buffer_size,
-		int flags)
+ext2_xattr_list(struct inode *inode, char *buffer, size_t buffer_size)
 {
 	struct buffer_head *bh = NULL;
 	struct ext2_xattr_entry *entry;
@@ -412,7 +411,7 @@
 		handler = ext2_xattr_handler(entry->e_name_index);
 		if (handler)
 			size += handler->list(NULL, inode, entry->e_name,
-					      entry->e_name_len, flags);
+					      entry->e_name_len);
 	}
 
 	if (ext2_xattr_cache_insert(bh))
@@ -435,7 +434,7 @@
 		handler = ext2_xattr_handler(entry->e_name_index);
 		if (handler)
 			buf += handler->list(buf, inode, entry->e_name,
-					     entry->e_name_len, flags);
+					     entry->e_name_len);
 	}
 	error = size;
 
===== fs/ext2/xattr.h 1.4 vs edited =====
--- 1.4/fs/ext2/xattr.h	Sat Feb 15 03:24:04 2003
+++ edited/fs/ext2/xattr.h	Thu Feb 20 19:27:15 2003
@@ -58,9 +58,9 @@
 struct ext2_xattr_handler {
 	char *prefix;
 	size_t (*list)(char *list, struct inode *inode, const char *name,
-		       int name_len, int flags);
+		       int name_len);
 	int (*get)(struct inode *inode, const char *name, void *buffer,
-		   size_t size, int flags);
+		   size_t size);
 	int (*set)(struct inode *inode, const char *name, const void *buffer,
 		   size_t size, int flags);
 };
@@ -69,12 +69,12 @@
 extern void ext2_xattr_unregister(int, struct ext2_xattr_handler *);
 
 extern int ext2_setxattr(struct dentry *, const char *, const void *, size_t, int);
-extern ssize_t ext2_getxattr(struct dentry *, const char *, void *, size_t, int);
-extern ssize_t ext2_listxattr(struct dentry *, char *, size_t, int);
-extern int ext2_removexattr(struct dentry *, const char *, int);
+extern ssize_t ext2_getxattr(struct dentry *, const char *, void *, size_t);
+extern ssize_t ext2_listxattr(struct dentry *, char *, size_t);
+extern int ext2_removexattr(struct dentry *, const char *);
 
 extern int ext2_xattr_get(struct inode *, int, const char *, void *, size_t);
-extern int ext2_xattr_list(struct inode *, char *, size_t, int flags);
+extern int ext2_xattr_list(struct inode *, char *, size_t);
 extern int ext2_xattr_set(struct inode *, int, const char *, const void *, size_t, int);
 
 extern void ext2_xattr_delete_inode(struct inode *);
===== fs/ext2/xattr_trusted.c 1.1 vs edited =====
--- 1.1/fs/ext2/xattr_trusted.c	Sat Feb 15 03:24:04 2003
+++ edited/fs/ext2/xattr_trusted.c	Thu Feb 20 19:31:21 2003
@@ -20,7 +20,7 @@
 {
 	const int prefix_len = sizeof(XATTR_TRUSTED_PREFIX)-1;
 
-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))
 		return 0;
 
 	if (list) {
@@ -37,7 +37,7 @@
 {
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return ext2_xattr_get(inode, EXT2_XATTR_INDEX_TRUSTED, name,
 			      buffer, size);
@@ -49,7 +49,7 @@
 {
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return ext2_xattr_set(inode, EXT2_XATTR_INDEX_TRUSTED, name,
 			      value, size, flags);
===== fs/ext2/xattr_user.c 1.5 vs edited =====
--- 1.5/fs/ext2/xattr_user.c	Sat Feb 15 03:24:03 2003
+++ edited/fs/ext2/xattr_user.c	Thu Feb 20 19:27:15 2003
@@ -19,12 +19,11 @@
 
 static size_t
 ext2_xattr_user_list(char *list, struct inode *inode,
-		     const char *name, int name_len, int flags)
+		     const char *name, int name_len)
 {
 	const int prefix_len = sizeof(XATTR_USER_PREFIX)-1;
 
-	if (!(flags & XATTR_KERNEL_CONTEXT) &&
-	    !test_opt(inode->i_sb, XATTR_USER))
+	if (!test_opt(inode->i_sb, XATTR_USER))
 		return 0;
 
 	if (list) {
@@ -37,23 +36,22 @@
 
 static int
 ext2_xattr_user_get(struct inode *inode, const char *name,
-		    void *buffer, size_t size, int flags)
+		    void *buffer, size_t size)
 {
+	int error;
+
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
-	if (!(flags & XATTR_KERNEL_CONTEXT)) {
-		int error;
-
-		if (!test_opt(inode->i_sb, XATTR_USER))
-			return -EOPNOTSUPP;
+	if (!test_opt(inode->i_sb, XATTR_USER))
+		return -EOPNOTSUPP;
 #ifdef CONFIG_EXT2_FS_POSIX_ACL
-		error = ext2_permission_locked(inode, MAY_READ);
+	error = ext2_permission_locked(inode, MAY_READ);
 #else
-		error = permission(inode, MAY_READ);
+	error = permission(inode, MAY_READ);
 #endif
-		if (error)
-			return error;
-	}
+	if (error)
+		return error;
+
 	return ext2_xattr_get(inode, EXT2_XATTR_INDEX_USER, name,
 			      buffer, size);
 }
@@ -62,24 +60,23 @@
 ext2_xattr_user_set(struct inode *inode, const char *name,
 		    const void *value, size_t size, int flags)
 {
+	int error;
+
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
+	if (!test_opt(inode->i_sb, XATTR_USER))
+		return -EOPNOTSUPP;
 	if ( !S_ISREG(inode->i_mode) &&
 	    (!S_ISDIR(inode->i_mode) || inode->i_mode & S_ISVTX))
 		return -EPERM;
-	if (!(flags & XATTR_KERNEL_CONTEXT)) {
-		int error;
-
-		if (!test_opt(inode->i_sb, XATTR_USER))
-			return -EOPNOTSUPP;
 #ifdef CONFIG_EXT2_FS_POSIX_ACL
-		error = ext2_permission_locked(inode, MAY_WRITE);
+	error = ext2_permission_locked(inode, MAY_WRITE);
 #else
-		error = permission(inode, MAY_WRITE);
+	error = permission(inode, MAY_WRITE);
 #endif
-		if (error)
-			return error;
-	}
+	if (error)
+		return error;
+  
 	return ext2_xattr_set(inode, EXT2_XATTR_INDEX_USER, name,
 			      value, size, flags);
 }
===== fs/ext3/acl.c 1.5 vs edited =====
--- 1.5/fs/ext3/acl.c	Sat Feb 15 03:24:02 2003
+++ edited/fs/ext3/acl.c	Thu Feb 20 19:27:15 2003
@@ -431,7 +431,7 @@
  */
 static size_t
 ext3_xattr_list_acl_access(char *list, struct inode *inode,
-			   const char *name, int name_len, int flags)
+			   const char *name, int name_len)
 {
 	const size_t size = sizeof(XATTR_NAME_ACL_ACCESS);
 
@@ -444,7 +444,7 @@
 
 static size_t
 ext3_xattr_list_acl_default(char *list, struct inode *inode,
-			    const char *name, int name_len, int flags)
+			    const char *name, int name_len)
 {
 	const size_t size = sizeof(XATTR_NAME_ACL_DEFAULT);
 
@@ -477,7 +477,7 @@
 
 static int
 ext3_xattr_get_acl_access(struct inode *inode, const char *name,
-			  void *buffer, size_t size, int flags)
+			  void *buffer, size_t size)
 {
 	if (strcmp(name, "") != 0)
 		return -EINVAL;
@@ -486,7 +486,7 @@
 
 static int
 ext3_xattr_get_acl_default(struct inode *inode, const char *name,
-			   void *buffer, size_t size, int flags)
+			   void *buffer, size_t size)
 {
 	if (strcmp(name, "") != 0)
 		return -EINVAL;
@@ -494,8 +494,7 @@
 }
 
 static int
-ext3_xattr_set_acl(struct inode *inode, int type, const void *value,
-		   size_t size)
+ext3_xattr_set_acl(struct inode *inode, int type, const void *value, size_t size)
 {
 	handle_t *handle;
 	struct posix_acl *acl;
===== fs/ext3/xattr.c 1.9 vs edited =====
--- 1.9/fs/ext3/xattr.c	Sat Feb 15 03:24:04 2003
+++ edited/fs/ext3/xattr.c	Thu Feb 20 19:27:15 2003
@@ -195,7 +195,7 @@
  */
 ssize_t
 ext3_getxattr(struct dentry *dentry, const char *name,
-	      void *buffer, size_t size, int flags)
+	      void *buffer, size_t size)
 {
 	struct ext3_xattr_handler *handler;
 	struct inode *inode = dentry->d_inode;
@@ -203,7 +203,7 @@
 	handler = ext3_xattr_resolve_name(&name);
 	if (!handler)
 		return -EOPNOTSUPP;
-	return handler->get(inode, name, buffer, size, flags);
+	return handler->get(inode, name, buffer, size);
 }
 
 /*
@@ -212,9 +212,9 @@
  * dentry->d_inode->i_sem down
  */
 ssize_t
-ext3_listxattr(struct dentry *dentry, char *buffer, size_t size, int flags)
+ext3_listxattr(struct dentry *dentry, char *buffer, size_t size)
 {
-	return ext3_xattr_list(dentry->d_inode, buffer, size, flags);
+	return ext3_xattr_list(dentry->d_inode, buffer, size);
 }
 
 /*
@@ -243,7 +243,7 @@
  * dentry->d_inode->i_sem down
  */
 int
-ext3_removexattr(struct dentry *dentry, const char *name, int flags)
+ext3_removexattr(struct dentry *dentry, const char *name)
 {
 	struct ext3_xattr_handler *handler;
 	struct inode *inode = dentry->d_inode;
@@ -251,7 +251,7 @@
 	handler = ext3_xattr_resolve_name(&name);
 	if (!handler)
 		return -EOPNOTSUPP;
-	return handler->set(inode, name, NULL, 0, flags | XATTR_REPLACE);
+	return handler->set(inode, name, NULL, 0, XATTR_REPLACE);
 }
 
 /*
@@ -364,8 +364,7 @@
  * used / required on success.
  */
 int
-ext3_xattr_list(struct inode *inode, char *buffer, size_t buffer_size,
-		int flags)
+ext3_xattr_list(struct inode *inode, char *buffer, size_t buffer_size)
 {
 	struct buffer_head *bh = NULL;
 	struct ext3_xattr_entry *entry;
@@ -405,7 +404,7 @@
 		handler = ext3_xattr_handler(entry->e_name_index);
 		if (handler)
 			size += handler->list(NULL, inode, entry->e_name,
-					      entry->e_name_len, flags);
+					      entry->e_name_len);
 	}
 
 	if (ext3_xattr_cache_insert(bh))
@@ -428,7 +427,7 @@
 		handler = ext3_xattr_handler(entry->e_name_index);
 		if (handler)
 			buf += handler->list(buf, inode, entry->e_name,
-					     entry->e_name_len, flags);
+					     entry->e_name_len);
 	}
 	error = size;
 
===== fs/ext3/xattr.h 1.5 vs edited =====
--- 1.5/fs/ext3/xattr.h	Sat Feb 15 03:24:04 2003
+++ edited/fs/ext3/xattr.h	Thu Feb 20 19:27:15 2003
@@ -57,9 +57,9 @@
 struct ext3_xattr_handler {
 	char *prefix;
 	size_t (*list)(char *list, struct inode *inode, const char *name,
-		       int name_len, int flags);
+		       int name_len);
 	int (*get)(struct inode *inode, const char *name, void *buffer,
-		   size_t size, int flags);
+		   size_t size);
 	int (*set)(struct inode *inode, const char *name, const void *buffer,
 		   size_t size, int flags);
 };
@@ -68,12 +68,12 @@
 extern void ext3_xattr_unregister(int, struct ext3_xattr_handler *);
 
 extern int ext3_setxattr(struct dentry *, const char *, const void *, size_t, int);
-extern ssize_t ext3_getxattr(struct dentry *, const char *, void *, size_t, int);
-extern ssize_t ext3_listxattr(struct dentry *, char *, size_t, int);
-extern int ext3_removexattr(struct dentry *, const char *, int);
+extern ssize_t ext3_getxattr(struct dentry *, const char *, void *, size_t);
+extern ssize_t ext3_listxattr(struct dentry *, char *, size_t);
+extern int ext3_removexattr(struct dentry *, const char *);
 
 extern int ext3_xattr_get(struct inode *, int, const char *, void *, size_t);
-extern int ext3_xattr_list(struct inode *, char *, size_t, int flags);
+extern int ext3_xattr_list(struct inode *, char *, size_t);
 extern int ext3_xattr_set(struct inode *, int, const char *, const void *, size_t, int);
 extern int ext3_xattr_set_handle(handle_t *, struct inode *, int, const char *, const void *, size_t, int);
 
===== fs/ext3/xattr_trusted.c 1.1 vs edited =====
--- 1.1/fs/ext3/xattr_trusted.c	Sat Feb 15 03:24:04 2003
+++ edited/fs/ext3/xattr_trusted.c	Thu Feb 20 19:30:51 2003
@@ -21,7 +21,7 @@
 {
 	const int prefix_len = sizeof(XATTR_TRUSTED_PREFIX)-1;
 
-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))
 		return 0;
 
 	if (list) {
@@ -38,7 +38,7 @@
 {
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return ext3_xattr_get(inode, EXT3_XATTR_INDEX_TRUSTED, name,
 			      buffer, size);
@@ -50,7 +50,7 @@
 {
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
-	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
+	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return ext3_xattr_set(inode, EXT3_XATTR_INDEX_TRUSTED, name,
 			      value, size, flags);
===== fs/ext3/xattr_user.c 1.6 vs edited =====
--- 1.6/fs/ext3/xattr_user.c	Sat Feb 15 03:24:03 2003
+++ edited/fs/ext3/xattr_user.c	Thu Feb 20 19:27:15 2003
@@ -21,12 +21,11 @@
 
 static size_t
 ext3_xattr_user_list(char *list, struct inode *inode,
-		     const char *name, int name_len, int flags)
+		     const char *name, int name_len)
 {
 	const int prefix_len = sizeof(XATTR_USER_PREFIX)-1;
 
-	if (!(flags & XATTR_KERNEL_CONTEXT) &&
-	    !test_opt(inode->i_sb, XATTR_USER))
+	if (!test_opt(inode->i_sb, XATTR_USER))
 		return 0;
 
 	if (list) {
@@ -39,23 +38,22 @@
 
 static int
 ext3_xattr_user_get(struct inode *inode, const char *name,
-		    void *buffer, size_t size, int flags)
+		    void *buffer, size_t size)
 {
+	int error;
+
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
-	if (!(flags & XATTR_KERNEL_CONTEXT)) {
-		int error;
-
-		if (!test_opt(inode->i_sb, XATTR_USER))
-			return -EOPNOTSUPP;
+	if (!test_opt(inode->i_sb, XATTR_USER))
+		return -EOPNOTSUPP;
 #ifdef CONFIG_EXT3_FS_POSIX_ACL
-		error = ext3_permission_locked(inode, MAY_READ);
+	error = ext3_permission_locked(inode, MAY_READ);
 #else
-		error = permission(inode, MAY_READ);
+	error = permission(inode, MAY_READ);
 #endif
-		if (error)
-			return error;
-	}
+	if (error)
+		return error;
+
 	return ext3_xattr_get(inode, EXT3_XATTR_INDEX_USER, name,
 			      buffer, size);
 }
@@ -64,26 +62,26 @@
 ext3_xattr_user_set(struct inode *inode, const char *name,
 		    const void *value, size_t size, int flags)
 {
+	int error;
+
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
+	if (!test_opt(inode->i_sb, XATTR_USER))
+		return -EOPNOTSUPP;
 	if ( !S_ISREG(inode->i_mode) &&
 	    (!S_ISDIR(inode->i_mode) || inode->i_mode & S_ISVTX))
 		return -EPERM;
-	if (!(flags & XATTR_KERNEL_CONTEXT)) {
-		int error;
-
-		if (!test_opt(inode->i_sb, XATTR_USER))
-			return -EOPNOTSUPP;
 #ifdef CONFIG_EXT3_FS_POSIX_ACL
-		error = ext3_permission_locked(inode, MAY_WRITE);
+	error = ext3_permission_locked(inode, MAY_WRITE);
 #else
-		error = permission(inode, MAY_WRITE);
+	error = permission(inode, MAY_WRITE);
 #endif
-		if (error)
-			return error;
-	}
+	if (error)
+		return error;
+  
 	return ext3_xattr_set(inode, EXT3_XATTR_INDEX_USER, name,
 			      value, size, flags);
+
 }
 
 struct ext3_xattr_handler ext3_xattr_user_handler = {
===== fs/jfs/jfs_xattr.h 1.4 vs edited =====
--- 1.4/fs/jfs/jfs_xattr.h	Sat Feb 15 03:24:02 2003
+++ edited/fs/jfs/jfs_xattr.h	Thu Feb 20 19:27:15 2003
@@ -57,8 +57,8 @@
 extern int jfs_setxattr(struct dentry *, const char *, const void *, size_t,
 			int);
 extern ssize_t __jfs_getxattr(struct inode *, const char *, void *, size_t);
-extern ssize_t jfs_getxattr(struct dentry *, const char *, void *, size_t, int);
-extern ssize_t jfs_listxattr(struct dentry *, char *, size_t, int);
-extern int jfs_removexattr(struct dentry *, const char *, int);
+extern ssize_t jfs_getxattr(struct dentry *, const char *, void *, size_t);
+extern ssize_t jfs_listxattr(struct dentry *, char *, size_t);
+extern int jfs_removexattr(struct dentry *, const char *);
 
 #endif	/* H_JFS_XATTR */
===== fs/jfs/xattr.c 1.11 vs edited =====
--- 1.11/fs/jfs/xattr.c	Sat Feb 15 03:24:02 2003
+++ edited/fs/jfs/xattr.c	Thu Feb 20 19:27:15 2003
@@ -962,13 +962,12 @@
 }
 
 ssize_t jfs_getxattr(struct dentry *dentry, const char *name, void *data,
-		     size_t buf_size, int flags)
+		     size_t buf_size)
 {
 	return __jfs_getxattr(dentry->d_inode, name, data, buf_size);
 }
 
-ssize_t jfs_listxattr(struct dentry * dentry, char *data, size_t buf_size,
-		      int flags)
+ssize_t jfs_listxattr(struct dentry * dentry, char *data, size_t buf_size)
 {
 	struct inode *inode = dentry->d_inode;
 	char *buffer;
@@ -1014,7 +1013,7 @@
 	return size;
 }
 
-int jfs_removexattr(struct dentry *dentry, const char *name, int flags)
+int jfs_removexattr(struct dentry *dentry, const char *name)
 {
 	return __jfs_setxattr(dentry->d_inode, name, 0, 0, XATTR_REPLACE);
 }
===== fs/xfs/linux/xfs_iops.c 1.15 vs edited =====
--- 1.15/fs/xfs/linux/xfs_iops.c	Sat Feb 15 03:24:02 2003
+++ edited/fs/xfs/linux/xfs_iops.c	Thu Feb 20 19:27:15 2003
@@ -640,8 +640,7 @@
 	struct dentry	*dentry,
 	const char	*name,
 	void		*data,
-	size_t		size,
-	int		flags)
+	size_t		size)
 {
 	ssize_t		error;
 	int		xflags = 0;
@@ -698,8 +697,7 @@
 linvfs_listxattr(
 	struct dentry		*dentry,
 	char			*data,
-	size_t			size,
-	int			flags)
+	size_t			size)
 {
 	ssize_t			error;
 	int			result = 0;
@@ -743,8 +741,7 @@
 STATIC int
 linvfs_removexattr(
 	struct dentry	*dentry,
-	const char	*name,
-	int		flags)
+	const char	*name)
 {
 	int		error;
 	int		xflags = 0;
===== include/linux/fs.h 1.219 vs edited =====
--- 1.219/include/linux/fs.h	Sat Feb 15 03:24:02 2003
+++ edited/include/linux/fs.h	Thu Feb 20 19:27:15 2003
@@ -743,9 +743,9 @@
 	int (*setattr) (struct dentry *, struct iattr *);
 	int (*getattr) (struct vfsmount *mnt, struct dentry *, struct kstat *);
 	int (*setxattr) (struct dentry *, const char *,const void *,size_t,int);
-	ssize_t (*getxattr) (struct dentry *, const char *, void *, size_t,int);
-	ssize_t (*listxattr) (struct dentry *, char *, size_t, int);
-	int (*removexattr) (struct dentry *, const char *, int);
+	ssize_t (*getxattr) (struct dentry *, const char *, void *, size_t);
+	ssize_t (*listxattr) (struct dentry *, char *, size_t);
+	int (*removexattr) (struct dentry *, const char *);
 };
 
 struct seq_file;
===== include/linux/i2c-proc.h 1.4 vs edited =====
--- 1.4/include/linux/i2c-proc.h	Tue Feb  4 21:58:09 2003
+++ edited/include/linux/i2c-proc.h	Thu Feb 20 19:59:01 2003
@@ -69,8 +69,7 @@
    these functions must be updated! */
 extern int i2c_register_entry(struct i2c_client *client,
 				  const char *prefix,
-				  ctl_table * ctl_template,
-				  struct module *controlling_mod);
+				  ctl_table * ctl_template);
 
 extern void i2c_deregister_entry(int id);
 
===== include/linux/xattr.h 1.3 vs edited =====
--- 1.3/include/linux/xattr.h	Sat Feb 15 03:24:03 2003
+++ edited/include/linux/xattr.h	Thu Feb 20 19:27:03 2003
@@ -9,8 +9,7 @@
 #ifndef _LINUX_XATTR_H
 #define _LINUX_XATTR_H
 
-#define XATTR_CREATE		0x1	/* fail if attr already exists */
-#define XATTR_REPLACE		0x2	/* fail if attr does not exist */
-#define XATTR_KERNEL_CONTEXT	0x4	/* called from kernel context */
+#define XATTR_CREATE	0x1	/* set value, fail if attr already exists */
+#define XATTR_REPLACE	0x2	/* set value, fail if attr does not exist */
 
 #endif	/* _LINUX_XATTR_H */

Re: [PATCH] backout the xattr override access checks flag

[Andreas Dilger]

On Feb 20, 2003  22:20 -0500, Christoph Hellwig wrote:
> -	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
> +	if (!capable(CAP_SYS_ADMIN))

How about using CAP_DAC_OVERRIDE instead of CAP_SYS_ADMIN, since the
latter is _way_ over used, and basically amounts to a uid=0 test these
days.

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

Re: [PATCH] backout the xattr override access checks flag

[Christoph Hellwig]

On Thu, Feb 20, 2003 at 01:20:58PM -0700, Andreas Dilger wrote:
> On Feb 20, 2003  22:20 -0500, Christoph Hellwig wrote:
> > -	if (!((flags & XATTR_KERNEL_CONTEXT) || capable(CAP_SYS_ADMIN)))
> > +	if (!capable(CAP_SYS_ADMIN))
> 
> How about using CAP_DAC_OVERRIDE instead of CAP_SYS_ADMIN, since the
> latter is _way_ over used, and basically amounts to a uid=0 test these
> days.

It's not a DAC override strictly spoken.  But I agree with you that it
shouldn't be CAP_SYS_ADMIN (see my previous discussion with Andreas).

Neverless this is purely a change to backout the flags change and I don't
want to mix up too much.

Re: [PATCH] backout the xattr override access checks flag

[Andreas Gruenbacher]

On Friday 21 February 2003 04:20, you wrote:
> This check backs out the XATTR_KERNEL_CONTEXT that tells filesystems
> to ignore the lack of capabilities of the caller that went in in the
> last week (through akpm..).
>
> It's a cludgy interface design to have flags that change access checks,
> and there's a much easier way by raising the capabilities in the process
> that actually needs this in kernel mode

Thanks, I'm not fully convinced that raising capabilities temporarily in the 
kernel is a good thing to do, but it surely obsoletes the 
XATTR_KERNEL_CONTEXT flag.

> (not that such code even exists yet).

There is an HSM project for which this feature has been added. I think they 
are using a loadable module.

> Something even better would probably be to move out the xattr access checks
> to common code.

There are two problems with that, so this doesn't seem any better to me, 
either:

(a) We would have to decode attribute names twice, once for checking 
permissions, and a second time for determining how to store them.

(b) Different file systems may implement different features with different, 
file system specific limitations. The VFS layer tests would have to accept 
all potentially useful things. The file system would have to re-check.


Cheers,
Andreas.

Re: [PATCH] backout the xattr override access checks flag

[Christoph Hellwig]

On Fri, Feb 21, 2003 at 11:20:25AM +0100, Andreas Gruenbacher wrote:
> > (not that such code even exists yet).
> 
> There is an HSM project for which this feature has been added. I think they 
> are using a loadable module.

URL?

> 
> > Something even better would probably be to move out the xattr access checks
> > to common code.
> 
> There are two problems with that, so this doesn't seem any better to me, 
> either:
> 
> (a) We would have to decode attribute names twice, once for checking 
> permissions, and a second time for determining how to store them.

Doing it in the VFS would probably mean a rather large interface change
so it should be decoded only once.  I.e. moving your current ext2/ext3-specific
handler abstraction to the VFS instead.

> (b) Different file systems may implement different features with different, 
> file system specific limitations. The VFS layer tests would have to accept 
> all potentially useful things. The file system would have to re-check.

What types of EAs do we have?

(1) user attributes - the only access checks needed are the normal DAC ones
(2) system/trusted - only privilegued access

I think that's doable.

Re: [PATCH] backout the xattr override access checks flag

[Andreas Gruenbacher]

On Friday 21 February 2003 16:54, Christoph Hellwig wrote:
> On Fri, Feb 21, 2003 at 11:20:25AM +0100, Andreas Gruenbacher wrote:
> > > (not that such code even exists yet).
> >
> > There is an HSM project for which this feature has been added. I think
> > they are using a loadable module.
>
> URL?
>
> > > Something even better would probably be to move out the xattr access
> > > checks to common code.
> >
> > There are two problems with that, so this doesn't seem any better to me,
> > either:
> >
> > (a) We would have to decode attribute names twice, once for checking
> > permissions, and a second time for determining how to store them.
>
> Doing it in the VFS would probably mean a rather large interface change
> so it should be decoded only once.  I.e. moving your current
> ext2/ext3-specific handler abstraction to the VFS instead.
>
> > (b) Different file systems may implement different features with
> > different, file system specific limitations. The VFS layer tests would
> > have to accept all potentially useful things. The file system would have
> > to re-check.
>
> What types of EAs do we have?
>
> (1) user attributes - the only access checks needed are the normal DAC ones
> (2) system/trusted - only privilegued access
>
> I think that's doable.

User and Trusted extended attributes have permission checks per namespace 
(i.e., the rules for all user.* attributes are identical, and the rules for 
all trusted.* attributes are identical).  System extended attribute access 
rules are per attribute: ACLs, Capabilities, MAC all have different policies.

Also, different file systems have different mechanisms for storing EAs, so 
that need to decode the attribute name, no matter what the VFS does.

I was thinking of at least abstracting the the permission checks from the file 
systems, so the file system layers can all use the same VFS helper function. 
But this is no real improvement, either:

	enum xattr_which { XATTR_USER, XATTR_TRUSTED,
				XATTR_POSIX_ACL, XATTR_CAP, ... };

	int xattr_permission(struct inode *inode, enum xattr_which which, int mask)
	{
		switch(which) {
			case XATTR_USER:
				return permission(inode, mask);

			case XATTR_TRUSTED:
				return capable(CAP_WHATEVER);

			case XATTR_POSIX_ACL:
				if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
					return -EPERM;
				return 0;
			default:
				return -EOPNOTSUPP;
		}
	}

This pseudo code is actually broken because it needs to be called from within 
the xattr inode operations where the inode lock is held, so the VFS 
permission() function cannot even be called because iops->permission() might 
grab the inode lock as well (and needs to on ACL aware systems).

So I agree with you that the checks could be moved to the VFS, but I don't see 
how this can be done in a way that is clearly better that keeping the checks 
in the FS.


Cheers,
Andreas.