[PATCH 0/5] smack: fix bugs: invalid unix socket label, invalid transmute attr

[PATCH 0/5] smack: fix bugs: invalid unix socket label, invalid transmute attr

[Konstantin Andreev]

Formerly, Smack inode security was initialized
by smack_d_instantiate() for all inodes,
except ones under /proc

Commit [1] 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().

This patch series fixes 2 particular omissions
I faced directly:

1) special handling of unix socket files (5th patch)
2) S_ISDIR check for "transmute" xattr (2nd patch)

I did not check for other omissions,
but there may be ones.

Patches 1,3,4 are necessary optimizations
in smack_inode_init_security() made along the way.

I structured the changes this way to make the review
process easier.

The patch set applies on top of:
https://github.com/cschaufler/smack-next/commits/next
commit 4b59f4fd0a36

[1] 2023-11-16 roberto.sassu
commit 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/

Konstantin Andreev (5):
  smack: deduplicate "does access rule request transmutation"
  smack: fix bug: SMACK64TRANSMUTE set on non-directory
  smack: deduplicate xattr setting in smack_inode_init_security()
  smack: always "instantiate" inode in smack_inode_init_security()
  smack: fix bug: invalid label of unix socket file

 Documentation/admin-guide/LSM/Smack.rst |   5 +
 security/smack/smack_lsm.c              | 159 +++++++++++++++---------
 2 files changed, 107 insertions(+), 57 deletions(-)

-- 
2.43.0

[PATCH 1/5] smack: deduplicate "does access rule request transmutation"

[Konstantin Andreev]

Signed-off-by: Konstantin Andreev <andreev@swemel.ru>
---
 security/smack/smack_lsm.c | 57 +++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 25 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 99833168604e..a7292d286f7c 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -963,6 +963,24 @@ static int smack_inode_alloc_security(struct inode *inode)
 	return 0;
 }
 
+/**
+ * smk_rule_transmutes - does access rule for (subject,object) contain 't'?
+ * @subject: a pointer to the subject's Smack label entry
+ * @object: a pointer to the object's Smack label entry
+ */
+static bool
+smk_rule_transmutes(struct smack_known *subject,
+	      const struct smack_known *object)
+{
+	int may;
+
+	rcu_read_lock();
+	may = smk_access_entry(subject->smk_known, object->smk_known,
+			       &subject->smk_rules);
+	rcu_read_unlock();
+	return (may > 0) && (may & MAY_TRANSMUTE);
+}
+
 /**
  * smack_inode_init_security - copy out the smack from an inode
  * @inode: the newly created inode
@@ -978,23 +996,19 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 				     struct xattr *xattrs, int *xattr_count)
 {
 	struct task_smack *tsp = smack_cred(current_cred());
-	struct inode_smack *issp = smack_inode(inode);
-	struct smack_known *skp = smk_of_task(tsp);
-	struct smack_known *isp = smk_of_inode(inode);
+	struct inode_smack * const issp = smack_inode(inode);
 	struct smack_known *dsp = smk_of_inode(dir);
 	struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count);
-	int may;
+	bool trans_cred;
+	bool trans_rule;
 
 	/*
 	 * 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();
-	}
+	trans_cred = (tsp->smk_task == tsp->smk_transmuted);
+	if (!trans_cred)
+		trans_rule = smk_rule_transmutes(smk_of_task(tsp), dsp);
 
 	/*
 	 * In addition to having smk_task equal to smk_transmuted,
@@ -1002,9 +1016,7 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 	 * requests transmutation then by all means transmute.
 	 * Mark the inode as changed.
 	 */
