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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
>
>
