Re: [PATCH] smack: preserve low-integrity labels across file copy via whitelist

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] security/smack: TOCTOU race in smack_inode_init_security() causes NULL deref
- [High] security/smack: lockless mutation of shared cred in smack_file_open()
- [High] security/smack: caching file labels on task creds mislabels unrelated files
- [Medium] security/smack: smk_preserve state leaks during label transmutation
--

commit 6dde1ac3f8846672b424ea2f203f58b7b5b2f900
Author: Tang Pengke <tangpengke@yhkylin.cn>
smack: preserve low-integrity labels across file copy via whitelist

This commit introduces a mechanism to preserve low-integrity Smack labels
across file copies by temporarily storing a whitelisted label in the
task's Smack credential during file open. This label is then inherited by
the next inode created by the task.

> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c

[ ... ]

> @@ -1066,6 +1073,19 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>  		}
>  	}
>  
> +#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
> +	/*
> +	 * If a whitelisted label was recorded on file open and no
> +	 * transmutation applies, inherit it so the new file keeps the
> +	 * source's (low-integrity) label across the copy.
> +	 */
> +	if (!trans_cred && tsp->smk_preserve != NULL &&
> +	    !(trans_rule && smk_inode_transmutable(dir))) {

[Severity: Medium]
If transmutation applies, the condition checking smk_inode_transmutable(dir)
evaluates to false and this block is skipped.

Because tsp->smk_preserve is not cleared in that path, will the untrusted
label leak and inadvertently taint the next unrelated file created in a
non-transmutable directory?

> +		issp->smk_inode = tsp->smk_preserve;
> +		tsp->smk_preserve = NULL;

[Severity: Critical]
Could a time-of-check to time-of-use race occur here if the task credential
is shared among multiple threads?

If thread A and thread B both enter smack_inode_init_security() concurrently,
could thread B clear tsp->smk_preserve right after thread A checks it for
non-null, causing thread A to assign a null pointer to issp->smk_inode? 

This appears to lead to a kernel panic when issp->smk_inode->smk_known is
subsequently passed to strlen() a few lines below.

> +	}
> +#endif
> +

[ ... ]

> @@ -2068,6 +2088,39 @@ static int smack_file_open(struct file *file)
>  	smk_ad_setfield_u_fs_path(&ad, file->f_path);
>  	rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad);
>  	rc = smk_bu_credfile(file->f_cred, file, MAY_READ, rc);
> +	if (rc)
> +		return rc;
> +
> +#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST

[ ... ]

> +	if (!list_empty(&smk_preserve_wl) &&
> +	    (file->f_flags & O_ACCMODE) == O_RDONLY &&
> +	    S_ISREG(inode->i_mode)) {
> +		struct smack_known *skp = smk_of_inode(inode);
> +
> +		if (smk_preserve_allowed(skp))
> +			tsp->smk_preserve = skp;

[Severity: High]
Is it safe to locklessly mutate a live, committed, and potentially shared
cred structure here? 

Since credentials can be shared between threads via clone flags, mutating
tsp->smk_preserve without synchronization might result in data races. Standard
kernel patterns generally treat cred structures as immutable after
commit_creds() is called.

[Severity: High]
Does latching the file's label onto the task simply because a file was read
risk mislabeling unrelated files during subsequent creations? 

For example, if a program reads a whitelisted file and subsequently creates
a completely unrelated temporary file, it seems the unrelated file will
erroneously inherit the whitelisted label in smack_inode_init_security().

> +	}
> +#endif
>  
>  	return rc;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/9EAFC73B591799F5+20260930030425.496501-1-tangpengke@yhkylin.cn?part=1