[PATCH 1/3] VFS: Fix access("file", X_OK) in the presence of ACLs

[PATCH 1/3] VFS: Fix access("file", X_OK) in the presence of ACLs

[Trond Myklebust]

From: Trond Myklebust <Trond.Myklebust@netapp.com>

Currently, the access() call will return incorrect information on NFS if
there exists an ACL that grants execute access to the user on a regular
file. The reason the information is incorrect is that the VFS overrides
this execute access in open_exec() by checking (inode->i_mode & 0111).

This patch propagates the VFS execute bit check back into the generic
permission() call.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/namei.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 664b4a5..08cc418 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -227,10 +227,10 @@ int generic_permission(struct inode *ino
 
 int permission(struct inode *inode, int mask, struct nameidata *nd)
 {
+	umode_t mode = inode->i_mode;
 	int retval, submask;
 
 	if (mask & MAY_WRITE) {
-		umode_t mode = inode->i_mode;
 
 		/*
 		 * Nobody gets write access to a read-only fs.
@@ -247,6 +247,13 @@ int permission(struct inode *inode, int 
 	}
 
 
+	/*
+	 * MAY_EXEC on regular files requires special handling: We override
+	 * filesystem execute permissions if the mode bits aren't set.
+	 */
+	if ((mask & MAY_EXEC) && S_ISREG(mode) && !(mode & S_IXUGO))
+		return -EACCES;
+
 	/* Ordinary permission routines do not understand MAY_APPEND. */
 	submask = mask & ~MAY_APPEND;
 	if (inode->i_op && inode->i_op->permission)

[PATCH 2/3] VFS: Remove redundant open-coded mode bit check in prepare_binfmt().

[Trond Myklebust]

From: Trond Myklebust <Trond.Myklebust@netapp.com>

The check in prepare_binfmt() for inode->i_mode & 0111 is redundant,
since open_exec() will already have done that.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/exec.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 8344ba7..a6f64a9 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -922,12 +922,6 @@ int prepare_binprm(struct linux_binprm *
 	int retval;
 
 	mode = inode->i_mode;
-	/*
-	 * Check execute perms again - if the caller has CAP_DAC_OVERRIDE,
-	 * generic_permission lets a non-executable through
-	 */
-	if (!(mode & 0111))	/* with at least _one_ execute bit set */
-		return -EACCES;
 	if (bprm->file->f_op == NULL)
 		return -EACCES;
 

[PATCH 3/3] VFS: Remove redundant open-coded mode bit checks in open_exec().

[Trond Myklebust]

From: Trond Myklebust <Trond.Myklebust@netapp.com>

The check in open_exec() for inode->i_mode & 0111 has been made
redundant by the fix to permission().

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/exec.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index a6f64a9..f7aabfe 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -486,8 +486,6 @@ struct file *open_exec(const char *name)
 		if (!(nd.mnt->mnt_flags & MNT_NOEXEC) &&
 		    S_ISREG(inode->i_mode)) {
 			int err = vfs_permission(&nd, MAY_EXEC);
-			if (!err && !(inode->i_mode & 0111))
-				err = -EACCES;
 			file = ERR_PTR(err);
 			if (!err) {
 				file = nameidata_to_filp(&nd, O_RDONLY);

Re: [PATCH 1/3] VFS: Fix access("file", X_OK) in the presence of ACLs

[Peter Staubach]

Trond Myklebust wrote:

>From: Trond Myklebust <Trond.Myklebust@netapp.com>
>
>Currently, the access() call will return incorrect information on NFS if
>there exists an ACL that grants execute access to the user on a regular
>file. The reason the information is incorrect is that the VFS overrides
>this execute access in open_exec() by checking (inode->i_mode & 0111).
>
>This patch propagates the VFS execute bit check back into the generic
>permission() call.
>
>Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
>---
>
> fs/namei.c |    9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
>diff --git a/fs/namei.c b/fs/namei.c
>index 664b4a5..08cc418 100644
>--- a/fs/namei.c
>+++ b/fs/namei.c
>@@ -227,10 +227,10 @@ int generic_permission(struct inode *ino
> 
> int permission(struct inode *inode, int mask, struct nameidata *nd)
> {
>+	umode_t mode = inode->i_mode;
> 	int retval, submask;
> 
> 	if (mask & MAY_WRITE) {
>-		umode_t mode = inode->i_mode;
> 
> 		/*
> 		 * Nobody gets write access to a read-only fs.
>@@ -247,6 +247,13 @@ int permission(struct inode *inode, int 
> 	}
> 
> 
>+	/*
>+	 * MAY_EXEC on regular files requires special handling: We override
>+	 * filesystem execute permissions if the mode bits aren't set.
>+	 */
>+	if ((mask & MAY_EXEC) && S_ISREG(mode) && !(mode & S_IXUGO))
>+		return -EACCES;
>+
> 	/* Ordinary permission routines do not understand MAY_APPEND. */
> 	submask = mask & ~MAY_APPEND;
> 	if (inode->i_op && inode->i_op->permission)
>-
>  
>

Does this imply that some of the code in places like generic_permission(),
fuse_permission(), and xfs_iaccess() can be cleaned up too?  They contain
code which appears to check to ensure that an exec bit is on before allowing
an override.

    Thanx...

       ps

Re: [PATCH 1/3] VFS: Fix access("file", X_OK) in the presence of ACLs

[Trond Myklebust]

On Thu, 2006-07-13 at 09:59 -0400, Peter Staubach wrote:
> Does this imply that some of the code in places like generic_permission(),
> fuse_permission(), and xfs_iaccess() can be cleaned up too?  They contain
> code which appears to check to ensure that an exec bit is on before allowing
> an override.

They probably can.

Note, though, that we still have a problem with NTFS or NFSv4 acls:
unlike POSIX acls, those permit you to give a particular user execute
rights without having an execute mode bit set.

If we want to allow Linux to support that type of ACL, then we will in
fact need to move the mode bit check down into the filesystems, and
remove the VFS override.
The patches that I sent in have shied short of doing this (they only
remove the existing inconsistency within the VFS) but I'd be happy to
write something up if Al is willing to consider it.

Cheers,
  Trond