Return-Path: linux-nfs-owner@vger.kernel.org Received: from fieldses.org ([174.143.236.118]:38223 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760799Ab3D3Ptu (ORCPT ); Tue, 30 Apr 2013 11:49:50 -0400 Date: Tue, 30 Apr 2013 11:49:47 -0400 To: Steve Dickson Cc: Trond Myklebust , "J. Bruce Fields" , "David P. Quigley" , Linux NFS list , Linux FS devel list , Linux Security List , SELinux List Subject: Re: [PATCH 16/17] NFSD: Server implementation of MAC Labeling Message-ID: <20130430154947.GW17268@fieldses.org> References: <1367240239-19326-1-git-send-email-SteveD@redhat.com> <1367240239-19326-17-git-send-email-SteveD@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <1367240239-19326-17-git-send-email-SteveD@redhat.com> From: "J. Bruce Fields" Sender: linux-nfs-owner@vger.kernel.org List-ID: On Mon, Apr 29, 2013 at 08:57:18AM -0400, Steve Dickson wrote: > From: David Quigley > > This patch adds the ability to encode and decode file labels on the server for > the purpose of sending them to the client and also to process label change > requests from the client. One overflow and a few nits: > Signed-off-by: Matthew N. Dodd > Signed-off-by: Miguel Rodel Felipe > Signed-off-by: Phua Eu Gene > Signed-off-by: Khin Mi Mi Aung > --- > fs/nfsd/nfs4proc.c | 41 +++++++++++++++++ > fs/nfsd/nfs4xdr.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++---- > fs/nfsd/nfsd.h | 15 +++++++ > fs/nfsd/nfsproc.c | 1 + > fs/nfsd/vfs.c | 28 ++++++++++++ > fs/nfsd/vfs.h | 2 + > fs/nfsd/xdr4.h | 3 ++ > 7 files changed, 209 insertions(+), 8 deletions(-) > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > index ae73175..bb17589 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -42,6 +42,36 @@ > #include "current_stateid.h" > #include "netns.h" > > +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL > +#include > + > +static inline void > +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval) > +{ > + struct inode *inode = resfh->fh_dentry->d_inode; > + int status; > + > + mutex_lock(&inode->i_mutex); > + status = security_inode_setsecctx(resfh->fh_dentry, > + label->label, label->len); > + mutex_unlock(&inode->i_mutex); > + > + if (status) > + /* > + * We should probably fail the whole open at this point, > + * but we've already created or opened the file, so it's > + * too late; So this seems the least of evils: > + */ > + bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL; > + > + return; > +} > +#else > +static inline void > +nfsd4_security_inode_setsecctx(struct svc_fh *resfh, struct nfs4_label *label, u32 *bmval) > +{ } > +#endif > + > #define NFSDDBG_FACILITY NFSDDBG_PROC > > static u32 nfsd_attrmask[] = { > @@ -230,6 +260,9 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o > (u32 *)open->op_verf.data, > &open->op_truncate, &open->op_created); > > + if (!status && open->op_label != NULL) > + nfsd4_security_inode_setsecctx(resfh, open->op_label, open->op_bmval); > + > /* > * Following rfc 3530 14.2.16, use the returned bitmask > * to indicate which attributes we used to store the > @@ -599,6 +632,9 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > if (status) > goto out; > > + if (create->cr_label != NULL) > + nfsd4_security_inode_setsecctx(&resfh, create->cr_label, create->cr_bmval); > + > if (create->cr_acl != NULL) > do_set_nfs4_acl(rqstp, &resfh, create->cr_acl, > create->cr_bmval); > @@ -888,6 +924,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > setattr->sa_acl); > if (status) > goto out; > + if (setattr->sa_label != NULL) > + status = nfsd4_set_nfs4_label(rqstp, &cstate->current_fh, > + setattr->sa_label); > + if (status) > + goto out; > status = nfsd_setattr(rqstp, &cstate->current_fh, &setattr->sa_iattr, > 0, (time_t)0); > out: > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index b38de7a..9d7af7a 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -55,6 +55,11 @@ > #include "cache.h" > #include "netns.h" > > +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL > +#include > +#endif > + > + > #define NFSDDBG_FACILITY NFSDDBG_XDR > > /* > @@ -79,6 +84,24 @@ check_filename(char *str, int len) > return nfserr_badname; > return 0; > } > +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL > +static struct nfs4_label *nfsd4_label_alloc(u32 len) > +{ > + struct nfs4_label *label = NULL; > + > + if (len > NFS4_MAXLABELLEN) > + return ERR_PTR(-EMSGSIZE); > + > + label = kzalloc(len + sizeof(struct nfs4_label), GFP_KERNEL); That should be len + sizeof(struct nfs4_label) + 1, since decode_fattr null-terminates the label. > + if (label == NULL) > + return ERR_PTR(-ENOMEM); > + > + label->label = (char *)(label + 1); > + label->len = len; > + > + return label; > +} > +#endif > > #define DECODE_HEAD \ > __be32 *p; \ > @@ -242,7 +265,8 @@ nfsd4_decode_bitmap(struct nfsd4_compoundargs *argp, u32 *bmval) > > static __be32 > nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval, > - struct iattr *iattr, struct nfs4_acl **acl) > + struct iattr *iattr, struct nfs4_acl **acl, > + struct nfs4_label **label) > { > int expected_len, len = 0; > u32 dummy32; > @@ -386,6 +410,41 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval, > goto xdr_error; > } > } > + > + *label = NULL; > +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL > + if (bmval[2] & FATTR4_WORD2_SECURITY_LABEL) { > + uint32_t pi; > + uint32_t lfs; > + > + READ_BUF(4); > + len += 4; > + READ32(lfs); > + READ_BUF(4); > + len += 4; > + READ32(pi); > + READ_BUF(4); > + len += 4; > + READ32(dummy32); > + READ_BUF(dummy32); > + len += (XDR_QUADLEN(dummy32) << 2); > + READMEM(buf, dummy32); > + > + *label = nfsd4_label_alloc(dummy32); > + if (IS_ERR(*label)) { > + host_err = PTR_ERR(*label); > + goto out_nfserr; > + } > + > + memcpy((*label)->label, buf, (*label)->len); > + ((char *)(*label)->label)[(*label)->len] = '\0'; As noted above, this is past the end of the allocated label. Also, label is already defined as (char *), so the cast's unnecessary. > + (*label)->pi = pi; > + (*label)->lfs = lfs; > + > + defer_free(argp, kfree, *label); > + } > +#endif > + > if (bmval[0] & ~NFSD_WRITEABLE_ATTRS_WORD0 > || bmval[1] & ~NFSD_WRITEABLE_ATTRS_WORD1 > || bmval[2] & ~NFSD_WRITEABLE_ATTRS_WORD2) > @@ -582,7 +641,7 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create > return status; > > status = nfsd4_decode_fattr(argp, create->cr_bmval, &create->cr_iattr, > - &create->cr_acl); > + &create->cr_acl, &create->cr_label); > if (status) > goto out; > > @@ -832,7 +891,7 @@ nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open) > case NFS4_CREATE_UNCHECKED: > case NFS4_CREATE_GUARDED: > status = nfsd4_decode_fattr(argp, open->op_bmval, > - &open->op_iattr, &open->op_acl); > + &open->op_iattr, &open->op_acl, &open->op_label); > if (status) > goto out; > break; > @@ -846,7 +905,7 @@ nfsd4_decode_open(struct nfsd4_compoundargs *argp, struct nfsd4_open *open) > READ_BUF(NFS4_VERIFIER_SIZE); > COPYMEM(open->op_verf.data, NFS4_VERIFIER_SIZE); > status = nfsd4_decode_fattr(argp, open->op_bmval, > - &open->op_iattr, &open->op_acl); > + &open->op_iattr, &open->op_acl, &open->op_label); > if (status) > goto out; > break; > @@ -1068,7 +1127,7 @@ nfsd4_decode_setattr(struct nfsd4_compoundargs *argp, struct nfsd4_setattr *seta > if (status) > return status; > return nfsd4_decode_fattr(argp, setattr->sa_bmval, &setattr->sa_iattr, > - &setattr->sa_acl); > + &setattr->sa_acl, &setattr->sa_label); > } > > static __be32 > @@ -1989,6 +2048,50 @@ nfsd4_encode_aclname(struct svc_rqst *rqstp, struct nfs4_ace *ace, > FATTR4_WORD0_RDATTR_ERROR) > #define WORD1_ABSENT_FS_ATTRS FATTR4_WORD1_MOUNTED_ON_FILEID > > +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL > +static inline __be32 > +nfsd4_encode_security_label(struct svc_rqst *rqstp, struct dentry *dentry, __be32 **pp, int *buflen) > +{ > + void *context; > + int err; > + int len; > + uint32_t pi = 0; > + uint32_t lfs = 0; > + __be32 *p = *pp; > + > + err = 0; > + (void)security_inode_getsecctx(dentry->d_inode, &context, &len); > + if (len < 0) > + return nfserrno(len); > + > + if (*buflen < ((XDR_QUADLEN(len) << 2) + 4 + 4 + 4)) { > + err = nfserr_resource; > + goto out; > + } > + > + /* > + * 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) > + return nfserr_resource; > + > + WRITE32(lfs); > + WRITE32(pi); > + p = xdr_encode_opaque(p, context, len); > + *buflen -= (XDR_QUADLEN(len) << 2) + 4; > + > + *pp = p; > +out: > + security_release_secctx(context, len); > + return err; > +} > +#else > +static inline __be32 > +nfsd4_encode_security_label(struct svc_rqst *rqstp, struct dentry *dentry, __be32 **pp, int *buflen) > +{ return 0; } > +#endif > + > static __be32 fattr_handle_absent_fs(u32 *bmval0, u32 *bmval1, u32 *rdattr_err) > { > /* As per referral draft: */ > @@ -2439,6 +2542,12 @@ out_acl: > get_parent_attributes(exp, &stat); > WRITE64(stat.ino); > } > + if (bmval2 & FATTR4_WORD2_SECURITY_LABEL) { > + status = nfsd4_encode_security_label(rqstp, dentry, > + &p, &buflen); > + if (status) > + goto out; > + } > if (bmval2 & FATTR4_WORD2_SUPPATTR_EXCLCREAT) { > WRITE32(3); > WRITE32(NFSD_SUPPATTR_EXCLCREAT_WORD0); > @@ -3226,16 +3335,18 @@ nfsd4_encode_setattr(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4 > { > __be32 *p; > > - RESERVE_SPACE(12); > + RESERVE_SPACE(16); > if (nfserr) { > - WRITE32(2); > + WRITE32(3); > + WRITE32(0); > WRITE32(0); > WRITE32(0); > } > else { > - WRITE32(2); > + WRITE32(3); > WRITE32(setattr->sa_bmval[0]); > WRITE32(setattr->sa_bmval[1]); > + WRITE32(setattr->sa_bmval[2]); > } > ADJUST_ARGS(); > return nfserr; > diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h > index 4c682fd..0a2e30c 100644 > --- a/fs/nfsd/nfsd.h > +++ b/fs/nfsd/nfsd.h > @@ -328,6 +328,14 @@ void nfsd_lockd_shutdown(void); > #define NFSD4_1_SUPPORTED_ATTRS_WORD2 \ > (NFSD4_SUPPORTED_ATTRS_WORD2 | FATTR4_WORD2_SUPPATTR_EXCLCREAT) > > +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL > +#define NFSD4_2_SUPPORTED_ATTRS_WORD2 \ > + (NFSD4_SUPPORTED_ATTRS_WORD2 | NFSD4_1_SUPPORTED_ATTRS_WORD2 | \ > + FATTR4_WORD2_SECURITY_LABEL) Note NFSD4_1_SUPPORTED_ATTRS_WORD2 was already defined to include NFSD4_SUPPORTED_ATTRS_WORD2, so the above is overkill. > +#else > +#define NFSD4_2_SUPPORTED_ATTRS_WORD2 0 > +#endif > + > static inline u32 nfsd_suppattrs0(u32 minorversion) > { > return minorversion ? NFSD4_1_SUPPORTED_ATTRS_WORD0 > @@ -342,6 +350,9 @@ static inline u32 nfsd_suppattrs1(u32 minorversion) > > static inline u32 nfsd_suppattrs2(u32 minorversion) > { > + if (minorversion == 2) > + return NFSD4_2_SUPPORTED_ATTRS_WORD2; > + > return minorversion ? NFSD4_1_SUPPORTED_ATTRS_WORD2 > : NFSD4_SUPPORTED_ATTRS_WORD2; > } > @@ -356,7 +367,11 @@ static inline u32 nfsd_suppattrs2(u32 minorversion) > #define NFSD_WRITEABLE_ATTRS_WORD1 \ > (FATTR4_WORD1_MODE | FATTR4_WORD1_OWNER | FATTR4_WORD1_OWNER_GROUP \ > | FATTR4_WORD1_TIME_ACCESS_SET | FATTR4_WORD1_TIME_MODIFY_SET) > +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL > +#define NFSD_WRITEABLE_ATTRS_WORD2 FATTR4_WORD2_SECURITY_LABEL > +#else > #define NFSD_WRITEABLE_ATTRS_WORD2 0 > +#endif > > #define NFSD_SUPPATTR_EXCLCREAT_WORD0 \ > NFSD_WRITEABLE_ATTRS_WORD0 > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > index 54c6b3d..85289a5 100644 > --- a/fs/nfsd/nfsproc.c > +++ b/fs/nfsd/nfsproc.c > @@ -743,6 +743,7 @@ nfserrno (int errno) > { nfserr_notsupp, -EOPNOTSUPP }, > { nfserr_toosmall, -ETOOSMALL }, > { nfserr_serverfault, -ESERVERFAULT }, > + { nfs4err_badlabel, -EMSGSIZE }, The only place we use this is to enforce the maximum label size in nfsd4_label_alloc, called only in decode_fattr. I'd actually rather enforce the maximum label size in decode_fattr, where we can easily return nfs4err_badlabel. And do away with this mapping. --b. > }; > int i; > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 2b2e239..96f9a82 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -28,6 +28,7 @@ > #include > #include > #include > +#include > > #ifdef CONFIG_NFSD_V3 > #include "xdr3.h" > @@ -621,6 +622,33 @@ int nfsd4_is_junction(struct dentry *dentry) > return 0; > return 1; > } > +#ifdef CONFIG_NFSD_V4_SECURITY_LABEL > +__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp, > + struct nfs4_label *label) > +{ > + __be32 error; > + int host_error; > + struct dentry *dentry; > + > + error = fh_verify(rqstp, fhp, 0 /* S_IFREG */, NFSD_MAY_SATTR); > + if (error) > + return error; > + > + dentry = fhp->fh_dentry; > + > + mutex_lock(&dentry->d_inode->i_mutex); > + host_error = security_inode_setsecctx(dentry, label->label, label->len); > + mutex_unlock(&dentry->d_inode->i_mutex); > + return nfserrno(host_error); > +} > +#else > +__be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp, > + struct nfs4_label *label) > +{ > + return nfserr_notsupp; > +} > +#endif > + > #endif /* defined(CONFIG_NFSD_V4) */ > > #ifdef CONFIG_NFSD_V3 > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > index 5b58941..a9d6f0f 100644 > --- a/fs/nfsd/vfs.h > +++ b/fs/nfsd/vfs.h > @@ -56,6 +56,8 @@ int nfsd_mountpoint(struct dentry *, struct svc_export *); > __be32 nfsd4_set_nfs4_acl(struct svc_rqst *, struct svc_fh *, > struct nfs4_acl *); > int nfsd4_get_nfs4_acl(struct svc_rqst *, struct dentry *, struct nfs4_acl **); > +__be32 nfsd4_set_nfs4_label(struct svc_rqst *, struct svc_fh *, > + struct nfs4_label *); > #endif /* CONFIG_NFSD_V4 */ > __be32 nfsd_create(struct svc_rqst *, struct svc_fh *, > char *name, int len, struct iattr *attrs, > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > index 546f898..132a671 100644 > --- a/fs/nfsd/xdr4.h > +++ b/fs/nfsd/xdr4.h > @@ -118,6 +118,7 @@ struct nfsd4_create { > struct iattr cr_iattr; /* request */ > struct nfsd4_change_info cr_cinfo; /* response */ > struct nfs4_acl *cr_acl; > + struct nfs4_label *cr_label; > }; > #define cr_linklen u.link.namelen > #define cr_linkname u.link.name > @@ -246,6 +247,7 @@ struct nfsd4_open { > struct nfs4_file *op_file; /* used during processing */ > struct nfs4_ol_stateid *op_stp; /* used during processing */ > struct nfs4_acl *op_acl; > + struct nfs4_label *op_label; > }; > #define op_iattr iattr > > @@ -330,6 +332,7 @@ struct nfsd4_setattr { > u32 sa_bmval[3]; /* request */ > struct iattr sa_iattr; /* request */ > struct nfs4_acl *sa_acl; > + struct nfs4_label *sa_label; > }; > > struct nfsd4_setclientid { > -- > 1.8.1.4 >