| Message ID | 20180626084306.27511-1-zyan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/26/2018 04:43 AM, Yan, Zheng wrote: > This is preparation for CephFS security label. CephFS's implementation uses > dentry_init_security() to get security context before inode is created, > then sends open/mkdir/mknod request to MDS, together with security xattr > "security.<security module name>" Can you describe how your approach compares to the NFSv4 labeling support, and why it requires exporting this information from this hook when NFSv4 did not? > > Signed-off-by: "Yan, Zheng" <zyan@redhat.com> > --- > fs/nfs/nfs4proc.c | 3 ++- > include/linux/lsm_hooks.h | 4 ++-- > include/linux/security.h | 9 +++++---- > security/security.c | 7 ++++--- > security/selinux/hooks.c | 8 ++++++-- > 5 files changed, 19 insertions(+), 12 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 6dd146885da9..d18a5fb7aec3 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -122,7 +122,8 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > return NULL; > > err = security_dentry_init_security(dentry, sattr->ia_mode, > - &dentry->d_name, (void **)&label->label, &label->len); > + &dentry->d_name, NULL, > + (void **)&label->label, &label->len); > if (err == 0) > return label; > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 8f1131c8dd54..e176c2032bdc 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -1476,8 +1476,8 @@ union security_list_options { > unsigned long *set_kern_flags); > int (*sb_parse_opts_str)(char *options, struct security_mnt_opts *opts); > int (*dentry_init_security)(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen); > + const struct qstr *name, const char **label, Seems like "label" could be confusing given that it means something else in the NFSv4 code, and what is actually being provided here is the xattr name suffix. > + void **ctx, u32 *ctxlen); > int (*dentry_create_files_as)(struct dentry *dentry, int mode, > struct qstr *name, > const struct cred *old, > diff --git a/include/linux/security.h b/include/linux/security.h > index 63030c85ee19..df2d73998c64 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -246,8 +246,9 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, > unsigned long *set_kern_flags); > int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts); > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen); > + const struct qstr *name, > + const char **label, > + void **ctx, u32 *ctxlen); > int security_dentry_create_files_as(struct dentry *dentry, int mode, > struct qstr *name, > const struct cred *old, > @@ -609,8 +610,8 @@ static inline void security_inode_free(struct inode *inode) > static inline int security_dentry_init_security(struct dentry *dentry, > int mode, > const struct qstr *name, > - void **ctx, > - u32 *ctxlen) > + const char **label, > + void **ctx, u32 *ctxlen) > { > return -EOPNOTSUPP; > } > diff --git a/security/security.c b/security/security.c > index 68f46d849abe..69818d46aa28 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -450,11 +450,12 @@ void security_inode_free(struct inode *inode) > } > > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen) > + const struct qstr *name, > + const char **label, > + void **ctx, u32 *ctxlen) > { > return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, > - name, ctx, ctxlen); > + name, label, ctx, ctxlen); > } > EXPORT_SYMBOL(security_dentry_init_security); > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 2b5ee5fbd652..eca3879d9357 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2985,8 +2985,9 @@ static void selinux_inode_free_security(struct inode *inode) > } > > static int selinux_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen) > + const struct qstr *name, > + const char **label, > + void **ctx, u32 *ctxlen) > { > u32 newsid; > int rc; > @@ -2998,6 +2999,9 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode, > if (rc) > return rc; > > + if (label) > + *label = XATTR_SELINUX_SUFFIX; > + > return security_sid_to_context(&selinux_state, newsid, (char **)ctx, > ctxlen); > } > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Jun 26, 2018, at 21:28, Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On 06/26/2018 04:43 AM, Yan, Zheng wrote: >> This is preparation for CephFS security label. CephFS's implementation uses >> dentry_init_security() to get security context before inode is created, >> then sends open/mkdir/mknod request to MDS, together with security xattr >> "security.<security module name>" > > Can you describe how your approach compares to the NFSv4 labeling support, and why it requires > exporting this information from this hook when NFSv4 did not? NFS client only support single security label, it passes unnamed security context to NFSD. NFSD stores the security context by calling security_inode_setsecctx(). For selinux, the security context is stored in xattr ‘security.selinux’. CephFS does not exports other filesystems. So It's irrelevant to CephFS which security module is enabled on the host that runs ceph-mds (counterpart of NFSD) Regards Yan, Zheng >> >> Signed-off-by: "Yan, Zheng" <zyan@redhat.com> >> --- >> fs/nfs/nfs4proc.c | 3 ++- >> include/linux/lsm_hooks.h | 4 ++-- >> include/linux/security.h | 9 +++++---- >> security/security.c | 7 ++++--- >> security/selinux/hooks.c | 8 ++++++-- >> 5 files changed, 19 insertions(+), 12 deletions(-) >> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index 6dd146885da9..d18a5fb7aec3 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -122,7 +122,8 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >> return NULL; >> >> err = security_dentry_init_security(dentry, sattr->ia_mode, >> - &dentry->d_name, (void **)&label->label, &label->len); >> + &dentry->d_name, NULL, >> + (void **)&label->label, &label->len); >> if (err == 0) >> return label; >> >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> index 8f1131c8dd54..e176c2032bdc 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -1476,8 +1476,8 @@ union security_list_options { >> unsigned long *set_kern_flags); >> int (*sb_parse_opts_str)(char *options, struct security_mnt_opts *opts); >> int (*dentry_init_security)(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen); >> + const struct qstr *name, const char **label, > > Seems like "label" could be confusing given that it means something else in the NFSv4 code, > and what is actually being provided here is the xattr name suffix. > >> + void **ctx, u32 *ctxlen); >> int (*dentry_create_files_as)(struct dentry *dentry, int mode, >> struct qstr *name, >> const struct cred *old, >> diff --git a/include/linux/security.h b/include/linux/security.h >> index 63030c85ee19..df2d73998c64 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -246,8 +246,9 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, >> unsigned long *set_kern_flags); >> int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts); >> int security_dentry_init_security(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen); >> + const struct qstr *name, >> + const char **label, >> + void **ctx, u32 *ctxlen); >> int security_dentry_create_files_as(struct dentry *dentry, int mode, >> struct qstr *name, >> const struct cred *old, >> @@ -609,8 +610,8 @@ static inline void security_inode_free(struct inode *inode) >> static inline int security_dentry_init_security(struct dentry *dentry, >> int mode, >> const struct qstr *name, >> - void **ctx, >> - u32 *ctxlen) >> + const char **label, >> + void **ctx, u32 *ctxlen) >> { >> return -EOPNOTSUPP; >> } >> diff --git a/security/security.c b/security/security.c >> index 68f46d849abe..69818d46aa28 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -450,11 +450,12 @@ void security_inode_free(struct inode *inode) >> } >> >> int security_dentry_init_security(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen) >> + const struct qstr *name, >> + const char **label, >> + void **ctx, u32 *ctxlen) >> { >> return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, >> - name, ctx, ctxlen); >> + name, label, ctx, ctxlen); >> } >> EXPORT_SYMBOL(security_dentry_init_security); >> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index 2b5ee5fbd652..eca3879d9357 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -2985,8 +2985,9 @@ static void selinux_inode_free_security(struct inode *inode) >> } >> >> static int selinux_dentry_init_security(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen) >> + const struct qstr *name, >> + const char **label, >> + void **ctx, u32 *ctxlen) >> { >> u32 newsid; >> int rc; >> @@ -2998,6 +2999,9 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode, >> if (rc) >> return rc; >> >> + if (label) >> + *label = XATTR_SELINUX_SUFFIX; >> + >> return security_sid_to_context(&selinux_state, newsid, (char **)ctx, >> ctxlen); >> } >> > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/26/2018 1:43 AM, Yan, Zheng wrote: > This is preparation for CephFS security label. CephFS's implementation uses > dentry_init_security() to get security context before inode is created, > then sends open/mkdir/mknod request to MDS, together with security xattr > "security.<security module name>" There is no requirement that a security module use "security.<security module name>" to name it's attributes. Smack, for example, uses "security.SMACK64". Further, Smack uses multiple attributes; "security.SMACK64TRANSMUTE", "security.SMACK64MMAP", "security.SMACK64EXEC" as well as "security.SMACK64". Also, when (if) we get full module stacking in place you will need to handle the possibility that there may be more than one security module providing security xattrs. > > Signed-off-by: "Yan, Zheng" <zyan@redhat.com> > --- > fs/nfs/nfs4proc.c | 3 ++- > include/linux/lsm_hooks.h | 4 ++-- > include/linux/security.h | 9 +++++---- > security/security.c | 7 ++++--- > security/selinux/hooks.c | 8 ++++++-- > 5 files changed, 19 insertions(+), 12 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 6dd146885da9..d18a5fb7aec3 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -122,7 +122,8 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > return NULL; > > err = security_dentry_init_security(dentry, sattr->ia_mode, > - &dentry->d_name, (void **)&label->label, &label->len); > + &dentry->d_name, NULL, > + (void **)&label->label, &label->len); > if (err == 0) > return label; > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 8f1131c8dd54..e176c2032bdc 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -1476,8 +1476,8 @@ union security_list_options { > unsigned long *set_kern_flags); > int (*sb_parse_opts_str)(char *options, struct security_mnt_opts *opts); > int (*dentry_init_security)(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen); > + const struct qstr *name, const char **label, > + void **ctx, u32 *ctxlen); > int (*dentry_create_files_as)(struct dentry *dentry, int mode, > struct qstr *name, > const struct cred *old, > diff --git a/include/linux/security.h b/include/linux/security.h > index 63030c85ee19..df2d73998c64 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -246,8 +246,9 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, > unsigned long *set_kern_flags); > int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts); > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen); > + const struct qstr *name, > + const char **label, > + void **ctx, u32 *ctxlen); > int security_dentry_create_files_as(struct dentry *dentry, int mode, > struct qstr *name, > const struct cred *old, > @@ -609,8 +610,8 @@ static inline void security_inode_free(struct inode *inode) > static inline int security_dentry_init_security(struct dentry *dentry, > int mode, > const struct qstr *name, > - void **ctx, > - u32 *ctxlen) > + const char **label, > + void **ctx, u32 *ctxlen) > { > return -EOPNOTSUPP; > } > diff --git a/security/security.c b/security/security.c > index 68f46d849abe..69818d46aa28 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -450,11 +450,12 @@ void security_inode_free(struct inode *inode) > } > > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen) > + const struct qstr *name, > + const char **label, > + void **ctx, u32 *ctxlen) > { > return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, > - name, ctx, ctxlen); > + name, label, ctx, ctxlen); > } > EXPORT_SYMBOL(security_dentry_init_security); > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 2b5ee5fbd652..eca3879d9357 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2985,8 +2985,9 @@ static void selinux_inode_free_security(struct inode *inode) > } > > static int selinux_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen) > + const struct qstr *name, > + const char **label, > + void **ctx, u32 *ctxlen) > { > u32 newsid; > int rc; > @@ -2998,6 +2999,9 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode, > if (rc) > return rc; > > + if (label) > + *label = XATTR_SELINUX_SUFFIX; > + > return security_sid_to_context(&selinux_state, newsid, (char **)ctx, > ctxlen); > } -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Jun 27, 2018, at 00:21, Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 6/26/2018 1:43 AM, Yan, Zheng wrote: >> This is preparation for CephFS security label. CephFS's implementation uses >> dentry_init_security() to get security context before inode is created, >> then sends open/mkdir/mknod request to MDS, together with security xattr >> "security.<security module name>" > > There is no requirement that a security module use > "security.<security module name>" to name it's attributes. > Smack, for example, uses "security.SMACK64”. This does not conflict with this patch. If smack implements dentry_init_security(), if can return whatever it likes. > Further, Smack uses multiple attributes; "security.SMACK64TRANSMUTE", > "security.SMACK64MMAP", "security.SMACK64EXEC" as well as > "security.SMACK64”. > > Also, when (if) we get full module stacking in place you > will need to handle the possibility that there may be more > than one security module providing security xattrs. > We can introduce another type of “dentry_init_security”, which return a xattr array. All cephfs wants is a function to get security xattrs before inode is created. It does not matter if the function return one xattr or multiple xattrs. Regards Yan, Zheng >> >> Signed-off-by: "Yan, Zheng" <zyan@redhat.com> >> --- >> fs/nfs/nfs4proc.c | 3 ++- >> include/linux/lsm_hooks.h | 4 ++-- >> include/linux/security.h | 9 +++++---- >> security/security.c | 7 ++++--- >> security/selinux/hooks.c | 8 ++++++-- >> 5 files changed, 19 insertions(+), 12 deletions(-) >> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index 6dd146885da9..d18a5fb7aec3 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -122,7 +122,8 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >> return NULL; >> >> err = security_dentry_init_security(dentry, sattr->ia_mode, >> - &dentry->d_name, (void **)&label->label, &label->len); >> + &dentry->d_name, NULL, >> + (void **)&label->label, &label->len); >> if (err == 0) >> return label; >> >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> index 8f1131c8dd54..e176c2032bdc 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -1476,8 +1476,8 @@ union security_list_options { >> unsigned long *set_kern_flags); >> int (*sb_parse_opts_str)(char *options, struct security_mnt_opts *opts); >> int (*dentry_init_security)(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen); >> + const struct qstr *name, const char **label, >> + void **ctx, u32 *ctxlen); >> int (*dentry_create_files_as)(struct dentry *dentry, int mode, >> struct qstr *name, >> const struct cred *old, >> diff --git a/include/linux/security.h b/include/linux/security.h >> index 63030c85ee19..df2d73998c64 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -246,8 +246,9 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, >> unsigned long *set_kern_flags); >> int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts); >> int security_dentry_init_security(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen); >> + const struct qstr *name, >> + const char **label, >> + void **ctx, u32 *ctxlen); >> int security_dentry_create_files_as(struct dentry *dentry, int mode, >> struct qstr *name, >> const struct cred *old, >> @@ -609,8 +610,8 @@ static inline void security_inode_free(struct inode *inode) >> static inline int security_dentry_init_security(struct dentry *dentry, >> int mode, >> const struct qstr *name, >> - void **ctx, >> - u32 *ctxlen) >> + const char **label, >> + void **ctx, u32 *ctxlen) >> { >> return -EOPNOTSUPP; >> } >> diff --git a/security/security.c b/security/security.c >> index 68f46d849abe..69818d46aa28 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -450,11 +450,12 @@ void security_inode_free(struct inode *inode) >> } >> >> int security_dentry_init_security(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen) >> + const struct qstr *name, >> + const char **label, >> + void **ctx, u32 *ctxlen) >> { >> return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, >> - name, ctx, ctxlen); >> + name, label, ctx, ctxlen); >> } >> EXPORT_SYMBOL(security_dentry_init_security); >> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index 2b5ee5fbd652..eca3879d9357 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -2985,8 +2985,9 @@ static void selinux_inode_free_security(struct inode *inode) >> } >> >> static int selinux_dentry_init_security(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen) >> + const struct qstr *name, >> + const char **label, >> + void **ctx, u32 *ctxlen) >> { >> u32 newsid; >> int rc; >> @@ -2998,6 +2999,9 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode, >> if (rc) >> return rc; >> >> + if (label) >> + *label = XATTR_SELINUX_SUFFIX; >> + >> return security_sid_to_context(&selinux_state, newsid, (char **)ctx, >> ctxlen); >> } > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2018-06-26 at 16:43 +0800, Yan, Zheng wrote: > This is preparation for CephFS security label. CephFS's implementation uses > dentry_init_security() to get security context before inode is created, > then sends open/mkdir/mknod request to MDS, together with security xattr > "security.<security module name>" > > Signed-off-by: "Yan, Zheng" <zyan@redhat.com> > --- > fs/nfs/nfs4proc.c | 3 ++- > include/linux/lsm_hooks.h | 4 ++-- > include/linux/security.h | 9 +++++---- > security/security.c | 7 ++++--- > security/selinux/hooks.c | 8 ++++++-- > 5 files changed, 19 insertions(+), 12 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 6dd146885da9..d18a5fb7aec3 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -122,7 +122,8 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > return NULL; > > err = security_dentry_init_security(dentry, sattr->ia_mode, > - &dentry->d_name, (void **)&label->label, &label->len); > + &dentry->d_name, NULL, > + (void **)&label->label, &label->len); > if (err == 0) > return label; > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 8f1131c8dd54..e176c2032bdc 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -1476,8 +1476,8 @@ union security_list_options { > unsigned long *set_kern_flags); > int (*sb_parse_opts_str)(char *options, struct security_mnt_opts *opts); > int (*dentry_init_security)(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen); > + const struct qstr *name, const char **label, > + void **ctx, u32 *ctxlen); > int (*dentry_create_files_as)(struct dentry *dentry, int mode, > struct qstr *name, > const struct cred *old, > diff --git a/include/linux/security.h b/include/linux/security.h > index 63030c85ee19..df2d73998c64 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -246,8 +246,9 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, > unsigned long *set_kern_flags); > int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts); > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen); > + const struct qstr *name, > + const char **label, > + void **ctx, u32 *ctxlen); > int security_dentry_create_files_as(struct dentry *dentry, int mode, > struct qstr *name, > const struct cred *old, > @@ -609,8 +610,8 @@ static inline void security_inode_free(struct inode *inode) > static inline int security_dentry_init_security(struct dentry *dentry, > int mode, > const struct qstr *name, > - void **ctx, > - u32 *ctxlen) > + const char **label, > + void **ctx, u32 *ctxlen) > { > return -EOPNOTSUPP; > } > diff --git a/security/security.c b/security/security.c > index 68f46d849abe..69818d46aa28 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -450,11 +450,12 @@ void security_inode_free(struct inode *inode) > } > > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen) > + const struct qstr *name, > + const char **label, > + void **ctx, u32 *ctxlen) > { > return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, > - name, ctx, ctxlen); > + name, label, ctx, ctxlen); > } > EXPORT_SYMBOL(security_dentry_init_security); > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 2b5ee5fbd652..eca3879d9357 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2985,8 +2985,9 @@ static void selinux_inode_free_security(struct inode *inode) > } > > static int selinux_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen) > + const struct qstr *name, > + const char **label, > + void **ctx, u32 *ctxlen) > { > u32 newsid; > int rc; > @@ -2998,6 +2999,9 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode, > if (rc) > return rc; > > + if (label) > + *label = XATTR_SELINUX_SUFFIX; > + nit: I'd probably call this "name" since that's what it's called in selinux_inode_init_security. "label" has a different connotation in this context. > return security_sid_to_context(&selinux_state, newsid, (char **)ctx, > ctxlen); > } Patch looks reasonable to me though. Acked-by: Jeff Layton <jlayton@kernel.org>
On 9/6/2018 8:08 AM, Jeff Layton wrote: > On Tue, 2018-06-26 at 16:43 +0800, Yan, Zheng wrote: >> This is preparation for CephFS security label. CephFS's implementation uses >> dentry_init_security() to get security context before inode is created, >> then sends open/mkdir/mknod request to MDS, together with security xattr >> "security.<security module name>" Please excuse my late entry into this review. First, *Do not prefix general LSM interface work with "selinux:"* You should only use that prefix when you're dealing with the internals of SELinux. Use "LSM:" if you are changing LSM interfaces. If you are changing security.h or lsm_hooks.h you are almost certainly changing LSM interfaces. Because you prefixed this work with "selinux:" I pretty much ignored it. Please don't do that again. Why aren't you using security_inode_getsecctx, like kernfs and nfs? >> >> Signed-off-by: "Yan, Zheng" <zyan@redhat.com> >> --- >> fs/nfs/nfs4proc.c | 3 ++- >> include/linux/lsm_hooks.h | 4 ++-- >> include/linux/security.h | 9 +++++---- >> security/security.c | 7 ++++--- >> security/selinux/hooks.c | 8 ++++++-- >> 5 files changed, 19 insertions(+), 12 deletions(-) >> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index 6dd146885da9..d18a5fb7aec3 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -122,7 +122,8 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >> return NULL; >> >> err = security_dentry_init_security(dentry, sattr->ia_mode, >> - &dentry->d_name, (void **)&label->label, &label->len); >> + &dentry->d_name, NULL, >> + (void **)&label->label, &label->len); >> if (err == 0) >> return label; >> >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> index 8f1131c8dd54..e176c2032bdc 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -1476,8 +1476,8 @@ union security_list_options { >> unsigned long *set_kern_flags); >> int (*sb_parse_opts_str)(char *options, struct security_mnt_opts *opts); >> int (*dentry_init_security)(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen); >> + const struct qstr *name, const char **label, >> + void **ctx, u32 *ctxlen); >> int (*dentry_create_files_as)(struct dentry *dentry, int mode, >> struct qstr *name, >> const struct cred *old, >> diff --git a/include/linux/security.h b/include/linux/security.h >> index 63030c85ee19..df2d73998c64 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -246,8 +246,9 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, >> unsigned long *set_kern_flags); >> int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts); >> int security_dentry_init_security(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen); >> + const struct qstr *name, >> + const char **label, >> + void **ctx, u32 *ctxlen); >> int security_dentry_create_files_as(struct dentry *dentry, int mode, >> struct qstr *name, >> const struct cred *old, >> @@ -609,8 +610,8 @@ static inline void security_inode_free(struct inode *inode) >> static inline int security_dentry_init_security(struct dentry *dentry, >> int mode, >> const struct qstr *name, >> - void **ctx, >> - u32 *ctxlen) >> + const char **label, >> + void **ctx, u32 *ctxlen) >> { >> return -EOPNOTSUPP; >> } >> diff --git a/security/security.c b/security/security.c >> index 68f46d849abe..69818d46aa28 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -450,11 +450,12 @@ void security_inode_free(struct inode *inode) >> } >> >> int security_dentry_init_security(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen) >> + const struct qstr *name, >> + const char **label, >> + void **ctx, u32 *ctxlen) >> { >> return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, >> - name, ctx, ctxlen); >> + name, label, ctx, ctxlen); >> } >> EXPORT_SYMBOL(security_dentry_init_security); >> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index 2b5ee5fbd652..eca3879d9357 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -2985,8 +2985,9 @@ static void selinux_inode_free_security(struct inode *inode) >> } >> >> static int selinux_dentry_init_security(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen) >> + const struct qstr *name, >> + const char **label, >> + void **ctx, u32 *ctxlen) >> { >> u32 newsid; >> int rc; >> @@ -2998,6 +2999,9 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode, >> if (rc) >> return rc; >> >> + if (label) >> + *label = XATTR_SELINUX_SUFFIX; >> + > nit: I'd probably call this "name" since that's what it's called in > selinux_inode_init_security. "label" has a different connotation in this > context. > >> return security_sid_to_context(&selinux_state, newsid, (char **)ctx, >> ctxlen); >> } > Patch looks reasonable to me though. > > Acked-by: Jeff Layton <jlayton@kernel.org> > >
On Thu, Sep 6, 2018 at 11:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 9/6/2018 8:08 AM, Jeff Layton wrote: > > On Tue, 2018-06-26 at 16:43 +0800, Yan, Zheng wrote: > >> This is preparation for CephFS security label. CephFS's implementation uses > >> dentry_init_security() to get security context before inode is created, > >> then sends open/mkdir/mknod request to MDS, together with security xattr > >> "security.<security module name>" > > Please excuse my late entry into this review. > > First, *Do not prefix general LSM interface work with "selinux:"* > You should only use that prefix when you're dealing with the > internals of SELinux. Use "LSM:" if you are changing LSM interfaces. > If you are changing security.h or lsm_hooks.h you are almost > certainly changing LSM interfaces. > > Because you prefixed this work with "selinux:" I pretty much > ignored it. Please don't do that again. > > Why aren't you using security_inode_getsecctx, like kernfs and nfs? > Sorry, I don't understand why you mention security_inode_getsecctx(). The selinux_dentry_init_security is called for new inode, I don't how security_inode_getsecctx() can help. Regards Yan, Zheng > > >> > >> Signed-off-by: "Yan, Zheng" <zyan@redhat.com> > >> --- > >> fs/nfs/nfs4proc.c | 3 ++- > >> include/linux/lsm_hooks.h | 4 ++-- > >> include/linux/security.h | 9 +++++---- > >> security/security.c | 7 ++++--- > >> security/selinux/hooks.c | 8 ++++++-- > >> 5 files changed, 19 insertions(+), 12 deletions(-) > >> > >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > >> index 6dd146885da9..d18a5fb7aec3 100644 > >> --- a/fs/nfs/nfs4proc.c > >> +++ b/fs/nfs/nfs4proc.c > >> @@ -122,7 +122,8 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > >> return NULL; > >> > >> err = security_dentry_init_security(dentry, sattr->ia_mode, > >> - &dentry->d_name, (void **)&label->label, &label->len); > >> + &dentry->d_name, NULL, > >> + (void **)&label->label, &label->len); > >> if (err == 0) > >> return label; > >> > >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > >> index 8f1131c8dd54..e176c2032bdc 100644 > >> --- a/include/linux/lsm_hooks.h > >> +++ b/include/linux/lsm_hooks.h > >> @@ -1476,8 +1476,8 @@ union security_list_options { > >> unsigned long *set_kern_flags); > >> int (*sb_parse_opts_str)(char *options, struct security_mnt_opts *opts); > >> int (*dentry_init_security)(struct dentry *dentry, int mode, > >> - const struct qstr *name, void **ctx, > >> - u32 *ctxlen); > >> + const struct qstr *name, const char **label, > >> + void **ctx, u32 *ctxlen); > >> int (*dentry_create_files_as)(struct dentry *dentry, int mode, > >> struct qstr *name, > >> const struct cred *old, > >> diff --git a/include/linux/security.h b/include/linux/security.h > >> index 63030c85ee19..df2d73998c64 100644 > >> --- a/include/linux/security.h > >> +++ b/include/linux/security.h > >> @@ -246,8 +246,9 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, > >> unsigned long *set_kern_flags); > >> int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts); > >> int security_dentry_init_security(struct dentry *dentry, int mode, > >> - const struct qstr *name, void **ctx, > >> - u32 *ctxlen); > >> + const struct qstr *name, > >> + const char **label, > >> + void **ctx, u32 *ctxlen); > >> int security_dentry_create_files_as(struct dentry *dentry, int mode, > >> struct qstr *name, > >> const struct cred *old, > >> @@ -609,8 +610,8 @@ static inline void security_inode_free(struct inode *inode) > >> static inline int security_dentry_init_security(struct dentry *dentry, > >> int mode, > >> const struct qstr *name, > >> - void **ctx, > >> - u32 *ctxlen) > >> + const char **label, > >> + void **ctx, u32 *ctxlen) > >> { > >> return -EOPNOTSUPP; > >> } > >> diff --git a/security/security.c b/security/security.c > >> index 68f46d849abe..69818d46aa28 100644 > >> --- a/security/security.c > >> +++ b/security/security.c > >> @@ -450,11 +450,12 @@ void security_inode_free(struct inode *inode) > >> } > >> > >> int security_dentry_init_security(struct dentry *dentry, int mode, > >> - const struct qstr *name, void **ctx, > >> - u32 *ctxlen) > >> + const struct qstr *name, > >> + const char **label, > >> + void **ctx, u32 *ctxlen) > >> { > >> return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, > >> - name, ctx, ctxlen); > >> + name, label, ctx, ctxlen); > >> } > >> EXPORT_SYMBOL(security_dentry_init_security); > >> > >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >> index 2b5ee5fbd652..eca3879d9357 100644 > >> --- a/security/selinux/hooks.c > >> +++ b/security/selinux/hooks.c > >> @@ -2985,8 +2985,9 @@ static void selinux_inode_free_security(struct inode *inode) > >> } > >> > >> static int selinux_dentry_init_security(struct dentry *dentry, int mode, > >> - const struct qstr *name, void **ctx, > >> - u32 *ctxlen) > >> + const struct qstr *name, > >> + const char **label, > >> + void **ctx, u32 *ctxlen) > >> { > >> u32 newsid; > >> int rc; > >> @@ -2998,6 +2999,9 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode, > >> if (rc) > >> return rc; > >> > >> + if (label) > >> + *label = XATTR_SELINUX_SUFFIX; > >> + > > nit: I'd probably call this "name" since that's what it's called in > > selinux_inode_init_security. "label" has a different connotation in this > > context. > > > >> return security_sid_to_context(&selinux_state, newsid, (char **)ctx, > >> ctxlen); > >> } > > Patch looks reasonable to me though. > > > > Acked-by: Jeff Layton <jlayton@kernel.org> > > > > >
On 9/7/2018 1:31 AM, Yan, Zheng wrote: > On Thu, Sep 6, 2018 at 11:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 9/6/2018 8:08 AM, Jeff Layton wrote: >>> On Tue, 2018-06-26 at 16:43 +0800, Yan, Zheng wrote: >>>> This is preparation for CephFS security label. CephFS's implementation uses >>>> dentry_init_security() to get security context before inode is created, >>>> then sends open/mkdir/mknod request to MDS, together with security xattr >>>> "security.<security module name>" >> Please excuse my late entry into this review. >> >> First, *Do not prefix general LSM interface work with "selinux:"* >> You should only use that prefix when you're dealing with the >> internals of SELinux. Use "LSM:" if you are changing LSM interfaces. >> If you are changing security.h or lsm_hooks.h you are almost >> certainly changing LSM interfaces. >> >> Because you prefixed this work with "selinux:" I pretty much >> ignored it. Please don't do that again. >> >> Why aren't you using security_inode_getsecctx, like kernfs and nfs? >> > Sorry, I don't understand why you mention security_inode_getsecctx(). > The selinux_dentry_init_security is called for new inode, I don't how > security_inode_getsecctx() can help. I didn't do a great job describing my issue. Let me try again. There is no way you should need to know the name of the security module (e.g. SELinux, Smack) in the CephFS code. Nor should you be assuming that there is only one security attribute, nor that it is named "security.<security module name>". Sure, that's true for SELinux, but SELinux isn't the only security module in town. Other filesystems don't need anything like this. Why do you? I will leave the implications of having multiple security modules with security attributes aside because support for that is a work in progress. Leave it to say that other filesystem will have no problem with it at all. > > Regards > Yan, Zheng > > > >>>> Signed-off-by: "Yan, Zheng" <zyan@redhat.com> >>>> --- >>>> fs/nfs/nfs4proc.c | 3 ++- >>>> include/linux/lsm_hooks.h | 4 ++-- >>>> include/linux/security.h | 9 +++++---- >>>> security/security.c | 7 ++++--- >>>> security/selinux/hooks.c | 8 ++++++-- >>>> 5 files changed, 19 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >>>> index 6dd146885da9..d18a5fb7aec3 100644 >>>> --- a/fs/nfs/nfs4proc.c >>>> +++ b/fs/nfs/nfs4proc.c >>>> @@ -122,7 +122,8 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >>>> return NULL; >>>> >>>> err = security_dentry_init_security(dentry, sattr->ia_mode, >>>> - &dentry->d_name, (void **)&label->label, &label->len); >>>> + &dentry->d_name, NULL, >>>> + (void **)&label->label, &label->len); >>>> if (err == 0) >>>> return label; >>>> >>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>>> index 8f1131c8dd54..e176c2032bdc 100644 >>>> --- a/include/linux/lsm_hooks.h >>>> +++ b/include/linux/lsm_hooks.h >>>> @@ -1476,8 +1476,8 @@ union security_list_options { >>>> unsigned long *set_kern_flags); >>>> int (*sb_parse_opts_str)(char *options, struct security_mnt_opts *opts); >>>> int (*dentry_init_security)(struct dentry *dentry, int mode, >>>> - const struct qstr *name, void **ctx, >>>> - u32 *ctxlen); >>>> + const struct qstr *name, const char **label, >>>> + void **ctx, u32 *ctxlen); >>>> int (*dentry_create_files_as)(struct dentry *dentry, int mode, >>>> struct qstr *name, >>>> const struct cred *old, >>>> diff --git a/include/linux/security.h b/include/linux/security.h >>>> index 63030c85ee19..df2d73998c64 100644 >>>> --- a/include/linux/security.h >>>> +++ b/include/linux/security.h >>>> @@ -246,8 +246,9 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, >>>> unsigned long *set_kern_flags); >>>> int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts); >>>> int security_dentry_init_security(struct dentry *dentry, int mode, >>>> - const struct qstr *name, void **ctx, >>>> - u32 *ctxlen); >>>> + const struct qstr *name, >>>> + const char **label, >>>> + void **ctx, u32 *ctxlen); >>>> int security_dentry_create_files_as(struct dentry *dentry, int mode, >>>> struct qstr *name, >>>> const struct cred *old, >>>> @@ -609,8 +610,8 @@ static inline void security_inode_free(struct inode *inode) >>>> static inline int security_dentry_init_security(struct dentry *dentry, >>>> int mode, >>>> const struct qstr *name, >>>> - void **ctx, >>>> - u32 *ctxlen) >>>> + const char **label, >>>> + void **ctx, u32 *ctxlen) >>>> { >>>> return -EOPNOTSUPP; >>>> } >>>> diff --git a/security/security.c b/security/security.c >>>> index 68f46d849abe..69818d46aa28 100644 >>>> --- a/security/security.c >>>> +++ b/security/security.c >>>> @@ -450,11 +450,12 @@ void security_inode_free(struct inode *inode) >>>> } >>>> >>>> int security_dentry_init_security(struct dentry *dentry, int mode, >>>> - const struct qstr *name, void **ctx, >>>> - u32 *ctxlen) >>>> + const struct qstr *name, >>>> + const char **label, >>>> + void **ctx, u32 *ctxlen) >>>> { >>>> return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, >>>> - name, ctx, ctxlen); >>>> + name, label, ctx, ctxlen); >>>> } >>>> EXPORT_SYMBOL(security_dentry_init_security); >>>> >>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>>> index 2b5ee5fbd652..eca3879d9357 100644 >>>> --- a/security/selinux/hooks.c >>>> +++ b/security/selinux/hooks.c >>>> @@ -2985,8 +2985,9 @@ static void selinux_inode_free_security(struct inode *inode) >>>> } >>>> >>>> static int selinux_dentry_init_security(struct dentry *dentry, int mode, >>>> - const struct qstr *name, void **ctx, >>>> - u32 *ctxlen) >>>> + const struct qstr *name, >>>> + const char **label, >>>> + void **ctx, u32 *ctxlen) >>>> { >>>> u32 newsid; >>>> int rc; >>>> @@ -2998,6 +2999,9 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode, >>>> if (rc) >>>> return rc; >>>> >>>> + if (label) >>>> + *label = XATTR_SELINUX_SUFFIX; >>>> + >>> nit: I'd probably call this "name" since that's what it's called in >>> selinux_inode_init_security. "label" has a different connotation in this >>> context. >>> >>>> return security_sid_to_context(&selinux_state, newsid, (char **)ctx, >>>> ctxlen); >>>> } >>> Patch looks reasonable to me though. >>> >>> Acked-by: Jeff Layton <jlayton@kernel.org> >>> >>>
On Sat, Sep 8, 2018 at 4:31 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 9/7/2018 1:31 AM, Yan, Zheng wrote: > > On Thu, Sep 6, 2018 at 11:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> On 9/6/2018 8:08 AM, Jeff Layton wrote: > >>> On Tue, 2018-06-26 at 16:43 +0800, Yan, Zheng wrote: > >>>> This is preparation for CephFS security label. CephFS's implementation uses > >>>> dentry_init_security() to get security context before inode is created, > >>>> then sends open/mkdir/mknod request to MDS, together with security xattr > >>>> "security.<security module name>" > >> Please excuse my late entry into this review. > >> > >> First, *Do not prefix general LSM interface work with "selinux:"* > >> You should only use that prefix when you're dealing with the > >> internals of SELinux. Use "LSM:" if you are changing LSM interfaces. > >> If you are changing security.h or lsm_hooks.h you are almost > >> certainly changing LSM interfaces. > >> > >> Because you prefixed this work with "selinux:" I pretty much > >> ignored it. Please don't do that again. > >> > >> Why aren't you using security_inode_getsecctx, like kernfs and nfs? > >> > > Sorry, I don't understand why you mention security_inode_getsecctx(). > > The selinux_dentry_init_security is called for new inode, I don't how > > security_inode_getsecctx() can help. > > I didn't do a great job describing my issue. Let me try again. > > There is no way you should need to know the name of the security > module (e.g. SELinux, Smack) in the CephFS code. Nor should you be > assuming that there is only one security attribute, nor that it is > named "security.<security module name>". Sure, that's true for > SELinux, but SELinux isn't the only security module in town. > > Other filesystems don't need anything like this. Why do you? > What I want is a way to get security context before inode is created. The closest interface is security_dentry_init_security(). Currently the only NFS uses security_dentry_init_security() and only selinux has dentry_init_security callback. NFS store selinux context in its security label (NFS inode can only have one security label, security label can only store one security context). SO current NFS security label implementation can not support multiple security modules. I don't want to limit cephfs to single security module. So I make cephfs store security context in security.<security module name> xattr. When multiple security modules work is done in the future, cephfs can easily supports it. (introduce a new security_dentry_init_security() which return security contexts for all enabled security modules) Regards Yan, Zheng > I will leave the implications of having multiple security modules > with security attributes aside because support for that is a work > in progress. Leave it to say that other filesystem will have no > problem with it at all. > > > > > Regards > > Yan, Zheng > > > > > > > >>>> Signed-off-by: "Yan, Zheng" <zyan@redhat.com> > >>>> --- > >>>> fs/nfs/nfs4proc.c | 3 ++- > >>>> include/linux/lsm_hooks.h | 4 ++-- > >>>> include/linux/security.h | 9 +++++---- > >>>> security/security.c | 7 ++++--- > >>>> security/selinux/hooks.c | 8 ++++++-- > >>>> 5 files changed, 19 insertions(+), 12 deletions(-) > >>>> > >>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > >>>> index 6dd146885da9..d18a5fb7aec3 100644 > >>>> --- a/fs/nfs/nfs4proc.c > >>>> +++ b/fs/nfs/nfs4proc.c > >>>> @@ -122,7 +122,8 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > >>>> return NULL; > >>>> > >>>> err = security_dentry_init_security(dentry, sattr->ia_mode, > >>>> - &dentry->d_name, (void **)&label->label, &label->len); > >>>> + &dentry->d_name, NULL, > >>>> + (void **)&label->label, &label->len); > >>>> if (err == 0) > >>>> return label; > >>>> > >>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > >>>> index 8f1131c8dd54..e176c2032bdc 100644 > >>>> --- a/include/linux/lsm_hooks.h > >>>> +++ b/include/linux/lsm_hooks.h > >>>> @@ -1476,8 +1476,8 @@ union security_list_options { > >>>> unsigned long *set_kern_flags); > >>>> int (*sb_parse_opts_str)(char *options, struct security_mnt_opts *opts); > >>>> int (*dentry_init_security)(struct dentry *dentry, int mode, > >>>> - const struct qstr *name, void **ctx, > >>>> - u32 *ctxlen); > >>>> + const struct qstr *name, const char **label, > >>>> + void **ctx, u32 *ctxlen); > >>>> int (*dentry_create_files_as)(struct dentry *dentry, int mode, > >>>> struct qstr *name, > >>>> const struct cred *old, > >>>> diff --git a/include/linux/security.h b/include/linux/security.h > >>>> index 63030c85ee19..df2d73998c64 100644 > >>>> --- a/include/linux/security.h > >>>> +++ b/include/linux/security.h > >>>> @@ -246,8 +246,9 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, > >>>> unsigned long *set_kern_flags); > >>>> int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts); > >>>> int security_dentry_init_security(struct dentry *dentry, int mode, > >>>> - const struct qstr *name, void **ctx, > >>>> - u32 *ctxlen); > >>>> + const struct qstr *name, > >>>> + const char **label, > >>>> + void **ctx, u32 *ctxlen); > >>>> int security_dentry_create_files_as(struct dentry *dentry, int mode, > >>>> struct qstr *name, > >>>> const struct cred *old, > >>>> @@ -609,8 +610,8 @@ static inline void security_inode_free(struct inode *inode) > >>>> static inline int security_dentry_init_security(struct dentry *dentry, > >>>> int mode, > >>>> const struct qstr *name, > >>>> - void **ctx, > >>>> - u32 *ctxlen) > >>>> + const char **label, > >>>> + void **ctx, u32 *ctxlen) > >>>> { > >>>> return -EOPNOTSUPP; > >>>> } > >>>> diff --git a/security/security.c b/security/security.c > >>>> index 68f46d849abe..69818d46aa28 100644 > >>>> --- a/security/security.c > >>>> +++ b/security/security.c > >>>> @@ -450,11 +450,12 @@ void security_inode_free(struct inode *inode) > >>>> } > >>>> > >>>> int security_dentry_init_security(struct dentry *dentry, int mode, > >>>> - const struct qstr *name, void **ctx, > >>>> - u32 *ctxlen) > >>>> + const struct qstr *name, > >>>> + const char **label, > >>>> + void **ctx, u32 *ctxlen) > >>>> { > >>>> return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, > >>>> - name, ctx, ctxlen); > >>>> + name, label, ctx, ctxlen); > >>>> } > >>>> EXPORT_SYMBOL(security_dentry_init_security); > >>>> > >>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >>>> index 2b5ee5fbd652..eca3879d9357 100644 > >>>> --- a/security/selinux/hooks.c > >>>> +++ b/security/selinux/hooks.c > >>>> @@ -2985,8 +2985,9 @@ static void selinux_inode_free_security(struct inode *inode) > >>>> } > >>>> > >>>> static int selinux_dentry_init_security(struct dentry *dentry, int mode, > >>>> - const struct qstr *name, void **ctx, > >>>> - u32 *ctxlen) > >>>> + const struct qstr *name, > >>>> + const char **label, > >>>> + void **ctx, u32 *ctxlen) > >>>> { > >>>> u32 newsid; > >>>> int rc; > >>>> @@ -2998,6 +2999,9 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode, > >>>> if (rc) > >>>> return rc; > >>>> > >>>> + if (label) > >>>> + *label = XATTR_SELINUX_SUFFIX; > >>>> + > >>> nit: I'd probably call this "name" since that's what it's called in > >>> selinux_inode_init_security. "label" has a different connotation in this > >>> context. > >>> > >>>> return security_sid_to_context(&selinux_state, newsid, (char **)ctx, > >>>> ctxlen); > >>>> } > >>> Patch looks reasonable to me though. > >>> > >>> Acked-by: Jeff Layton <jlayton@kernel.org> > >>> > >>> >
On 9/9/2018 8:06 PM, Yan, Zheng wrote: > On Sat, Sep 8, 2018 at 4:31 AM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 9/7/2018 1:31 AM, Yan, Zheng wrote: >>> On Thu, Sep 6, 2018 at 11:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> On 9/6/2018 8:08 AM, Jeff Layton wrote: >>>>> On Tue, 2018-06-26 at 16:43 +0800, Yan, Zheng wrote: >>>>>> This is preparation for CephFS security label. CephFS's implementation uses >>>>>> dentry_init_security() to get security context before inode is created, >>>>>> then sends open/mkdir/mknod request to MDS, together with security xattr >>>>>> "security.<security module name>" >>>> Please excuse my late entry into this review. >>>> >>>> First, *Do not prefix general LSM interface work with "selinux:"* >>>> You should only use that prefix when you're dealing with the >>>> internals of SELinux. Use "LSM:" if you are changing LSM interfaces. >>>> If you are changing security.h or lsm_hooks.h you are almost >>>> certainly changing LSM interfaces. >>>> >>>> Because you prefixed this work with "selinux:" I pretty much >>>> ignored it. Please don't do that again. >>>> >>>> Why aren't you using security_inode_getsecctx, like kernfs and nfs? >>>> >>> Sorry, I don't understand why you mention security_inode_getsecctx(). >>> The selinux_dentry_init_security is called for new inode, I don't how >>> security_inode_getsecctx() can help. >> I didn't do a great job describing my issue. Let me try again. >> >> There is no way you should need to know the name of the security >> module (e.g. SELinux, Smack) in the CephFS code. Nor should you be >> assuming that there is only one security attribute, nor that it is >> named "security.<security module name>". Sure, that's true for >> SELinux, but SELinux isn't the only security module in town. >> >> Other filesystems don't need anything like this. Why do you? >> > What I want is a way to get security context before inode is created. What are you doing differently from the other filesystems? Why doesn't the security_d_instantiate mechanism work for you? > The closest interface is security_dentry_init_security(). Currently > the only NFS uses security_dentry_init_security() and only selinux has > dentry_init_security callback. NFS store selinux context in its > security label (NFS inode can only have one security label, security > label can only store one security context). SO current NFS security > label implementation can not support multiple security modules. That's correct. The labeled NFS is a very specific mechanism. It does not do anything except Mandatory Access Control labeling. It cannot be used for any other attributes. I am not a fan of the labeled NFS implementation, and was vocal about it at the time. > I don't want to limit cephfs to single security module. So I make > cephfs store security context in security.<security module name> > xattr. Please look at the mechanism used in kernfs. > When multiple security modules work is done in the future, > cephfs can easily supports it. (introduce a new > security_dentry_init_security() which return security contexts for all > enabled security modules) That is easier said than done. I've proposed several mechanisms, none of which are especially satisfactory. The best mechanism I see is the one used in kernfs, which is why I suggest you use that.
On Wed, Sep 12, 2018 at 1:23 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 9/9/2018 8:06 PM, Yan, Zheng wrote: > > On Sat, Sep 8, 2018 at 4:31 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> On 9/7/2018 1:31 AM, Yan, Zheng wrote: > >>> On Thu, Sep 6, 2018 at 11:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >>>> On 9/6/2018 8:08 AM, Jeff Layton wrote: > >>>>> On Tue, 2018-06-26 at 16:43 +0800, Yan, Zheng wrote: > >>>>>> This is preparation for CephFS security label. CephFS's implementation uses > >>>>>> dentry_init_security() to get security context before inode is created, > >>>>>> then sends open/mkdir/mknod request to MDS, together with security xattr > >>>>>> "security.<security module name>" > >>>> Please excuse my late entry into this review. > >>>> > >>>> First, *Do not prefix general LSM interface work with "selinux:"* > >>>> You should only use that prefix when you're dealing with the > >>>> internals of SELinux. Use "LSM:" if you are changing LSM interfaces. > >>>> If you are changing security.h or lsm_hooks.h you are almost > >>>> certainly changing LSM interfaces. > >>>> > >>>> Because you prefixed this work with "selinux:" I pretty much > >>>> ignored it. Please don't do that again. > >>>> > >>>> Why aren't you using security_inode_getsecctx, like kernfs and nfs? > >>>> > >>> Sorry, I don't understand why you mention security_inode_getsecctx(). > >>> The selinux_dentry_init_security is called for new inode, I don't how > >>> security_inode_getsecctx() can help. > >> I didn't do a great job describing my issue. Let me try again. > >> > >> There is no way you should need to know the name of the security > >> module (e.g. SELinux, Smack) in the CephFS code. Nor should you be > >> assuming that there is only one security attribute, nor that it is > >> named "security.<security module name>". Sure, that's true for > >> SELinux, but SELinux isn't the only security module in town. > >> > >> Other filesystems don't need anything like this. Why do you? > >> > > What I want is a way to get security context before inode is created. > > What are you doing differently from the other filesystems? > Why doesn't the security_d_instantiate mechanism work for you? > Because I want to minimize overhead of setting security xattrs for new inode. When creating new file/dir, cephfs client sends one request to MDS. The request can carry xattrs. MDS can create new file/dir and setting xattrs at the same time. If cephfs follows other filesystems' code, cephfs client needs to send to two request to MDS for each new file/dir, one for create, one for setting security xattrs. > > The closest interface is security_dentry_init_security(). Currently > > the only NFS uses security_dentry_init_security() and only selinux has > > dentry_init_security callback. NFS store selinux context in its > > security label (NFS inode can only have one security label, security > > label can only store one security context). SO current NFS security > > label implementation can not support multiple security modules. > > That's correct. The labeled NFS is a very specific mechanism. It > does not do anything except Mandatory Access Control labeling. It > cannot be used for any other attributes. I am not a fan of the > labeled NFS implementation, and was vocal about it at the time. > > > I don't want to limit cephfs to single security module. So I make > > cephfs store security context in security.<security module name> > > xattr. > > Please look at the mechanism used in kernfs. > kernfs' mechanism is simple. It just provides kernfs_trusted_xattr_handler. It does not initialize security context for new file/dir. I don't see how to adapt its mechanism for cephfs. Regards Yan, Zheng > > When multiple security modules work is done in the future, > > cephfs can easily supports it. (introduce a new > > security_dentry_init_security() which return security contexts for all > > enabled security modules) > > That is easier said than done. I've proposed several mechanisms, > none of which are especially satisfactory. The best mechanism I see > is the one used in kernfs, which is why I suggest you use that. >
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 6dd146885da9..d18a5fb7aec3 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -122,7 +122,8 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, return NULL; err = security_dentry_init_security(dentry, sattr->ia_mode, - &dentry->d_name, (void **)&label->label, &label->len); + &dentry->d_name, NULL, + (void **)&label->label, &label->len); if (err == 0) return label; diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 8f1131c8dd54..e176c2032bdc 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1476,8 +1476,8 @@ union security_list_options { unsigned long *set_kern_flags); int (*sb_parse_opts_str)(char *options, struct security_mnt_opts *opts); int (*dentry_init_security)(struct dentry *dentry, int mode, - const struct qstr *name, void **ctx, - u32 *ctxlen); + const struct qstr *name, const char **label, + void **ctx, u32 *ctxlen); int (*dentry_create_files_as)(struct dentry *dentry, int mode, struct qstr *name, const struct cred *old, diff --git a/include/linux/security.h b/include/linux/security.h index 63030c85ee19..df2d73998c64 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -246,8 +246,9 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, unsigned long *set_kern_flags); int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts); int security_dentry_init_security(struct dentry *dentry, int mode, - const struct qstr *name, void **ctx, - u32 *ctxlen); + const struct qstr *name, + const char **label, + void **ctx, u32 *ctxlen); int security_dentry_create_files_as(struct dentry *dentry, int mode, struct qstr *name, const struct cred *old, @@ -609,8 +610,8 @@ static inline void security_inode_free(struct inode *inode) static inline int security_dentry_init_security(struct dentry *dentry, int mode, const struct qstr *name, - void **ctx, - u32 *ctxlen) + const char **label, + void **ctx, u32 *ctxlen) { return -EOPNOTSUPP; } diff --git a/security/security.c b/security/security.c index 68f46d849abe..69818d46aa28 100644 --- a/security/security.c +++ b/security/security.c @@ -450,11 +450,12 @@ void security_inode_free(struct inode *inode) } int security_dentry_init_security(struct dentry *dentry, int mode, - const struct qstr *name, void **ctx, - u32 *ctxlen) + const struct qstr *name, + const char **label, + void **ctx, u32 *ctxlen) { return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, - name, ctx, ctxlen); + name, label, ctx, ctxlen); } EXPORT_SYMBOL(security_dentry_init_security); diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 2b5ee5fbd652..eca3879d9357 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2985,8 +2985,9 @@ static void selinux_inode_free_security(struct inode *inode) } static int selinux_dentry_init_security(struct dentry *dentry, int mode, - const struct qstr *name, void **ctx, - u32 *ctxlen) + const struct qstr *name, + const char **label, + void **ctx, u32 *ctxlen) { u32 newsid; int rc; @@ -2998,6 +2999,9 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode, if (rc) return rc; + if (label) + *label = XATTR_SELINUX_SUFFIX; + return security_sid_to_context(&selinux_state, newsid, (char **)ctx, ctxlen); }
This is preparation for CephFS security label. CephFS's implementation uses dentry_init_security() to get security context before inode is created, then sends open/mkdir/mknod request to MDS, together with security xattr "security.<security module name>" Signed-off-by: "Yan, Zheng" <zyan@redhat.com> --- fs/nfs/nfs4proc.c | 3 ++- include/linux/lsm_hooks.h | 4 ++-- include/linux/security.h | 9 +++++---- security/security.c | 7 ++++--- security/selinux/hooks.c | 8 ++++++-- 5 files changed, 19 insertions(+), 12 deletions(-)