linux-kernel - Re: [RFC PATCH] NFS: Fix the access mask checks in NFSv4

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 31 Jul 2017 12:19:29 +0100
From:   Catalin Marinas <catalin.marinas@....com>
To:     linux-nfs@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org,
        Trond Myklebust <trond.myklebust@...marydata.com>,
        Anna Schumaker <Anna.Schumaker@...app.com>
Subject: Re: [RFC PATCH] NFS: Fix the access mask checks in NFSv4

Anna, Trond,

On Fri, Jul 28, 2017 at 06:54:52PM +0100, Catalin Marinas wrote:
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2236,7 +2236,7 @@ static int nfs4_opendata_access(struct rpc_cred *cred,
>  				int openflags)
>  {
>  	struct nfs_access_entry cache;
> -	u32 mask;
> +	int mask, cache_mask;
>  
>  	/* access call failed or for some reason the server doesn't
>  	 * support any access modes -- defer access call until later */
> @@ -2259,7 +2259,8 @@ static int nfs4_opendata_access(struct rpc_cred *cred,
>  	nfs_access_set_mask(&cache, opendata->o_res.access_result);
>  	nfs_access_add_cache(state->inode, &cache);
>  
> -	if ((mask & ~cache.mask & (MAY_READ | MAY_EXEC)) == 0)
> +	cache_mask = nfs_access_calc_mask(cache.mask, state->inode->i_mode);
> +	if ((mask & ~cache_mask & (MAY_READ | MAY_EXEC)) == 0)
>  		return 0;
>  
>  	return -EACCES;

I noticed -rc3 already has a fix here, commit 1e6f209515a0 ("NFS: Use
raw NFS access mask in nfs4_opendata_access()"), without having to
export nfs_access_calc_mask().

> @@ -3870,25 +3871,9 @@ static int _nfs4_proc_access(struct inode *inode, struct nfs_access_entry *entry
>  		.rpc_resp = &res,
>  		.rpc_cred = entry->cred,
>  	};
> -	int mode = entry->mask;
>  	int status = 0;
>  
> -	/*
> -	 * Determine which access bits we want to ask for...
> -	 */
> -	if (mode & MAY_READ)
> -		args.access |= NFS4_ACCESS_READ;
> -	if (S_ISDIR(inode->i_mode)) {
> -		if (mode & MAY_WRITE)
> -			args.access |= NFS4_ACCESS_MODIFY | NFS4_ACCESS_EXTEND | NFS4_ACCESS_DELETE;
> -		if (mode & MAY_EXEC)
> -			args.access |= NFS4_ACCESS_LOOKUP;
> -	} else {
> -		if (mode & MAY_WRITE)
> -			args.access |= NFS4_ACCESS_MODIFY | NFS4_ACCESS_EXTEND;
> -		if (mode & MAY_EXEC)
> -			args.access |= NFS4_ACCESS_EXECUTE;
> -	}
> +	args.access = entry->mask;

However, AFAICT, 'entry->mask' already uses the NFS4_* bits, so checking
'mode' against MAY_READ etc. to build up args.access is incorrect. Is
this hunk still needed?

-- 
Catalin