-	if ((tsp->smk_task == tsp->smk_transmuted) ||
-	    (may > 0 && ((may & MAY_TRANSMUTE) != 0) &&
-	     smk_inode_transmutable(dir))) {
+	if (trans_cred || (trans_rule && smk_inode_transmutable(dir))) {
 		struct xattr *xattr_transmute;
 
 		/*
@@ -1013,8 +1025,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 		 * inode label was already set correctly in
 		 * smack_inode_alloc_security().
 		 */
-		if (tsp->smk_task != tsp->smk_transmuted)
-			isp = issp->smk_inode = dsp;
+		if (!trans_cred)
+			issp->smk_inode = dsp;
 
 		issp->smk_flags |= SMK_INODE_TRANSMUTE;
 		xattr_transmute = lsm_get_xattr_slot(xattrs,
@@ -1034,11 +1046,13 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 	issp->smk_flags |= SMK_INODE_INSTANT;
 
 	if (xattr) {
-		xattr->value = kstrdup(isp->smk_known, GFP_NOFS);
+		const char *inode_label = issp->smk_inode->smk_known;
+
+		xattr->value = kstrdup(inode_label, GFP_NOFS);
 		if (!xattr->value)
 			return -ENOMEM;
 
-		xattr->value_len = strlen(isp->smk_known);
+		xattr->value_len = strlen(inode_label);
 		xattr->name = XATTR_SMACK_SUFFIX;
 	}
 
@@ -4922,7 +4936,6 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
 	struct task_smack *otsp = smack_cred(old);
 	struct task_smack *ntsp = smack_cred(new);
 	struct inode_smack *isp;
-	int may;
 
 	/*
 	 * Use the process credential unless all of
@@ -4936,18 +4949,12 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
 	isp = smack_inode(d_inode(dentry->d_parent));
 
 	if (isp->smk_flags & SMK_INODE_TRANSMUTE) {
-		rcu_read_lock();
-		may = smk_access_entry(otsp->smk_task->smk_known,
-				       isp->smk_inode->smk_known,
-				       &otsp->smk_task->smk_rules);
-		rcu_read_unlock();
-
 		/*
 		 * If the directory is transmuting and the rule
 		 * providing access is transmuting use the containing
 		 * directory label instead of the process label.
 		 */
-		if (may > 0 && (may & MAY_TRANSMUTE)) {
+		if (smk_rule_transmutes(otsp->smk_task, isp->smk_inode)) {
 			ntsp->smk_task = isp->smk_inode;
 			ntsp->smk_transmuted = ntsp->smk_task;
 		}
-- 
2.43.0

[PATCH 2/5] smack: fix bug: SMACK64TRANSMUTE set on non-directory

[Konstantin Andreev]

When a new file system object is created
and the conditions for label transmutation are met,
the SMACK64TRANSMUTE extended attribute is set
on the object regardless of its type:
file, pipe, socket, symlink, or directory.

However,
SMACK64TRANSMUTE may only be set on directories.

This bug is a combined effect of the commits [1] and [2]
which both transfer functionality
from smack_d_instantiate() to smack_inode_init_security(),
but only in part.

Commit [1] set blank  SMACK64TRANSMUTE on improper object types.
Commit [2] set "TRUE" SMACK64TRANSMUTE on improper object types.

[1] 2023-06-10,
Fixes: baed456a6a2f ("smack: Set the SMACK64TRANSMUTE xattr in smack_inode_init_security()")
Link: https://lore.kernel.org/linux-security-module/20230610075738.3273764-3-roberto.sassu@huaweicloud.com/

[2] 2023-11-16,
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>
---
 security/smack/smack_lsm.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index a7292d286f7c..2d3186e50c62 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1028,18 +1028,20 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 		if (!trans_cred)
 			issp->smk_inode = dsp;
 
-		issp->smk_flags |= SMK_INODE_TRANSMUTE;
-		xattr_transmute = lsm_get_xattr_slot(xattrs,
-						     xattr_count);
-		if (xattr_transmute) {
-			xattr_transmute->value = kmemdup(TRANS_TRUE,
-							 TRANS_TRUE_SIZE,
-							 GFP_NOFS);
-			if (!xattr_transmute->value)
-				return -ENOMEM;
+		if (S_ISDIR(inode->i_mode)) {
+			issp->smk_flags |= SMK_INODE_TRANSMUTE;
+			xattr_transmute = lsm_get_xattr_slot(xattrs,
+							     xattr_count);
+			if (xattr_transmute) {
+				xattr_transmute->value = kmemdup(TRANS_TRUE,
+								 TRANS_TRUE_SIZE,
+								 GFP_NOFS);
+				if (!xattr_transmute->value)
+					return -ENOMEM;
 
-			xattr_transmute->value_len = TRANS_TRUE_SIZE;
-			xattr_transmute->name = XATTR_SMACK_TRANSMUTE;
+				xattr_transmute->value_len = TRANS_TRUE_SIZE;
+				xattr_transmute->name = XATTR_SMACK_TRANSMUTE;
+			}
 		}
 	}
 
-- 
2.43.0

[PATCH 3/5] smack: deduplicate xattr setting in smack_inode_init_security()

[Konstantin Andreev]

Signed-off-by: Konstantin Andreev <andreev@swemel.ru>
---
 security/smack/smack_lsm.c | 54 ++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 2d3186e50c62..2b46a2867226 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -981,6 +981,24 @@ smk_rule_transmutes(struct smack_known *subject,
 	return (may > 0) && (may & MAY_TRANSMUTE);
 }
 
+static int
+xattr_dupval(struct xattr *xattrs, int *xattr_count,
+	     const char *name, const void *value, unsigned int vallen)
+{
+	struct xattr * const xattr = lsm_get_xattr_slot(xattrs, xattr_count);
+
+	if (!xattr)
+		return 0;
+
+	xattr->value = kmemdup(value, vallen, GFP_NOFS);
+	if (!xattr->value)
+		return -ENOMEM;
+
+	xattr->value_len = vallen;
+	xattr->name = name;
+	return 0;
+}
+
 /**
  * smack_inode_init_security - copy out the smack from an inode
  * @inode: the newly created inode
@@ -998,7 +1016,6 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 	struct task_smack *tsp = smack_cred(current_cred());
 	struct inode_smack * const issp = smack_inode(inode);
 	struct smack_known *dsp = smk_of_inode(dir);
-	struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count);
 	bool trans_cred;
 	bool trans_rule;
 
@@ -1017,8 +1034,6 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 	 * Mark the inode as changed.
 	 */
 	if (trans_cred || (trans_rule && smk_inode_transmutable(dir))) {
-		struct xattr *xattr_transmute;
-
 		/*
 		 * The caller of smack_dentry_create_files_as()
 		 * should have overridden the current cred, so the
@@ -1030,35 +1045,22 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 
 		if (S_ISDIR(inode->i_mode)) {
 			issp->smk_flags |= SMK_INODE_TRANSMUTE;
-			xattr_transmute = lsm_get_xattr_slot(xattrs,
-							     xattr_count);
-			if (xattr_transmute) {
-				xattr_transmute->value = kmemdup(TRANS_TRUE,
-								 TRANS_TRUE_SIZE,
-								 GFP_NOFS);
-				if (!xattr_transmute->value)
-					return -ENOMEM;
 
-				xattr_transmute->value_len = TRANS_TRUE_SIZE;
-				xattr_transmute->name = XATTR_SMACK_TRANSMUTE;
-			}
+			if (xattr_dupval(xattrs, xattr_count,
+				XATTR_SMACK_TRANSMUTE,
+				TRANS_TRUE,
+				TRANS_TRUE_SIZE
+			))
+				return -ENOMEM;
 		}
 	}
 
 	issp->smk_flags |= SMK_INODE_INSTANT;
 
-	if (xattr) {
-		const char *inode_label = issp->smk_inode->smk_known;
-
-		xattr->value = kstrdup(inode_label, GFP_NOFS);
-		if (!xattr->value)
-			return -ENOMEM;
-
-		xattr->value_len = strlen(inode_label);
-		xattr->name = XATTR_SMACK_SUFFIX;
-	}
-
-	return 0;
+	return xattr_dupval(xattrs, xattr_count,
+			    XATTR_SMACK_SUFFIX,
+			    issp->smk_inode->smk_known,
+		     strlen(issp->smk_inode->smk_known));
 }
 
 /**
-- 
2.43.0

[PATCH 4/5] smack: always "instantiate" inode in smack_inode_init_security()

[Konstantin Andreev]

If memory allocation for the SMACK64TRANSMUTE
xattr value fails in smack_inode_init_security(),
the SMK_INODE_INSTANT flag is not set in
(struct inode_smack *issp)->smk_flags,
leaving the inode as not "instantiated".

It does not matter if fs frees the inode
after failed smack_inode_init_security() call,
but there is no guarantee for this.

To be safe, mark the inode as "instantiated",
even if allocation of xattr values fails.

Signed-off-by: Konstantin Andreev <andreev@swemel.ru>
---

I have found one execution path
that does not explicitly free inode after
failed security_inode_init_security() call:

    fs/f2fs/recovery.c`recover_data()
    `fs/f2fs/recovery.c`recover_dentry(entry->inode,)
     `...
      `security_inode_init_security(,,,f2fs_initxattrs,)

and some pathes with unclear decision.
May they be affected or not, it is safer
to always "instantiate" inode.

 security/smack/smack_lsm.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 2b46a2867226..fb23254c8a54 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1016,6 +1016,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 	struct task_smack *tsp = smack_cred(current_cred());
 	struct inode_smack * const issp = smack_inode(inode);
 	struct smack_known *dsp = smk_of_inode(dir);
+	int rc = 0;
+	int transflag = 0;
 	bool trans_cred;
 	bool trans_rule;
 
@@ -1044,18 +1046,20 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 			issp->smk_inode = dsp;
 
 		if (S_ISDIR(inode->i_mode)) {
-			issp->smk_flags |= SMK_INODE_TRANSMUTE;
+			transflag = SMK_INODE_TRANSMUTE;
 
 			if (xattr_dupval(xattrs, xattr_count,
 				XATTR_SMACK_TRANSMUTE,
 				TRANS_TRUE,
 				TRANS_TRUE_SIZE
 			))
-				return -ENOMEM;
+				rc = -ENOMEM;
 		}
 	}
 
-	issp->smk_flags |= SMK_INODE_INSTANT;
+	issp->smk_flags |= (SMK_INODE_INSTANT | transflag);
+	if (rc)
+		return rc;
 
 	return xattr_dupval(xattrs, xattr_count,
 			    XATTR_SMACK_SUFFIX,
-- 
2.43.0

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

[Konstantin Andreev]

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;
+	}
 	/*
 	 * 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);
 }
-- 
2.43.0

Re: [PATCH 2/5] smack: fix bug: SMACK64TRANSMUTE set on non-directory

[Roberto Sassu]

On Mon, 2025-06-16 at 04:07 +0300, Konstantin Andreev wrote:
> When a new file system object is created
> and the conditions for label transmutation are met,
> the SMACK64TRANSMUTE extended attribute is set
> on the object regardless of its type:
> file, pipe, socket, symlink, or directory.
> 
> However,
> SMACK64TRANSMUTE may only be set on directories.
> 
> This bug is a combined effect of the commits [1] and [2]
> which both transfer functionality
> from smack_d_instantiate() to smack_inode_init_security(),
> but only in part.
> 
> Commit [1] set blank  SMACK64TRANSMUTE on improper object types.

Hi Konstantin

I didn't see where, can you point where it is done?

> Commit [2] set "TRUE" SMACK64TRANSMUTE on improper object types.
> 
> [1] 2023-06-10,
> Fixes: baed456a6a2f ("smack: Set the SMACK64TRANSMUTE xattr in smack_inode_init_security()")
> Link: https://lore.kernel.org/linux-security-module/20230610075738.3273764-3-roberto.sassu@huaweicloud.com/
> 
> [2] 2023-11-16,
> 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>
> ---
>  security/smack/smack_lsm.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index a7292d286f7c..2d3186e50c62 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1028,18 +1028,20 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>  		if (!trans_cred)
>  			issp->smk_inode = dsp;
>  
> -		issp->smk_flags |= SMK_INODE_TRANSMUTE;
> -		xattr_transmute = lsm_get_xattr_slot(xattrs,
> -						     xattr_count);
> -		if (xattr_transmute) {
> -			xattr_transmute->value = kmemdup(TRANS_TRUE,
> -							 TRANS_TRUE_SIZE,
> -							 GFP_NOFS);
> -			if (!xattr_transmute->value)
> -				return -ENOMEM;

To avoid having another indentation, you could also set xattr_transmute
to NULL initially, and call lsm_get_xattr_slot() only for directories.
Setting SMK_INODE_TRANSMUTE in issp->smk_flags should be done of course
if xattr_transmute != NULL.

Either way is fine for me.

Roberto

> +		if (S_ISDIR(inode->i_mode)) {
> +			issp->smk_flags |= SMK_INODE_TRANSMUTE;
> +			xattr_transmute = lsm_get_xattr_slot(xattrs,
> +							     xattr_count);
> +			if (xattr_transmute) {
> +				xattr_transmute->value = kmemdup(TRANS_TRUE,
> +								 TRANS_TRUE_SIZE,
> +								 GFP_NOFS);
> +				if (!xattr_transmute->value)
> +					return -ENOMEM;
>  
> -			xattr_transmute->value_len = TRANS_TRUE_SIZE;
> -			xattr_transmute->name = XATTR_SMACK_TRANSMUTE;
> +				xattr_transmute->value_len = TRANS_TRUE_SIZE;
> +				xattr_transmute->name = XATTR_SMACK_TRANSMUTE;
> +			}
>  		}
>  	}
>  

Re: [PATCH 3/5] smack: deduplicate xattr setting in smack_inode_init_security()

[Roberto Sassu]

On Mon, 2025-06-16 at 04:07 +0300, Konstantin Andreev wrote:
> Signed-off-by: Konstantin Andreev <andreev@swemel.ru>
> ---
>  security/smack/smack_lsm.c | 54 ++++++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 2d3186e50c62..2b46a2867226 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -981,6 +981,24 @@ smk_rule_transmutes(struct smack_known *subject,
>  	return (may > 0) && (may & MAY_TRANSMUTE);
>  }
>  
> +static int
> +xattr_dupval(struct xattr *xattrs, int *xattr_count,
> +	     const char *name, const void *value, unsigned int vallen)
> +{
> +	struct xattr * const xattr = lsm_get_xattr_slot(xattrs, xattr_count);
> +
> +	if (!xattr)
> +		return 0;
> +
> +	xattr->value = kmemdup(value, vallen, GFP_NOFS);
> +	if (!xattr->value)
> +		return -ENOMEM;
> +
> +	xattr->value_len = vallen;
> +	xattr->name = name;
> +	return 0;
> +}
> +
>  /**
>   * smack_inode_init_security - copy out the smack from an inode
>   * @inode: the newly created inode
> @@ -998,7 +1016,6 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>  	struct task_smack *tsp = smack_cred(current_cred());
>  	struct inode_smack * const issp = smack_inode(inode);
>  	struct smack_known *dsp = smk_of_inode(dir);
> -	struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count);
>  	bool trans_cred;
>  	bool trans_rule;
>  
> @@ -1017,8 +1034,6 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>  	 * Mark the inode as changed.
>  	 */
>  	if (trans_cred || (trans_rule && smk_inode_transmutable(dir))) {
> -		struct xattr *xattr_transmute;
> -
>  		/*
>  		 * The caller of smack_dentry_create_files_as()
>  		 * should have overridden the current cred, so the
> @@ -1030,35 +1045,22 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>  
>  		if (S_ISDIR(inode->i_mode)) {
>  			issp->smk_flags |= SMK_INODE_TRANSMUTE;
> -			xattr_transmute = lsm_get_xattr_slot(xattrs,
> -							     xattr_count);
> -			if (xattr_transmute) {
> -				xattr_transmute->value = kmemdup(TRANS_TRUE,
> -								 TRANS_TRUE_SIZE,
> -								 GFP_NOFS);
> -				if (!xattr_transmute->value)
> -					return -ENOMEM;
>  
> -				xattr_transmute->value_len = TRANS_TRUE_SIZE;
> -				xattr_transmute->name = XATTR_SMACK_TRANSMUTE;
> -			}
> +			if (xattr_dupval(xattrs, xattr_count,
> +				XATTR_SMACK_TRANSMUTE,
> +				TRANS_TRUE,
> +				TRANS_TRUE_SIZE
> +			))

Ok, can also be optimized that way...

Roberto

> +				return -ENOMEM;
>  		}
>  	}
>  
>  	issp->smk_flags |= SMK_INODE_INSTANT;
>  
> -	if (xattr) {
> -		const char *inode_label = issp->smk_inode->smk_known;
> -
> -		xattr->value = kstrdup(inode_label, GFP_NOFS);
> -		if (!xattr->value)
> -			return -ENOMEM;
> -
> -		xattr->value_len = strlen(inode_label);
> -		xattr->name = XATTR_SMACK_SUFFIX;
> -	}
> -
> -	return 0;
> +	return xattr_dupval(xattrs, xattr_count,
> +			    XATTR_SMACK_SUFFIX,
> +			    issp->smk_inode->smk_known,
> +		     strlen(issp->smk_inode->smk_known));
>  }
>  
>  /**

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

[Roberto Sassu]

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);
>  }

Re[2]: [PATCH 2/5] smack: fix bug: SMACK64TRANSMUTE set on non-directory

[Konstantin Andreev]

Roberto Sassu, 16 Jun 2025 10:42:16 +0200:
> On Mon, 2025-06-16 at 04:07 +0300, Konstantin Andreev wrote:
>> When a new file system object is created
>> and the conditions for label transmutation are met,
>> the SMACK64TRANSMUTE extended attribute is set
>> on the object regardless of its type:
>> file, pipe, socket, symlink, or directory.
>>
>> However,
>> SMACK64TRANSMUTE may only be set on directories.
>>
>> This bug is a combined effect of the commits [1] and [2]
>> which both transfer functionality
>> from smack_d_instantiate() to smack_inode_init_security(),
>> but only in part.
>>
>> Commit [1] set blank  SMACK64TRANSMUTE on improper object types.
> 
> Hi Konstantin
> 
> I didn't see where, can you point where it is done?

Hi, Roberto.

I didnt investigate this thoroughly, but, I guess,
this proceeds like:

1) smack_inode_init_security() indeed sets "TRUE"
    for SMACK64TRANSMUTE in xattr for inode of any type.

2) smack_d_instantiate() then “confirms” "TRUE"
    only for directories.

At the moment of [1] both smack_inode_init_security()
and smack_d_instantiate() had the responsibility for
initialization of the inode Smack security.

So, user-visible SMACK64TRANSMUTE exists, but blank,
while on-disk xattr still "TRUE".

After [2] smack_d_instantiate() no longer participates
in the initialization of new filesystem objects,
so user-visible SMACK64TRANSMUTE became "TRUE".

You can check the user-visible effect
by building the kernel of version [1].

Konstantin.

>> Commit [2] set "TRUE" SMACK64TRANSMUTE on improper object types.
>>
>> [1] 2023-06-10,
>> Fixes: baed456a6a2f ("smack: Set the SMACK64TRANSMUTE xattr in smack_inode_init_security()")
>> Link: https://lore.kernel.org/linux-security-module/20230610075738.3273764-3-roberto.sassu@huaweicloud.com/
>>
>> [2] 2023-11-16,
>> 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>
>> ---
>>   security/smack/smack_lsm.c | 24 +++++++++++++-----------
>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index a7292d286f7c..2d3186e50c62 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -1028,18 +1028,20 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>>   		if (!trans_cred)
>>   			issp->smk_inode = dsp;
>>   
>> -		issp->smk_flags |= SMK_INODE_TRANSMUTE;
>> -		xattr_transmute = lsm_get_xattr_slot(xattrs,
>> -						     xattr_count);
>> -		if (xattr_transmute) {
>> -			xattr_transmute->value = kmemdup(TRANS_TRUE,
>> -							 TRANS_TRUE_SIZE,
>> -							 GFP_NOFS);
>> -			if (!xattr_transmute->value)
>> -				return -ENOMEM;
> 
> To avoid having another indentation, you could also set xattr_transmute
> to NULL initially, and call lsm_get_xattr_slot() only for directories.
> Setting SMK_INODE_TRANSMUTE in issp->smk_flags should be done of course
> if xattr_transmute != NULL.
> 
> Either way is fine for me.
> 
> Roberto
> 
>> +		if (S_ISDIR(inode->i_mode)) {
>> +			issp->smk_flags |= SMK_INODE_TRANSMUTE;
>> +			xattr_transmute = lsm_get_xattr_slot(xattrs,
>> +							     xattr_count);
>> +			if (xattr_transmute) {
>> +				xattr_transmute->value = kmemdup(TRANS_TRUE,
>> +								 TRANS_TRUE_SIZE,
>> +								 GFP_NOFS);
>> +				if (!xattr_transmute->value)
>> +					return -ENOMEM;
>>   
>> -			xattr_transmute->value_len = TRANS_TRUE_SIZE;
>> -			xattr_transmute->name = XATTR_SMACK_TRANSMUTE;
>> +				xattr_transmute->value_len = TRANS_TRUE_SIZE;
>> +				xattr_transmute->name = XATTR_SMACK_TRANSMUTE;
>> +			}
>>   		}
>>   	}

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

[Konstantin Andreev]

Roberto Sassu, 16 Jun 2025 11:05:11 +0200:
> On Mon, 2025-06-16 at 04:07 +0300, Konstantin Andreev wrote:
>> According to [1], the label of a UNIX domain socket (UDS)
>>
>> [irrelevant portion of the message deleted]
>>
>> 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.

Certainly.

But I personally avoid duplication even of small cleanups
and avoid multiple returns at the price of few gotos.

A matter of style. Either way is fine for me.
Let Casey decide.

Konstantin

[the rest of the message deleted]

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

[Casey Schaufler]

On 6/16/2025 4:46 AM, Konstantin Andreev wrote:
> Roberto Sassu, 16 Jun 2025 11:05:11 +0200:
>> On Mon, 2025-06-16 at 04:07 +0300, Konstantin Andreev wrote:
>>> According to [1], the label of a UNIX domain socket (UDS)
>>>
>>> [irrelevant portion of the message deleted]
>>>
>>> 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.
>
> Certainly.
>
> But I personally avoid duplication even of small cleanups
> and avoid multiple returns at the price of few gotos.

I generally prefer

	if (xyzzy)
		return -ESOMETHING;

to

	if (xyzzy)
		goto err_out;
	...
err_out:
	return -ESOMETHING;

I grew up in the era of "gotos considered harmful". When
err_out does more than just return a goto is fine, but a goto
that has nothing but a return is unnecessary and adds code that
needn't be there.

>
> A matter of style. Either way is fine for me.
> Let Casey decide.
>
> Konstantin
>
> [the rest of the message deleted]
>

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

[Konstantin Andreev]

Casey Schaufler, 16 Jun 2025 10:11:55 -0700:
> On 6/16/2025 4:46 AM, Konstantin Andreev wrote:
>> Roberto Sassu, 16 Jun 2025 11:05:11 +0200:
>>> On Mon, 2025-06-16 at 04:07 +0300, Konstantin Andreev wrote:
>>>>
>>>> [irrelevant portion of the message deleted]
>>>>
>>>> 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.
>>
>> Certainly.
>>
>> But I personally avoid duplication even of small cleanups
>> and avoid multiple returns at the price of few gotos.
> 
> I generally prefer
> 
> 	if (xyzzy)
> 		return -ESOMETHING;
> 
> to
> 
> 	if (xyzzy)
> 		goto err_out;
> 	...
> err_out:
> 	return -ESOMETHING;
> 
> I grew up in the era of "gotos considered harmful". When
> err_out does more than just return a goto is fine, but a goto
> that has nothing but a return is unnecessary and adds code that
> needn't be there.

Got it. There is one line more than just return here:

| instant_inode:
|	issp->smk_flags |= (SMK_INODE_INSTANT | transflag);
|	return rc;
| }

What would you prefer in this case, leave it at that or convert to

| if (S_ISSOCK(inode->i_mode)) {
|    /* forced label, no need to save to xattrs */
|    issp->smk_inode = &smack_known_star;
|    // goto instant_inode; // <<<< to be removed
|    issp->smk_flags |= SMK_INODE_INSTANT;
|    return 0;
| }


[the rest of the message deleted]

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

[Casey Schaufler]

On 6/16/2025 10:53 AM, Konstantin Andreev wrote:
> Casey Schaufler, 16 Jun 2025 10:11:55 -0700:
>> On 6/16/2025 4:46 AM, Konstantin Andreev wrote:
>>> Roberto Sassu, 16 Jun 2025 11:05:11 +0200:
>>>> On Mon, 2025-06-16 at 04:07 +0300, Konstantin Andreev wrote:
>>>>>
>>>>> [irrelevant portion of the message deleted]
>>>>>
>>>>> 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.
>>>
>>> Certainly.
>>>
>>> But I personally avoid duplication even of small cleanups
>>> and avoid multiple returns at the price of few gotos.
>>
>> I generally prefer
>>
>>     if (xyzzy)
>>         return -ESOMETHING;
>>
>> to
>>
>>     if (xyzzy)
>>         goto err_out;
>>     ...
>> err_out:
>>     return -ESOMETHING;
>>
>> I grew up in the era of "gotos considered harmful". When
>> err_out does more than just return a goto is fine, but a goto
>> that has nothing but a return is unnecessary and adds code that
>> needn't be there.
>
> Got it. There is one line more than just return here:
>
> | instant_inode:
> |    issp->smk_flags |= (SMK_INODE_INSTANT | transflag);
> |    return rc;
> | }

Leave it as is. I haven't tried out the changes yet,
and will review more fully once I've verified they work
correctly.

>
> What would you prefer in this case, leave it at that or convert to
>
> | if (S_ISSOCK(inode->i_mode)) {
> |    /* forced label, no need to save to xattrs */
> |    issp->smk_inode = &smack_known_star;
> |    // goto instant_inode; // <<<< to be removed
> |    issp->smk_flags |= SMK_INODE_INSTANT;
> |    return 0;
> | }
>
>
> [the rest of the message deleted]
>

Re: [PATCH 0/5] smack: fix bugs: invalid unix socket label, invalid transmute attr

[Casey Schaufler]

On 6/15/2025 6:07 PM, Konstantin Andreev wrote:
> Formerly, Smack inode security was initialized
> by smack_d_instantiate() for all inodes,
> except ones under /proc

I have taken this patch set into smack-next.

>
> Commit [1] 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().
>
> This patch series fixes 2 particular omissions
> I faced directly:
>
> 1) special handling of unix socket files (5th patch)
> 2) S_ISDIR check for "transmute" xattr (2nd patch)
>
> I did not check for other omissions,
> but there may be ones.
>
> Patches 1,3,4 are necessary optimizations
> in smack_inode_init_security() made along the way.
>
> I structured the changes this way to make the review
> process easier.
>
> The patch set applies on top of:
> https://github.com/cschaufler/smack-next/commits/next
> commit 4b59f4fd0a36
>
> [1] 2023-11-16 roberto.sassu
> commit 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/
>
> Konstantin Andreev (5):
>   smack: deduplicate "does access rule request transmutation"
>   smack: fix bug: SMACK64TRANSMUTE set on non-directory
>   smack: deduplicate xattr setting in smack_inode_init_security()
>   smack: always "instantiate" inode in smack_inode_init_security()
>   smack: fix bug: invalid label of unix socket file
>
>  Documentation/admin-guide/LSM/Smack.rst |   5 +
>  security/smack/smack_lsm.c              | 159 +++++++++++++++---------
>  2 files changed, 107 insertions(+), 57 deletions(-)
>