Re: [PATCH 16/17] NFSD: Server implementation of MAC Labeling

[Steve Dickson <SteveD@redhat.com>]

On 08/05/13 21:50, J. Bruce Fields wrote:
> This also doesn't handle the unsupported case correctly.
> 
> Applying the following fix to my tree.
> 
> --b.
> 
> commit 4438e5f801c43cc0f383fdc85f3ec294a4155df1
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Wed May 8 21:38:00 2013 -0400
> 
>     nfsd4: fix handling of case when labels are unsupported
>     
>     Copy the logic in the ACL case to ensure that we just turn off the
>     bitmap in the getattr case instead of erroring out, and to ensure that
>     we get the supported_attributes attribute right.
>     
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 6e96c0d..3b4e59c 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2010,29 +2010,21 @@ nfsd4_encode_aclname(struct svc_rqst *rqstp, struct nfs4_ace *ace,
>  
>  #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>  static inline __be32
> -nfsd4_encode_security_label(struct svc_rqst *rqstp, struct dentry *dentry, __be32 **pp, int *buflen)
> +nfsd4_encode_security_label(struct svc_rqst *rqstp, void *context, int len, __be32 **pp, int *buflen)
>  {
> -	void *context;
> -	int host_err;
> -	__be32 err = nfserr_resource;
> -	u32 len;
>  	uint32_t pi = 0;
>  	uint32_t lfs = 0;
>  	__be32 *p = *pp;
>  
> -	host_err = security_inode_getsecctx(dentry->d_inode, &context, &len);
> -	if (host_err)
> -		return nfserrno(host_err);
> -
>  	if (*buflen < ((XDR_QUADLEN(len) << 2) + 4 + 4 + 4))
> -		goto out;
> +		return nfserr_resource;
>  
>  	/*
>  	 * For now we use a 0 here to indicate the null translation; in
>  	 * the future we may place a call to translation code here.
>  	 */
>  	if ((*buflen -= 8) < 0)
> -		goto out;
> +		return nfserr_resource;
>  
>  	WRITE32(lfs);
>  	WRITE32(pi);
> @@ -2040,10 +2032,7 @@ nfsd4_encode_security_label(struct svc_rqst *rqstp, struct dentry *dentry, __be3
>  	*buflen -= (XDR_QUADLEN(len) << 2) + 4;
>  
>  	*pp = p;
> -	err = 0;
> -out:
> -	security_release_secctx(context, len);
> -	return err;
> +	return 0;
>  }
>  #else
>  static inline __be32
> @@ -2110,6 +2099,9 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
>  	int err;
>  	int aclsupport = 0;
>  	struct nfs4_acl *acl = NULL;
> +	void *context = NULL;
> +	int contextlen;
> +	bool contextsupport = false;
>  	struct nfsd4_compoundres *resp = rqstp->rq_resp;
>  	u32 minorversion = resp->cstate.minorversion;
>  	struct path path = {
> @@ -2163,6 +2155,21 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
>  		}
>  	}
>  
> +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> +	if ((bmval[2] & FATTR4_WORD2_SECURITY_LABEL) ||
> +			bmval[0] & FATTR4_WORD0_SUPPORTED_ATTRS) {
> +		err = security_inode_getsecctx(dentry->d_inode,
> +						&context, &contextlen);
> +		contextsupport = (err = 0);
Don't you really mean "contextsupport = (err == 0);" ?
Otherwise contextsupport will never be set to 1.

steved.

> +		if (bmval2 & FATTR4_WORD2_SECURITY_LABEL) {
> +			if (err == -EOPNOTSUPP)
> +				bmval2 &= ~FATTR4_WORD2_SECURITY_LABEL;
> +			else if (err)
> +				goto out_nfserr;
> +		}
> +	}
> +#endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
> +
>  	if (bmval2) {
>  		if ((buflen -= 16) < 0)
>  			goto out_resource;
> @@ -2191,6 +2198,8 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
>  
>  		if (!aclsupport)
>  			word0 &= ~FATTR4_WORD0_ACL;
> +		if (!contextsupport)
> +			word2 &= ~FATTR4_WORD2_SECURITY_LABEL;
>  		if (!word2) {
>  			if ((buflen -= 12) < 0)
>  				goto out_resource;
> @@ -2499,8 +2508,8 @@ out_acl:
>  		WRITE64(stat.ino);
>  	}
>  	if (bmval2 & FATTR4_WORD2_SECURITY_LABEL) {
> -		status = nfsd4_encode_security_label(rqstp, dentry,
> -				&p, &buflen);
> +		status = nfsd4_encode_security_label(rqstp, context,
> +				contextlen, &p, &buflen);
>  		if (status)
>  			goto out;
>  	}
> @@ -2516,6 +2525,8 @@ out_acl:
>  	status = nfs_ok;
>  
>  out:
> +	if (context)
> +		security_release_secctx(context, contextlen);
>  	kfree(acl);
>  	if (fhp == &tempfh)
>  		fh_put(&tempfh);
>