Re: [PATCH 5/5] smack: fix bug: invalid label of unix socket file

[Roberto Sassu <roberto.sassu@huaweicloud.com>]

On Mon, 2025-06-16 at 04:07 +0300, Konstantin Andreev wrote:
> According to [1], the label of a UNIX domain socket (UDS)
> file (i.e., the filesystem object representing the socket)
> is not supposed to participate in Smack security.
> 
> To achieve this, [1] labels UDS files with "*"
> in smack_d_instantiate().
> 
> Before [2], smack_d_instantiate() was responsible
> for initializing Smack security for all inodes,
> except ones under /proc
> 
> [2] imposed the sole responsibility for initializing
> inode security for newly created filesystem objects
> on smack_inode_init_security().
> 
> However, smack_inode_init_security() lacks some logic
> present in smack_d_instantiate().
> In particular, it does not label UDS files with "*".
> 
> This patch adds the missing labeling of UDS files
> with "*" to smack_inode_init_security().
> 
> Labeling UDS files with "*" in smack_d_instantiate()
> still works for stale UDS files that already exist on
> disk. Stale UDS files are useless, but I keep labeling
> them for consistency and maybe to make easier for user
> to delete them.
> 
> Compared to [1], this version introduces the following
> improvements:
> 
>   * UDS file label is held inside inode only
>     and not saved to xattrs.
> 
>   * relabeling UDS files (setxattr, removexattr, etc.)
>     is blocked.
> 
> [1] 2010-11-24 Casey Schaufler
> commit b4e0d5f0791b ("Smack: UDS revision")
> 
> [2] 2023-11-16 roberto.sassu
> Fixes: e63d86b8b764 ("smack: Initialize the in-memory inode in smack_inode_init_security()")
> Link: https://lore.kernel.org/linux-security-module/20231116090125.187209-5-roberto.sassu@huaweicloud.com/
> 
> Signed-off-by: Konstantin Andreev <andreev@swemel.ru>
> ---
>  Documentation/admin-guide/LSM/Smack.rst |  5 +++
>  security/smack/smack_lsm.c              | 58 +++++++++++++++++++------
>  2 files changed, 49 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/admin-guide/LSM/Smack.rst b/Documentation/admin-guide/LSM/Smack.rst
> index 6d44f4fdbf59..1b554b5bf98e 100644
> --- a/Documentation/admin-guide/LSM/Smack.rst
> +++ b/Documentation/admin-guide/LSM/Smack.rst
> @@ -696,6 +696,11 @@ sockets.
>  	A privileged program may set this to match the label of another
>  	task with which it hopes to communicate.
>  
> +UNIX domain socket (UDS) with a BSD address functions both as a file in a
> +filesystem and as a socket. As a file, it carries the SMACK64 attribute. This
> +attribute is not involved in Smack security enforcement and is immutably
> +assigned the label "*".
> +
>  Smack Netlabel Exceptions
>  ~~~~~~~~~~~~~~~~~~~~~~~~~
>  
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index fb23254c8a54..1b41ae053966 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1021,6 +1021,16 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>  	bool trans_cred;
>  	bool trans_rule;
>  
> +	/*
> +	 * UNIX domain sockets use lower level socket data. Let
> +	 * UDS inode have fixed * label to keep smack_inode_permission() calm
> +	 * when called from unix_find_bsd()
> +	 */
> +	if (S_ISSOCK(inode->i_mode)) {
> +		/* forced label, no need to save to xattrs */
> +		issp->smk_inode = &smack_known_star;
> +		goto instant_inode;

Maybe you could also set SMK_INODE_INSTANT here and just return.

Roberto

> +	}
>  	/*
>  	 * If equal, transmuting already occurred in
>  	 * smack_dentry_create_files_as(). No need to check again.
> @@ -1057,14 +1067,16 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>  		}
>  	}
>  
> -	issp->smk_flags |= (SMK_INODE_INSTANT | transflag);
> -	if (rc)
> -		return rc;
> -
> -	return xattr_dupval(xattrs, xattr_count,
> +	if (rc == 0)
> +		if (xattr_dupval(xattrs, xattr_count,
>  			    XATTR_SMACK_SUFFIX,
>  			    issp->smk_inode->smk_known,
> -		     strlen(issp->smk_inode->smk_known));
> +		     strlen(issp->smk_inode->smk_known)
> +		))
> +			rc = -ENOMEM;
> +instant_inode:
> +	issp->smk_flags |= (SMK_INODE_INSTANT | transflag);
> +	return rc;
>  }
>  
>  /**
> @@ -1338,13 +1350,23 @@ static int smack_inode_setxattr(struct mnt_idmap *idmap,
>  	int check_import = 0;
>  	int check_star = 0;
>  	int rc = 0;
> +	umode_t const i_mode = d_backing_inode(dentry)->i_mode;
>  
>  	/*
>  	 * Check label validity here so import won't fail in post_setxattr
>  	 */
> -	if (strcmp(name, XATTR_NAME_SMACK) == 0 ||
> -	    strcmp(name, XATTR_NAME_SMACKIPIN) == 0 ||
> -	    strcmp(name, XATTR_NAME_SMACKIPOUT) == 0) {
> +	if (strcmp(name, XATTR_NAME_SMACK) == 0) {
> +		/*
> +		 * UDS inode has fixed label
> +		 */
> +		if (S_ISSOCK(i_mode)) {
> +			rc = -EINVAL;
> +		} else {
> +			check_priv = 1;
> +			check_import = 1;
> +		}
> +	} else if (strcmp(name, XATTR_NAME_SMACKIPIN) == 0 ||
> +		   strcmp(name, XATTR_NAME_SMACKIPOUT) == 0) {
>  		check_priv = 1;
>  		check_import = 1;
>  	} else if (strcmp(name, XATTR_NAME_SMACKEXEC) == 0 ||
> @@ -1354,7 +1376,7 @@ static int smack_inode_setxattr(struct mnt_idmap *idmap,
>  		check_star = 1;
>  	} else if (strcmp(name, XATTR_NAME_SMACKTRANSMUTE) == 0) {
>  		check_priv = 1;
> -		if (!S_ISDIR(d_backing_inode(dentry)->i_mode) ||
> +		if (!S_ISDIR(i_mode) ||
>  		    size != TRANS_TRUE_SIZE ||
>  		    strncmp(value, TRANS_TRUE, TRANS_TRUE_SIZE) != 0)
>  			rc = -EINVAL;
> @@ -1485,12 +1507,15 @@ static int smack_inode_removexattr(struct mnt_idmap *idmap,
>  	 * Don't do anything special for these.
>  	 *	XATTR_NAME_SMACKIPIN
>  	 *	XATTR_NAME_SMACKIPOUT
> +	 *	XATTR_NAME_SMACK if S_ISSOCK (UDS inode has fixed label)
>  	 */
>  	if (strcmp(name, XATTR_NAME_SMACK) == 0) {
> -		struct super_block *sbp = dentry->d_sb;
> -		struct superblock_smack *sbsp = smack_superblock(sbp);
> +		if (!S_ISSOCK(d_backing_inode(dentry)->i_mode)) {
> +			struct super_block *sbp = dentry->d_sb;
> +			struct superblock_smack *sbsp = smack_superblock(sbp);
>  
> -		isp->smk_inode = sbsp->smk_default;
> +			isp->smk_inode = sbsp->smk_default;
> +		}
>  	} else if (strcmp(name, XATTR_NAME_SMACKEXEC) == 0)
>  		isp->smk_task = NULL;
>  	else if (strcmp(name, XATTR_NAME_SMACKMMAP) == 0)
> @@ -3608,7 +3633,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
>  		 */
>  
>  		/*
> -		 * UNIX domain sockets use lower level socket data.
> +		 * UDS inode has fixed label (*)
>  		 */
>  		if (S_ISSOCK(inode->i_mode)) {
>  			final = &smack_known_star;
> @@ -4879,6 +4904,11 @@ static int smack_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
>  
>  static int smack_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
>  {
> +	/*
> +	 * UDS inode has fixed label. Ignore nfs label.
> +	 */
> +	if (S_ISSOCK(inode->i_mode))
> +		return 0;
>  	return smack_inode_setsecurity(inode, XATTR_SMACK_SUFFIX, ctx,
>  				       ctxlen, 0);
>  }