Re: [RFC][PATCH 2/2] smack: Record transmuting in smk_transmuted

[Casey Schaufler]

On 5/8/2023 10:02 AM, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
>
> smack_dentry_create_files_as() determines whether transmuting should occur
> based on the label of the parent directory the new inode will be added to,
> and not the label of the directory where it is created.
>
> This helps for example to do transmuting on overlayfs, since the latter
> first creates the inode in the working directory, and then moves it to the
> correct destination.
>
> However, despite smack_dentry_create_files_as() provides the correct label,
> smack_inode_init_security() does not know from passed information whether
> or not transmuting occurred. Without this information,
> smack_inode_init_security() cannot set SMK_INODE_CHANGED in smk_flags,
> which will result in the SMACK64TRANSMUTE xattr not being set in
> smack_d_instantiate().
>
> Thus, add the smk_transmuted field to the task_smack structure, and set it
> in smack_dentry_create_files_as() to smk_task if transmuting occurred. If
> smk_task is equal to smk_transmuted in smack_inode_init_security(), act as
> if transmuting was successful but without taking the label from the parent
> directory (the inode label was already set correctly from the current
> credentials in smack_inode_alloc_security()).
>
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>

Added to smack next.

> ---
>  security/smack/smack.h     |  1 +
>  security/smack/smack_lsm.c | 41 +++++++++++++++++++++++++++-----------
>  2 files changed, 30 insertions(+), 12 deletions(-)
>
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index e2239be7bd6..aa15ff56ed6 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -120,6 +120,7 @@ struct inode_smack {
>  struct task_smack {
>  	struct smack_known	*smk_task;	/* label for access control */
>  	struct smack_known	*smk_forked;	/* label when forked */
> +	struct smack_known	*smk_transmuted;/* label when transmuted */
>  	struct list_head	smk_rules;	/* per task access rules */
>  	struct mutex		smk_rules_lock;	/* lock for the rules */
>  	struct list_head	smk_relabel;	/* transit allowed labels */
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index c7e37ed2799..6e270cf3fd3 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -933,8 +933,9 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>  				     const struct qstr *qstr, const char **name,
>  				     void **value, size_t *len)
>  {
> +	struct task_smack *tsp = smack_cred(current_cred());
>  	struct inode_smack *issp = smack_inode(inode);
> -	struct smack_known *skp = smk_of_current();
> +	struct smack_known *skp = smk_of_task(tsp);
>  	struct smack_known *isp = smk_of_inode(inode);
>  	struct smack_known *dsp = smk_of_inode(dir);
>  	int may;
> @@ -943,20 +944,34 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>  		*name = XATTR_SMACK_SUFFIX;
>  
>  	if (value && len) {
> -		rcu_read_lock();
> -		may = smk_access_entry(skp->smk_known, dsp->smk_known,
> -				       &skp->smk_rules);
> -		rcu_read_unlock();
> +		/*
> +		 * If equal, transmuting already occurred in
> +		 * smack_dentry_create_files_as(). No need to check again.
> +		 */
> +		if (tsp->smk_task != tsp->smk_transmuted) {
> +			rcu_read_lock();
> +			may = smk_access_entry(skp->smk_known, dsp->smk_known,
> +					       &skp->smk_rules);
> +			rcu_read_unlock();
> +		}
>  
>  		/*
> -		 * If the access rule allows transmutation and
> -		 * the directory requests transmutation then
> -		 * by all means transmute.
> +		 * In addition to having smk_task equal to smk_transmuted,
> +		 * if the access rule allows transmutation and the directory
> +		 * requests transmutation then by all means transmute.
>  		 * Mark the inode as changed.
>  		 */
> -		if (may > 0 && ((may & MAY_TRANSMUTE) != 0) &&
> -		    smk_inode_transmutable(dir)) {
> -			isp = dsp;
> +		if ((tsp->smk_task == tsp->smk_transmuted) ||
> +		    (may > 0 && ((may & MAY_TRANSMUTE) != 0) &&
> +		     smk_inode_transmutable(dir))) {
> +			/*
> +			 * The caller of smack_dentry_create_files_as()
> +			 * should have overridden the current cred, so the
> +			 * inode label was already set correctly in
> +			 * smack_inode_alloc_security().
> +			 */
> +			if (tsp->smk_task != tsp->smk_transmuted)
> +				isp = dsp;
>  			issp->smk_flags |= SMK_INODE_CHANGED;
>  		}
>  
> @@ -4767,8 +4782,10 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
>  		 * providing access is transmuting use the containing
>  		 * directory label instead of the process label.
>  		 */
> -		if (may > 0 && (may & MAY_TRANSMUTE))
> +		if (may > 0 && (may & MAY_TRANSMUTE)) {
>  			ntsp->smk_task = isp->smk_inode;
> +			ntsp->smk_transmuted = ntsp->smk_task;
> +		}
>  	}
>  	return 0;
>  }