ACK: [Natty] SRU: nfsd4: permit read opens of executable-only files

Fri Dec 16 14:07:05 UTC 2011

On Fri, Dec 16, 2011 at 06:55:43AM -0700, Tim Gardner wrote:
> On 12/16/2011 04:56 AM, Herton Ronaldo Krzesinski wrote:
> >On Thu, Dec 15, 2011 at 02:31:39PM -0600, Chris J Arges wrote:
> >>SRU Justification
> >>
> >>Impact:
> >>
> >>NFSv4 mount point does not allow binary files to run when permissions
> >>are set to 111.
> >>
> >>Fix:
> >>
> >>Upstream commit a043226bc140a2c1dde162246d68a67e5043e6b2.
> >>Applies with minor conflicts.
> >>
> >>Testcase:
> >>Mount a directory using nfsv4. Inside that directory place a binary with
> >>---x--x--x permissions. Try to execute the binary. Without this patch it
> >>will not execute.
> >>
> >>Notes:
> >>This patch fixes this issue, but requires support from the nfs client as
> >>well. The Natty version of the client will need additional fixes to
> >>complete the fix. The upstream maintainer has verified that the kernel
> >>is doing the right thing. http://comments.gmane.org/gmane.linux.nfs/43028
> >>
> >>Thanks,
> >>--chris j arges
> >
> >> From c081b4c32ab604965c4d82438780b967f7c5ad62 Mon Sep 17 00:00:00 2001
> >>From: "J. Bruce Fields"<bfields at redhat.com>
> >>Date: Thu, 25 Aug 2011 10:48:39 -0400
> >>Subject: [PATCH] nfsd4: permit read opens of executable-only files
> >>
> >>A client that wants to execute a file must be able to read it.  Read
> >>opens over nfs are therefore implicitly allowed for executable files
> >>even when those files are not readable.
> >>
> >>NFSv2/v3 get this right by using a passed-in NFSD_MAY_OWNER_OVERRIDE on
> >>read requests, but NFSv4 has gotten this wrong ever since
> >>dc730e173785e29b297aa605786c94adaffe2544 "nfsd4: fix owner-override on
> >>open", when we realized that the file owner shouldn't override
> >>permissions on non-reclaim NFSv4 opens.
> >>
> >>So we can't use NFSD_MAY_OWNER_OVERRIDE to tell nfsd_permission to allow
> >>reads of executable files.
> >>
> >>So, do the same thing we do whenever we encounter another weird NFS
> >>permission nit: define yet another NFSD_MAY_* flag.
> >>
> >>The industry's future standardization on 128-bit processors will be
> >>motivated primarily by the need for integers with enough bits for all
> >>the NFSD_MAY_* flags.
> >>
> >>Reported-by: Leonardo Borda<leonardoborda at gmail.com>
> >>Cc: stable at kernel.org
> >>Signed-off-by: J. Bruce Fields<bfields at redhat.com>
> >>(cherry picked from commit a043226bc140a2c1dde162246d68a67e5043e6b2)
> >>
> >>BugLink: http://bugs.launchpad.net/bugs/833300
> >>
> >>Conflicts:
> >>
> >>	fs/nfsd/vfs.h
> >>
> >>Signed-off-by: Chris J Arges<chris.j.arges at canonical.com>
> >>---
> >>  fs/nfsd/nfs4proc.c |    2 ++
> >>  fs/nfsd/vfs.c      |    3 ++-
> >>  fs/nfsd/vfs.h      |    2 ++
> >>  3 files changed, 6 insertions(+), 1 deletions(-)
> >>
> >>diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >>index 5fcb139..2df5614 100644
> >>--- a/fs/nfsd/nfs4proc.c
> >>+++ b/fs/nfsd/nfs4proc.c
> >>@@ -156,6 +156,8 @@ do_open_permission(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfs
> >>  		!(open->op_share_access&  NFS4_SHARE_ACCESS_WRITE))
> >>  		return nfserr_inval;
> >>
> >>+	accmode |= NFSD_MAY_READ_IF_EXEC;
> >>+
> >>  	if (open->op_share_access&  NFS4_SHARE_ACCESS_READ)
> >>  		accmode |= NFSD_MAY_READ;
> >>  	if (open->op_share_access&  NFS4_SHARE_ACCESS_WRITE)
> >>diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> >>index 435f407..f0ee82a 100644
> >>--- a/fs/nfsd/vfs.c
> >>+++ b/fs/nfsd/vfs.c
> >>@@ -2104,7 +2104,8 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
> >>
> >>  	/* Allow read access to binaries even when mode 111 */
> >>  	if (err == -EACCES&&  S_ISREG(inode->i_mode)&&
> >>-	    acc == (NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE))
> >>+	     (acc == (NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE) ||
> >>+	      acc == (NFSD_MAY_READ | NFSD_MAY_READ_IF_EXEC)))
> >>  		err = inode_permission(inode, MAY_EXEC);
> >>
> >>  	return err? nfserrno(err) : 0;
> >>diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> >>index 9a370a5..dbe9ac1 100644
> >>--- a/fs/nfsd/vfs.h
> >>+++ b/fs/nfsd/vfs.h
> >>@@ -21,6 +21,8 @@
> >>  #define NFSD_MAY_LOCAL_ACCESS	128 /* IRIX doing local access check on device special file*/
> >>  #define NFSD_MAY_BYPASS_GSS_ON_ROOT 256
> >>  #define NFSD_MAY_NOT_BREAK_LEASE 512
> >>+#define NFSD_MAY_BYPASS_GSS	1024
> >>+#define NFSD_MAY_READ_IF_EXEC	2048
> >
> >Any reason to also add NFSD_MAY_BYPASS_GSS?
> >
> >Also may be it would be good to settle the client situation on natty
> >before, as the patch will not help currently.
> >
> >>
> >>  #define NFSD_MAY_CREATE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE)
> >>  #define NFSD_MAY_REMOVE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE|NFSD_MAY_TRUNC)
> >>--
> 
> The patch does seem to have picked up a bit of cruft with
> NFSD_MAY_BYPASS_GSS, but its harmless. It can be cleaned up when the
> patch is applied.
> 
> Herton - I think the kernel has to be in place before user space can
> work. This doesn't look like it'll cause any regression.

Ok, ack from my side as well then.

I just got confused by J. Bruce Fields comment Chris pointed out, seems
he is saying that nfs-utils or possibly nfs-common doesn't have anything
to do with the problem, but it's only a matter of kernel on the client
side, so may be just be sure to test with an updated kernel on both
server and client on Natty. Anyway the patch should be needed in either
case I expect.

> 
> rtg
> -- 
> Tim Gardner tim.gardner at canonical.com
> 

-- 
[]'s
Herton