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

[Tim Gardner]

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.

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