[PATCH 1/3] selinux: make dentry_init_security() return security module name

[Casey Schaufler]

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 at 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 at kernel.org>
>
>