From 635a01da8385fc00a144ec24684100bd1aa9db11 Mon Sep 17 00:00:00 2001 From: Konstantin Andreev Date: Mon, 16 Jun 2025 04:07:28 +0300 Subject: [PATCH 1/9] smack: deduplicate "does access rule request transmutation" Signed-off-by: Konstantin Andreev Signed-off-by: Casey Schaufler --- 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 fc340a6f0dde..8629e58ea4fa 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -962,6 +962,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 @@ -977,23 +995,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, @@ -1001,9 +1015,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; /* @@ -1012,8 +1024,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, @@ -1033,11 +1045,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; } @@ -4915,7 +4929,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 @@ -4929,18 +4942,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; } From 195da3ff244deff119c3f5244b464b2236ea1725 Mon Sep 17 00:00:00 2001 From: Konstantin Andreev Date: Mon, 16 Jun 2025 04:07:29 +0300 Subject: [PATCH 2/9] smack: fix bug: SMACK64TRANSMUTE set on non-directory 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 Signed-off-by: Casey Schaufler --- 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 8629e58ea4fa..d6f814aa15ba 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -1027,18 +1027,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; + } } } From 8e5d9f916a9678e2dcbed2289b87efd453e4e052 Mon Sep 17 00:00:00 2001 From: Konstantin Andreev Date: Mon, 16 Jun 2025 04:07:30 +0300 Subject: [PATCH 3/9] smack: deduplicate xattr setting in smack_inode_init_security() Signed-off-by: Konstantin Andreev Signed-off-by: Casey Schaufler --- 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 d6f814aa15ba..8609ae26e365 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -980,6 +980,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 @@ -997,7 +1015,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; @@ -1016,8 +1033,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 @@ -1029,35 +1044,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)); } /** From 69204f6cdb90f56b7ca27966d1080841108fc5de Mon Sep 17 00:00:00 2001 From: Konstantin Andreev Date: Mon, 16 Jun 2025 04:07:31 +0300 Subject: [PATCH 4/9] smack: always "instantiate" inode in smack_inode_init_security() 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 Signed-off-by: Casey Schaufler --- 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 8609ae26e365..5cd19f3498cb 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -1015,6 +1015,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; @@ -1043,18 +1045,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, From 78fc6a94be252b27bb73e4926eed70b5e302a8e0 Mon Sep 17 00:00:00 2001 From: Konstantin Andreev Date: Mon, 16 Jun 2025 04:07:32 +0300 Subject: [PATCH 5/9] smack: fix bug: invalid label of unix socket file 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 Signed-off-by: Casey Schaufler --- 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 5cd19f3498cb..30f20fef3331 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -1020,6 +1020,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. @@ -1056,14 +1066,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; } /** @@ -1337,13 +1349,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 || @@ -1353,7 +1375,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; @@ -1484,12 +1506,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) @@ -3607,7 +3632,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; @@ -4872,6 +4897,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); } From c147e13ea7fe9f118f8c9ba5e96cbd644b00d6b3 Mon Sep 17 00:00:00 2001 From: Konstantin Andreev Date: Tue, 17 Jun 2025 00:32:16 +0300 Subject: [PATCH 6/9] smack: fix bug: unprivileged task can create labels If an unprivileged task is allowed to relabel itself (/smack/relabel-self is not empty), it can freely create new labels by writing their names into own /proc/PID/attr/smack/current This occurs because do_setattr() imports the provided label in advance, before checking "relabel-self" list. This change ensures that the "relabel-self" list is checked before importing the label. Fixes: 38416e53936e ("Smack: limited capability for changing process label") Signed-off-by: Konstantin Andreev Signed-off-by: Casey Schaufler --- security/smack/smack_lsm.c | 41 +++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 30f20fef3331..159ccc1d9847 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3778,8 +3778,8 @@ static int do_setattr(u64 attr, void *value, size_t size) struct task_smack *tsp = smack_cred(current_cred()); struct cred *new; struct smack_known *skp; - struct smack_known_list_elem *sklep; - int rc; + char *labelstr; + int rc = 0; if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel)) return -EPERM; @@ -3790,28 +3790,41 @@ static int do_setattr(u64 attr, void *value, size_t size) if (attr != LSM_ATTR_CURRENT) return -EOPNOTSUPP; - skp = smk_import_entry(value, size); - if (IS_ERR(skp)) - return PTR_ERR(skp); + labelstr = smk_parse_smack(value, size); + if (IS_ERR(labelstr)) + return PTR_ERR(labelstr); /* * No process is ever allowed the web ("@") label * and the star ("*") label. */ - if (skp == &smack_known_web || skp == &smack_known_star) - return -EINVAL; + if (labelstr[1] == '\0' /* '@', '*' */) { + const char c = labelstr[0]; + + if (c == *smack_known_web.smk_known || + c == *smack_known_star.smk_known) { + rc = -EPERM; + goto free_labelstr; + } + } if (!smack_privileged(CAP_MAC_ADMIN)) { - rc = -EPERM; + const struct smack_known_list_elem *sklep; list_for_each_entry(sklep, &tsp->smk_relabel, list) - if (sklep->smk_label == skp) { - rc = 0; - break; - } - if (rc) - return rc; + if (strcmp(sklep->smk_label->smk_known, labelstr) == 0) + goto free_labelstr; + rc = -EPERM; } +free_labelstr: + kfree(labelstr); + if (rc) + return -EPERM; + + skp = smk_import_entry(value, size); + if (IS_ERR(skp)) + return PTR_ERR(skp); + new = prepare_creds(); if (new == NULL) return -ENOMEM; From 674e2b24791cbe8fd5dc8a0aed4cb4404fcd2028 Mon Sep 17 00:00:00 2001 From: Konstantin Andreev Date: Tue, 17 Jun 2025 00:32:17 +0300 Subject: [PATCH 7/9] smack: fix bug: setting task label silently ignores input garbage This command: # echo foo/bar >/proc/$$/attr/smack/current gives the task a label 'foo' w/o indication that label does not match input. Setting the label with lsm_set_self_attr() syscall behaves identically. This occures because: 1) smk_parse_smack() is used to convert input to a label 2) smk_parse_smack() takes only that part from the beginning of the input that looks like a label. 3) `/' is prohibited in labels, so only "foo" is taken. (2) is by design, because smk_parse_smack() is used for parsing strings which are more than just a label. Silent failure is not a good thing, and there are two indicators that this was not done intentionally: (size >= SMK_LONGLABEL) ~> invalid clause at the beginning of the do_setattr() and the "Returns the length of the smack label" claim in the do_setattr() description. So I fixed this by adding one tiny check: the taken label length == input length. Since input length is now strictly controlled, I changed the two ways of setting label smack_setselfattr(): lsm_set_self_attr() syscall smack_setprocattr(): > /proc/.../current to accommodate the divergence in what they understand by "input length": smack_setselfattr counts mandatory \0 into input length, smack_setprocattr does not. smack_setprocattr allows various trailers after label Related changes: * fixed description for smk_parse_smack * allow unprivileged tasks validate label syntax. * extract smk_parse_label_len() from smk_parse_smack() so parsing may be done w/o string allocation. * extract smk_import_valid_label() from smk_import_entry() to avoid repeated parsing. * smk_parse_smack(): scan null-terminated strings for no more than SMK_LONGLABEL(256) characters * smack_setselfattr(): require struct lsm_ctx . flags == 0 to reserve them for future. Fixes: e114e473771c ("Smack: Simplified Mandatory Access Control Kernel") Signed-off-by: Konstantin Andreev Signed-off-by: Casey Schaufler --- Documentation/admin-guide/LSM/Smack.rst | 11 ++- security/smack/smack.h | 3 + security/smack/smack_access.c | 93 ++++++++++++++----- security/smack/smack_lsm.c | 117 +++++++++++++++--------- 4 files changed, 157 insertions(+), 67 deletions(-) diff --git a/Documentation/admin-guide/LSM/Smack.rst b/Documentation/admin-guide/LSM/Smack.rst index 1b554b5bf98e..c5ed775f2d10 100644 --- a/Documentation/admin-guide/LSM/Smack.rst +++ b/Documentation/admin-guide/LSM/Smack.rst @@ -601,10 +601,15 @@ specification. Task Attribute ~~~~~~~~~~~~~~ -The Smack label of a process can be read from /proc//attr/current. A -process can read its own Smack label from /proc/self/attr/current. A +The Smack label of a process can be read from ``/proc//attr/current``. A +process can read its own Smack label from ``/proc/self/attr/current``. A privileged process can change its own Smack label by writing to -/proc/self/attr/current but not the label of another process. +``/proc/self/attr/current`` but not the label of another process. + +Format of writing is : only the label or the label followed by one of the +3 trailers: ``\n`` (by common agreement for ``/proc/...`` interfaces), +``\0`` (because some applications incorrectly include it), +``\n\0`` (because we think some applications may incorrectly include it). File Attribute ~~~~~~~~~~~~~~ diff --git a/security/smack/smack.h b/security/smack/smack.h index bf6a6ed3946c..759343a6bbae 100644 --- a/security/smack/smack.h +++ b/security/smack/smack.h @@ -286,9 +286,12 @@ int smk_tskacc(struct task_smack *, struct smack_known *, int smk_curacc(struct smack_known *, u32, struct smk_audit_info *); int smack_str_from_perm(char *string, int access); struct smack_known *smack_from_secid(const u32); +int smk_parse_label_len(const char *string, int len); char *smk_parse_smack(const char *string, int len); int smk_netlbl_mls(int, char *, struct netlbl_lsm_secattr *, int); struct smack_known *smk_import_entry(const char *, int); +struct smack_known *smk_import_valid_label(const char *label, int label_len, + gfp_t gfp); void smk_insert_entry(struct smack_known *skp); struct smack_known *smk_find_entry(const char *); bool smack_privileged(int cap); diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c index 2e4a0cb22782..a289cb6672bd 100644 --- a/security/smack/smack_access.c +++ b/security/smack/smack_access.c @@ -443,19 +443,19 @@ struct smack_known *smk_find_entry(const char *string) } /** - * smk_parse_smack - parse smack label from a text string - * @string: a text string that might contain a Smack label - * @len: the maximum size, or zero if it is NULL terminated. + * smk_parse_label_len - calculate the length of the starting segment + * in the string that constitutes a valid smack label + * @string: a text string that might contain a Smack label at the beginning + * @len: the maximum size to look into, may be zero if string is null-terminated * - * Returns a pointer to the clean label or an error code. + * Returns the length of the segment (0 < L < SMK_LONGLABEL) or an error code. */ -char *smk_parse_smack(const char *string, int len) +int smk_parse_label_len(const char *string, int len) { - char *smack; int i; - if (len <= 0) - len = strlen(string) + 1; + if (len <= 0 || len > SMK_LONGLABEL) + len = SMK_LONGLABEL; /* * Reserve a leading '-' as an indicator that @@ -463,7 +463,7 @@ char *smk_parse_smack(const char *string, int len) * including /smack/cipso and /smack/cipso2 */ if (string[0] == '-') - return ERR_PTR(-EINVAL); + return -EINVAL; for (i = 0; i < len; i++) if (string[i] > '~' || string[i] <= ' ' || string[i] == '/' || @@ -471,6 +471,25 @@ char *smk_parse_smack(const char *string, int len) break; if (i == 0 || i >= SMK_LONGLABEL) + return -EINVAL; + + return i; +} + +/** + * smk_parse_smack - copy the starting segment in the string + * that constitutes a valid smack label + * @string: a text string that might contain a Smack label at the beginning + * @len: the maximum size to look into, may be zero if string is null-terminated + * + * Returns a pointer to the copy of the label or an error code. + */ +char *smk_parse_smack(const char *string, int len) +{ + char *smack; + int i = smk_parse_label_len(string, len); + + if (i < 0) return ERR_PTR(-EINVAL); smack = kstrndup(string, i, GFP_NOFS); @@ -554,31 +573,25 @@ int smack_populate_secattr(struct smack_known *skp) } /** - * smk_import_entry - import a label, return the list entry - * @string: a text string that might be a Smack label - * @len: the maximum size, or zero if it is NULL terminated. + * smk_import_valid_allocated_label - import a label, return the list entry + * @smack: a text string that is a valid Smack label and may be kfree()ed. + * It is consumed: either becomes a part of the entry or kfree'ed. * - * Returns a pointer to the entry in the label list that - * matches the passed string, adding it if necessary, - * or an error code. + * Returns: see description of smk_import_entry() */ -struct smack_known *smk_import_entry(const char *string, int len) +static struct smack_known * +smk_import_allocated_label(char *smack, gfp_t gfp) { struct smack_known *skp; - char *smack; int rc; - smack = smk_parse_smack(string, len); - if (IS_ERR(smack)) - return ERR_CAST(smack); - mutex_lock(&smack_known_lock); skp = smk_find_entry(smack); if (skp != NULL) goto freeout; - skp = kzalloc(sizeof(*skp), GFP_NOFS); + skp = kzalloc(sizeof(*skp), gfp); if (skp == NULL) { skp = ERR_PTR(-ENOMEM); goto freeout; @@ -608,6 +621,42 @@ struct smack_known *smk_import_entry(const char *string, int len) return skp; } +/** + * smk_import_entry - import a label, return the list entry + * @string: a text string that might contain a Smack label at the beginning + * @len: the maximum size to look into, may be zero if string is null-terminated + * + * Returns a pointer to the entry in the label list that + * matches the passed string, adding it if necessary, + * or an error code. + */ +struct smack_known *smk_import_entry(const char *string, int len) +{ + char *smack = smk_parse_smack(string, len); + + if (IS_ERR(smack)) + return ERR_CAST(smack); + + return smk_import_allocated_label(smack, GFP_NOFS); +} + +/** + * smk_import_valid_label - import a label, return the list entry + * @label a text string that is a valid Smack label, not null-terminated + * + * Returns: see description of smk_import_entry() + */ +struct smack_known * +smk_import_valid_label(const char *label, int label_len, gfp_t gfp) +{ + char *smack = kstrndup(label, label_len, gfp); + + if (!smack) + return ERR_PTR(-ENOMEM); + + return smk_import_allocated_label(smack, gfp); +} + /** * smack_from_secid - find the Smack label associated with a secid * @secid: an integer that might be associated with a Smack label diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 159ccc1d9847..adf1c542d213 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3710,7 +3710,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode) * @attr: which attribute to fetch * @ctx: buffer to receive the result * @size: available size in, actual size out - * @flags: unused + * @flags: reserved, currently zero * * Fill the passed user space @ctx with the details of the requested * attribute. @@ -3771,57 +3771,52 @@ static int smack_getprocattr(struct task_struct *p, const char *name, char **val * Sets the Smack value of the task. Only setting self * is permitted and only with privilege * - * Returns the length of the smack label or an error code + * Returns zero on success or an error code */ -static int do_setattr(u64 attr, void *value, size_t size) +static int do_setattr(unsigned int attr, void *value, size_t size) { struct task_smack *tsp = smack_cred(current_cred()); struct cred *new; struct smack_known *skp; - char *labelstr; - int rc = 0; - - if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel)) - return -EPERM; + int label_len; + /* + * let unprivileged user validate input, check permissions later + */ if (value == NULL || size == 0 || size >= SMK_LONGLABEL) return -EINVAL; - if (attr != LSM_ATTR_CURRENT) - return -EOPNOTSUPP; - - labelstr = smk_parse_smack(value, size); - if (IS_ERR(labelstr)) - return PTR_ERR(labelstr); + label_len = smk_parse_label_len(value, size); + if (label_len < 0 || label_len != size) + return -EINVAL; /* * No process is ever allowed the web ("@") label * and the star ("*") label. */ - if (labelstr[1] == '\0' /* '@', '*' */) { - const char c = labelstr[0]; + if (label_len == 1 /* '@', '*' */) { + const char c = *(const char *)value; if (c == *smack_known_web.smk_known || - c == *smack_known_star.smk_known) { - rc = -EPERM; - goto free_labelstr; - } + c == *smack_known_star.smk_known) + return -EPERM; } if (!smack_privileged(CAP_MAC_ADMIN)) { const struct smack_known_list_elem *sklep; - list_for_each_entry(sklep, &tsp->smk_relabel, list) - if (strcmp(sklep->smk_label->smk_known, labelstr) == 0) - goto free_labelstr; - rc = -EPERM; + list_for_each_entry(sklep, &tsp->smk_relabel, list) { + const char *cp = sklep->smk_label->smk_known; + + if (strlen(cp) == label_len && + strncmp(cp, value, label_len) == 0) + goto in_relabel; + } + return -EPERM; +in_relabel: + ; } -free_labelstr: - kfree(labelstr); - if (rc) - return -EPERM; - - skp = smk_import_entry(value, size); + skp = smk_import_valid_label(value, label_len, GFP_KERNEL); if (IS_ERR(skp)) return PTR_ERR(skp); @@ -3837,7 +3832,7 @@ static int do_setattr(u64 attr, void *value, size_t size) smk_destroy_label_list(&tsp->smk_relabel); commit_creds(new); - return size; + return 0; } /** @@ -3845,7 +3840,7 @@ static int do_setattr(u64 attr, void *value, size_t size) * @attr: which attribute to set * @ctx: buffer containing the data * @size: size of @ctx - * @flags: unused + * @flags: reserved, must be zero * * Fill the passed user space @ctx with the details of the requested * attribute. @@ -3855,12 +3850,26 @@ static int do_setattr(u64 attr, void *value, size_t size) static int smack_setselfattr(unsigned int attr, struct lsm_ctx *ctx, u32 size, u32 flags) { - int rc; + if (attr != LSM_ATTR_CURRENT) + return -EOPNOTSUPP; - rc = do_setattr(attr, ctx->ctx, ctx->ctx_len); - if (rc > 0) - return 0; - return rc; + if (ctx->flags) + return -EINVAL; + /* + * string must have \0 terminator, included in ctx->ctx + * (see description of struct lsm_ctx) + */ + if (ctx->ctx_len == 0) + return -EINVAL; + + if (ctx->ctx[ctx->ctx_len - 1] != '\0') + return -EINVAL; + /* + * other do_setattr() caller, smack_setprocattr(), + * does not count \0 into size, so + * decreasing length by 1 to accommodate the divergence. + */ + return do_setattr(attr, ctx->ctx, ctx->ctx_len - 1); } /** @@ -3872,15 +3881,39 @@ static int smack_setselfattr(unsigned int attr, struct lsm_ctx *ctx, * Sets the Smack value of the task. Only setting self * is permitted and only with privilege * - * Returns the length of the smack label or an error code + * Returns the size of the input value or an error code */ static int smack_setprocattr(const char *name, void *value, size_t size) { - int attr = lsm_name_to_attr(name); + size_t realsize = size; + unsigned int attr = lsm_name_to_attr(name); - if (attr != LSM_ATTR_UNDEF) - return do_setattr(attr, value, size); - return -EINVAL; + switch (attr) { + case LSM_ATTR_UNDEF: return -EINVAL; + default: return -EOPNOTSUPP; + case LSM_ATTR_CURRENT: + ; + } + + /* + * The value for the "current" attribute is the label + * followed by one of the 4 trailers: none, \0, \n, \n\0 + * + * I.e. following inputs are accepted as 3-characters long label "foo": + * + * "foo" (3 characters) + * "foo\0" (4 characters) + * "foo\n" (4 characters) + * "foo\n\0" (5 characters) + */ + + if (realsize && (((const char *)value)[realsize - 1] == '\0')) + --realsize; + + if (realsize && (((const char *)value)[realsize - 1] == '\n')) + --realsize; + + return do_setattr(attr, value, realsize) ? : size; } /** From 6ddd169d0288ebee8ec9afcd75825ce8476edb00 Mon Sep 17 00:00:00 2001 From: Konstantin Andreev Date: Thu, 26 Jun 2025 19:47:29 +0300 Subject: [PATCH 8/9] smack: fix kernel-doc warnings for smk_import_valid_label() Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202506251712.x5SJiNlh-lkp@intel.com/ Signed-off-by: Konstantin Andreev Signed-off-by: Casey Schaufler --- security/smack/smack_access.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c index a289cb6672bd..09167be79122 100644 --- a/security/smack/smack_access.c +++ b/security/smack/smack_access.c @@ -642,9 +642,11 @@ struct smack_known *smk_import_entry(const char *string, int len) /** * smk_import_valid_label - import a label, return the list entry - * @label a text string that is a valid Smack label, not null-terminated + * @label: a text string that is a valid Smack label, not null-terminated + * @label_len: the length of the text string in the @label + * @gfp: the GFP mask used for allocating memory for the @label text string copy * - * Returns: see description of smk_import_entry() + * Return: see description of smk_import_entry() */ struct smack_known * smk_import_valid_label(const char *label, int label_len, gfp_t gfp) From 29c701f90b9341f1f9c1854a9c22b71c2318457d Mon Sep 17 00:00:00 2001 From: Casey Schaufler Date: Tue, 11 Nov 2025 12:00:18 -0800 Subject: [PATCH 9/9] Smack: function parameter 'gfp' not described Add a descrition of the gfp parameter to smk_import_allocated_label(). Signed-off-by: Casey Schaufler Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202511061746.dPegBnNf-lkp@intel.com/ --- security/smack/smack_access.c | 1 + 1 file changed, 1 insertion(+) diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c index 09167be79122..fc507dcc7ea5 100644 --- a/security/smack/smack_access.c +++ b/security/smack/smack_access.c @@ -576,6 +576,7 @@ int smack_populate_secattr(struct smack_known *skp) * smk_import_valid_allocated_label - import a label, return the list entry * @smack: a text string that is a valid Smack label and may be kfree()ed. * It is consumed: either becomes a part of the entry or kfree'ed. + * @gfp: Allocation type * * Returns: see description of smk_import_entry() */