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

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

[Tang Pengke]

Files copied from an untrusted channel (USB, network, serial) inherit the
creating process's Smack label, losing the source's label.  That breaks
provenance and lets a file from an untrusted source silently acquire a
trusted label.

Record a "preserve" label when a process opens a whitelisted regular file
read-only and the read check passes.  The next inode it creates inherits
that label instead of the process label, unless transmutation applies.
The record is one-shot: consumed on creation, dropped when transmutation
takes precedence, cleared on exec, and not inherited across fork.

An overwrite -- O_TRUNC on a file that already exists -- never reaches
inode_init_security(), so the contents would come from the untrusted
source while the file kept its old label.  The record is therefore also
handed to an overwrite target at open time and written back when the file
is released, which is the first point where the inode lock is gone and
the xattr can be updated.  Only a target that carries the task's own
label, or one already carrying a preservable label, is relabelled; a
trusted file keeps its label.

The source record lives in a per-task LSM blob rather than in the cred.  A
cred is immutable once committed and is shared by every thread of a
process; writing the record there would be a data race between threads,
and one thread could consume a label another thread had just recorded.
The overwrite record lives in the file blob instead, so two overwrites in
flight cannot clobber each other.

The preservable labels form a whitelist exposed through
/smack/preserve-whitelist.  Only untrusted-channel / low-integrity labels
should be added.  Recording happens only after a successful read check, so
a process cannot create a file with a label it could not read (which
would be a write-down primitive), and because the whitelist holds only low
labels the mechanism can only ever preserve a low label, never a system
trusted or sensitive one.  An empty whitelist is also the fast path in
smack_file_open().

Known limitation: the kernel cannot distinguish a copy from an unrelated
create, so a task that opens a whitelisted file and then creates an
unrelated file will label the latter with the preserved low label.
Restricting the whitelist to low-integrity labels bounds the impact.

The whole mechanism is guarded by CONFIG_SECURITY_SMACK_COPYWHITELIST,
which defaults to no.  With the option off nothing is compiled in: no
hooks are registered, the file blob keeps its original size, and the
existing accessors are unchanged, so the default Smack behavior is
unaffected and the patch is purely additive.

Signed-off-by: Tang Pengke <tangpengke@yhkylin.cn>
---
Changes in v4:
- Narrow the preserve-label scope: only labels listed on preserve-whitelist
  are recorded, and the list holds untrusted-channel labels only.  This
  keeps shared-library loads from ever arming a record and makes the
  write-down primitive impossible by construction rather than by policy.
- Move record clearing from bprm_creds_for_exec to bprm_committing_creds.
  The former also runs for AT_EXECVE_CHECK and for execs that fail later,
  which would discard a still-valid record.
- Skip FMODE_EXEC opens in smack_file_open so the exec path never arms a
  record.
- Require MAY_WRITE on the recorded label before inheriting it in
  smack_inode_init_security.  A task that may read a label but not write
  it must not be able to mint a file carrying that label.
- Skip FMODE_CREATED opens in smack_file_open: the create already consumed
  the record, and re-arming it there would turn the one-shot record into
  an endless source.
- Handle overlayfs copy-up: set a one-shot smk_copy_up marker in
  smack_inode_copy_up and skip both transmutation and any pending record
  in smack_inode_init_security, so the upper inode keeps the lower label
  instead of being relabelled with the upper directory's label.
- Propagate allocation failure from smk_preserve_set() instead of
  silently ignoring it.
- Validate the preserve-whitelist write path: reject ppos != 0 and
  count == 0 or count > SMK_LONGLABEL, and import the label so it is
  interned like every other label.
- Carry the label across an overwrite too.  Opening an existing file with
  O_TRUNC never reaches inode_init_security(), so the contents could come
  from an untrusted source while the file kept its old label; an
  executable file would then run untrusted content.  Hand the record to
  the target at open time and write it back in file_free_security(),
  which runs outside the inode lock.  Only a target carrying the task's
  own label, or one already carrying a preservable label, is relabelled,
  so a trusted file cannot be downgraded this way.  The record lives in
  the file blob rather than the task blob, so two overwrites in flight in
  one task cannot clobber each other's record.
- Drop a preserve record when its source file is released.  If the copy
  never creates anything, the record survived and was then inherited by
  an unrelated later create.  Store the source inode with the record and
  clear both in a new file_free_security hook.
- Take smk_preserve_wl_lock in the seq_file iterator.
- Take the inode lock around the release-time setxattr.  Filesystems such
  as UBIFS assert inode_is_locked() in their xattr_set handler, so calling
  __vfs_setxattr_noperm() without it trips the assertion and puts the
  filesystem into read-only mode.
- All of the above is inside CONFIG_SECURITY_SMACK_COPYWHITELIST.  With the
  option off nothing is compiled in: no hooks are registered, the file blob
  keeps its original size, and smack_file() and its callers are unchanged.

 security/smack/Kconfig        |  21 +++
 security/smack/smack.h        |  61 +++++++++
 security/smack/smack_access.c |  78 +++++++++++
 security/smack/smack_lsm.c    | 250 ++++++++++++++++++++++++++++++++++
 security/smack/smackfs.c      | 120 ++++++++++++++++
 5 files changed, 530 insertions(+)

diff --git a/security/smack/Kconfig b/security/smack/Kconfig
index 5a8dfad46..134449fb6 100644
--- a/security/smack/Kconfig
+++ b/security/smack/Kconfig
@@ -53,3 +53,24 @@ config SECURITY_SMACK_APPEND_SIGNALS
 	  to differentiate between delivering a network packet and
 	  delivering a signal in the Smack rules.
 	  If you are unsure how to answer this question, answer N.
+
+config SECURITY_SMACK_COPYWHITELIST
+	bool "Preserve low-integrity labels when copying files"
+	depends on SECURITY_SMACK
+	default n
+	help
+	  Enable a whitelist of labels that are preserved when a file is
+	  copied. When a process reads a file whose label is on the
+	  whitelist and then creates a new file, the new file inherits
+	  the source label instead of the process label. This is useful
+	  for tracking the provenance of data copied from untrusted
+	  sources such as removable media or network mounts, where the
+	  filesystem cannot store a per-file label.
+
+	  Only low-integrity labels should be placed on the whitelist,
+	  so that a process can only ever preserve a low label and never
+	  a trusted or sensitive one. The whitelist is managed through
+	  the /smack/preserve-whitelist interface.
+
+	  If you are unsure how to answer this question, answer N.
+
diff --git a/security/smack/smack.h b/security/smack/smack.h
index 9b9eb262f..fd366425d 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -73,6 +73,25 @@ struct smack_known {
 	struct mutex			smk_rules_lock;	/* lock for rules */
 };
 
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+/*
+ * An entry in the preserve whitelist.
+ *
+ * The whitelist is the single source of truth for which labels are
+ * preservable.  Its emptiness is the fast path in smack_file_open(),
+ * and membership is tested by walking the list.
+ *
+ * Only untrusted-channel / low-integrity labels should be added, so a
+ * process can only ever preserve a low label and never a system trusted
+ * label (which would be a write-down primitive and let shared-library
+ * loads pollute the record).
+ */
+struct smk_preserve_wl {
+	struct list_head		list;
+	struct smack_known		*skp;
+};
+#endif
+
 /*
  * Maximum number of bytes for the levels in a CIPSO IP option.
  * Why 23? CIPSO is constrained to 30, so a 32 byte buffer is
@@ -126,6 +145,21 @@ struct task_smack {
 	struct list_head	smk_relabel;	/* transit allowed labels */
 };
 
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+/*
+ * Per-thread state for a single copy operation.  This is deliberately
+ * kept out of the cred: credentials are immutable once committed and are
+ * shared by every thread of a process, while the label to preserve
+ * belongs to one copy and must not be visible to, or raced by, the other
+ * threads.
+ */
+struct smack_task {
+	struct smack_known	*smk_preserve;	/* label to inherit on next create */
+	struct inode		*smk_preserve_inode;	/* inode the record came from */
+	bool			smk_copy_up;	/* overlayfs copy-up in progress */
+};
+#endif
+
 #define	SMK_INODE_INSTANT	0x01	/* inode is instantiated */
 #define	SMK_INODE_TRANSMUTE	0x02	/* directory is transmuting */
 #define	SMK_INODE_CHANGED	0x04	/* smack was transmuted (unused) */
@@ -312,6 +346,10 @@ bool smack_privileged(int cap);
 bool smack_privileged_cred(int cap, const struct cred *cred);
 void smk_destroy_label_list(struct list_head *list);
 int smack_populate_secattr(struct smack_known *skp);
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+bool smk_preserve_allowed(struct smack_known *skp);
+int smk_preserve_set(struct smack_known *skp, bool on);
+#endif
 
 /*
  * Shared data.
@@ -326,6 +364,10 @@ extern struct smack_known *smack_unconfined;
 #endif
 extern int smack_ptrace_rule;
 extern struct lsm_blob_sizes smack_blob_sizes;
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+extern struct list_head smk_preserve_wl;
+extern struct mutex smk_preserve_wl_lock;
+#endif
 
 extern struct smack_known smack_known_floor;
 extern struct smack_known smack_known_hat;
@@ -352,12 +394,31 @@ static inline struct task_smack *smack_cred(const struct cred *cred)
 	return cred->security + smack_blob_sizes.lbs_cred;
 }
 
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+static inline struct smack_task *smack_task(const struct task_struct *task)
+{
+	return task->security + smack_blob_sizes.lbs_task;
+}
+#endif
+
 static inline struct smack_known **smack_file(const struct file *file)
 {
 	return (struct smack_known **)(file->f_security +
 				       smack_blob_sizes.lbs_file);
 }
 
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+/*
+ * Per-file overwrite record, stored right after smack_file().  Keeping it
+ * separate leaves the existing blob layout untouched when the feature is
+ * off, and two overwrites in flight cannot clobber each other's record.
+ */
+static inline struct smack_known **smack_file_overwrite(const struct file *file)
+{
+	return smack_file(file) + 1;
+}
+#endif
+
 static inline struct inode_smack *smack_inode(const struct inode *inode)
 {
 	return inode->i_security + smack_blob_sizes.lbs_inode;
diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
index 350b88d58..421d76212 100644
--- a/security/smack/smack_access.c
+++ b/security/smack/smack_access.c
@@ -53,6 +53,84 @@ static u32 smack_next_secid = 10;
 int log_policy = SMACK_AUDIT_DENIED;
 #endif /* CONFIG_AUDIT */
 
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+/*
+ * Preserve whitelist: only labels on this list are preserved on copy.
+ *
+ * The list is the single source of truth for "which labels are
+ * preservable". It also lets smack_file_open() take a plain
+ * list_empty() fast path.
+ *
+ * Only untrusted-channel / low-integrity labels should be added, so
+ * that a process can only ever preserve a low label -- never a system
+ * trusted or sensitive label -- which would otherwise be a write-down
+ * primitive (and would let shared-library loads pollute the record).
+ *
+ * Entries hold raw smack_known pointers. SMACK global labels are never
+ * freed at runtime (smk_destroy_label_list frees list elements, not the
+ * label objects), so these pointers stay valid for the life of the
+ * system.
+ */
+LIST_HEAD(smk_preserve_wl);
+DEFINE_MUTEX(smk_preserve_wl_lock);
+
+bool smk_preserve_allowed(struct smack_known *skp)
+{
+	struct smk_preserve_wl *e;
+
+	mutex_lock(&smk_preserve_wl_lock);
+	list_for_each_entry(e, &smk_preserve_wl, list) {
+		if (e->skp == skp) {
+			mutex_unlock(&smk_preserve_wl_lock);
+			return true;
+		}
+	}
+	mutex_unlock(&smk_preserve_wl_lock);
+	return false;
+}
+
+/*
+ * Add or remove a label from the preserve whitelist.
+ *
+ * The sysfs interface (preserve-whitelist) calls this with on=true for
+ * a plain label, and on=false for a label prefixed with '-'.
+ */
+int smk_preserve_set(struct smack_known *skp, bool on)
+{
+	struct smk_preserve_wl *e, *new = NULL;
+
+	if (on) {
+		new = kzalloc_obj(*new, GFP_KERNEL);
+		if (!new)
+			return -ENOMEM;
+		new->skp = skp;
+	}
+
+	mutex_lock(&smk_preserve_wl_lock);
+	if (on) {
+		list_for_each_entry(e, &smk_preserve_wl, list) {
+			if (e->skp == skp) {
+				kfree(new);
+				new = NULL;
+				break;
+			}
+		}
+		if (new)
+			list_add_tail(&new->list, &smk_preserve_wl);
+	} else {
+		list_for_each_entry(e, &smk_preserve_wl, list) {
+			if (e->skp == skp) {
+				list_del(&e->list);
+				kfree(e);
+				break;
+			}
+		}
+	}
+	mutex_unlock(&smk_preserve_wl_lock);
+	return 0;
+}
+#endif /* CONFIG_SECURITY_SMACK_COPYWHITELIST */
+
 /**
  * smk_access_entry - look up matching access rule
  * @subject_label: a pointer to the subject's Smack label
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index ff115068c..e18f06f03 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -944,6 +944,23 @@ static int smack_bprm_creds_for_exec(struct linux_binprm *bprm)
 	return 0;
 }
 
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+/**
+ * smack_bprm_committing_creds - exec is about to commit the new creds
+ * @bprm: the exec information
+ *
+ * Every failure path has been passed at this point, so a pending copy
+ * record is stale.  Clearing it here rather than in the earlier
+ * bprm_creds_for_exec hook matters: that hook also runs for
+ * AT_EXECVE_CHECK and for execs that fail afterwards, and clearing there
+ * would drop the record even though no exec happened.
+ */
+static void smack_bprm_committing_creds(const struct linux_binprm *bprm)
+{
+	smack_task(current)->smk_preserve = NULL;
+}
+#endif
+
 /*
  * Inode hooks
  */
@@ -998,6 +1015,31 @@ xattr_dupval(struct xattr *xattrs, int *xattr_count,
 	return 0;
 }
 
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+/**
+ * smack_copy_up_begin - consume the one-shot overlayfs copy-up marker
+ *
+ * Overlayfs creates the upper inode through inode_init_security() and must
+ * keep the lower label.  The marker makes that path leave both the inode
+ * and any pending copy record alone, so it is consumed here, before either
+ * branch below can look at it.
+ */
+static bool smack_copy_up_begin(void)
+{
+	struct smack_task *stp = smack_task(current);
+	bool copy_up = stp->smk_copy_up;
+
+	if (copy_up)
+		stp->smk_copy_up = false;
+	return copy_up;
+}
+#else
+static inline bool smack_copy_up_begin(void)
+{
+	return false;
+}
+#endif
+
 /**
  * smack_inode_init_security - copy out the smack from an inode
  * @inode: the newly created inode
@@ -1019,6 +1061,10 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 	int transflag = 0;
 	bool trans_cred;
 	bool trans_rule;
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+	bool transmutes;
+	bool copy_up = smack_copy_up_begin();
+#endif
 
 	/*
 	 * UNIX domain sockets use lower level socket data. Let
@@ -1037,6 +1083,9 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 	trans_cred = (tsp->smk_task == tsp->smk_transmuted);
 	if (!trans_cred)
 		trans_rule = smk_rule_transmutes(smk_of_task(tsp), dsp);
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+	transmutes = trans_cred || (trans_rule && smk_inode_transmutable(dir));
+#endif
 
 	/*
 	 * In addition to having smk_task equal to smk_transmuted,
@@ -1044,7 +1093,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
 	 * requests transmutation then by all means transmute.
 	 * Mark the inode as changed.
 	 */
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+	if (!copy_up && transmutes) {
+#else
 	if (trans_cred || (trans_rule && smk_inode_transmutable(dir))) {
+#endif
 		/*
 		 * The caller of smack_dentry_create_files_as()
 		 * should have overridden the current cred, so the
@@ -1066,6 +1119,33 @@ 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, inherit it so
+	 * the new file keeps the source's (low-integrity) label across the
+	 * copy.  Two cases take precedence, and the record is one-shot so
+	 * it is dropped either way:
+	 *
+	 *  - a copy-up, which must keep the lower label untouched;
+	 *  - a transmutation, otherwise the record would leak into a later
+	 *    create in a non-transmutable directory.
+	 *
+	 * The record is only inherited when the task may also write the
+	 * label.  A task that can read skp but not write it must not be
+	 * able to mint a file carrying skp -- that would be a write into a
+	 * domain the policy keeps it out of.
+	 */
+	if (!copy_up) {
+		struct smack_known *skp = smack_task(current)->smk_preserve;
+
+		if (skp != NULL && !transmutes &&
+		    smk_tskacc(tsp, skp, MAY_WRITE, NULL) == 0)
+			issp->smk_inode = skp;
+		smack_task(current)->smk_preserve = NULL;
+		smack_task(current)->smk_preserve_inode = NULL;
+	}
+#endif
+
 	if (rc == 0)
 		if (xattr_dupval(xattrs, xattr_count,
 			    XATTR_SMACK_SUFFIX,
@@ -2068,10 +2148,160 @@ 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
+	struct smack_task *stp = smack_task(current);
+
+	/*
+	 * Record a whitelisted label after a successful read check so the
+	 * next file create (smack_inode_init_security) can inherit it.
+	 *
+	 * The record must happen only after the read check passes;
+	 * otherwise a process without read access to the source could
+	 * create a file with that label without ever reading it, which
+	 * would be a write-down primitive.
+	 *
+	 * The whitelist must only hold untrusted-channel / low-integrity
+	 * labels (peripheral, network, serial, and other external
+	 * sources), never system trusted labels (system binaries, shared
+	 * libraries, etc.) -- otherwise shared-library loads would pollute
+	 * the record.  Because of this constraint no library path
+	 * filtering is needed here: the whitelist check already excludes
+	 * every non-whitelisted file, including shared libraries.
+	 *
+	 * Skip files this open created (FMODE_CREATED): the create already
+	 * consumed the record, and re-arming it here would turn the
+	 * one-shot into an endless source of copies.  Skip exec opens
+	 * (FMODE_EXEC) as well; the exec path must not arm a record, and
+	 * an exec that later fails would otherwise leave one behind.
+	 *
+	 * The list_empty() check is a lockless fast path.  A concurrent
+	 * whitelist update can only make us miss recording once; it can
+	 * never make us record a label that is not on the list, because
+	 * the authoritative test is smk_preserve_allowed() below, which
+	 * does take the lock.
+	 */
+	if (!(file->f_mode & (FMODE_CREATED | FMODE_EXEC)) &&
+	    !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)) {
+			stp->smk_preserve = skp;
+			stp->smk_preserve_inode = inode;
+		}
+	}
+
+	/*
+	 * An overwrite skips inode_init_security(), so hand the record to
+	 * the file and apply it on release.  O_TRUNC is still set here:
+	 * do_dentry_open() prunes it only after security_file_open(), and
+	 * handle_truncate() runs later still.  That is why the
+	 * file_truncate hook cannot serve instead -- by then the flag is
+	 * gone and ftruncate(2) is indistinguishable.
+	 *
+	 * Only a file carrying the task's own label, or a preservable one,
+	 * is relabelled; a trusted file keeps its label.
+	 */
+	if (stp->smk_preserve != NULL &&
+	    !(file->f_mode & FMODE_CREATED) &&
+	    (file->f_flags & O_ACCMODE) == O_WRONLY &&
+	    (file->f_flags & O_TRUNC) &&
+	    S_ISREG(inode->i_mode)) {
+		struct smack_known *tgt = smk_of_inode(inode);
+
+		if ((tgt == smk_of_task(tsp) || smk_preserve_allowed(tgt)) &&
+		    smk_tskacc(tsp, tgt, MAY_WRITE, NULL) == 0)
+			*smack_file_overwrite(file) = stp->smk_preserve;
+		stp->smk_preserve = NULL;
+		stp->smk_preserve_inode = NULL;
+	}
+#endif
 
 	return rc;
 }
 
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+/**
+ * smk_relabel_on_release - give a file the label of what was copied into it
+ * @file: the file being released
+ * @skp: label to apply
+ *
+ * Write the xattr before adopting the label in memory, so a failed write
+ * leaves the inode matching the disk.
+ *
+ * __vfs_setxattr_noperm() skips security_inode_setxattr() on purpose:
+ * vfs_setxattr() would reach smack_inode_setxattr() and demand
+ * CAP_MAC_ADMIN, while the label was already vetted in smack_file_open()
+ * against a narrower rule.
+ */
+static void smk_relabel_on_release(struct file *file, struct smack_known *skp)
+{
+	struct dentry *dentry = file->f_path.dentry;
+	struct inode *inode = d_inode(dentry);
+	struct inode_smack *isp = smack_inode(inode);
+	int rc;
+
+	if (isp->smk_inode == skp)
+		return;
+
+	/*
+	 * __vfs_setxattr_noperm() requires the inode lock: filesystems such
+	 * as UBIFS assert inode_is_locked() in their xattr_set handler.
+	 * __fput() runs from task_work, so taking the lock here is safe.
+	 */
+	inode_lock(inode);
+	rc = __vfs_setxattr_noperm(file_mnt_idmap(file), dentry,
+				   XATTR_NAME_SMACK,
+				   skp->smk_known, strlen(skp->smk_known), 0);
+	inode_unlock(inode);
+
+	if (rc == 0)
+		isp->smk_inode = skp;
+}
+#endif
+
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+/**
+ * smack_file_free_security - finish a copy, drop stale records
+ * @file: the file being freed
+ *
+ * Two records can be outstanding:
+ *
+ *  - the overwrite label, kept in the file blob.  Its contents are
+ *    complete and the inode lock is gone, so this is the first safe
+ *    point to write it back;
+ *  - a source record, kept in the task blob, whose source is released
+ *    before any create consumed it.  It is stale; dropping it keeps an
+ *    unrelated later create from inheriting a label it never read.
+ *
+ * The task blob lookup uses current.  fput() normally defers to
+ * task_work on the closing task, which is correct; should it fall back
+ * to ____fput() from a workqueue the lookup misses and the stale record
+ * survives until the next open of a whitelisted file replaces it.  The
+ * overwrite record is unaffected because it lives in the file blob.
+ */
+static void smack_file_free_security(struct file *file)
+{
+	struct inode *inode = file_inode(file);
+	struct smack_task *stp = smack_task(current);
+	struct smack_known *skp = *smack_file_overwrite(file);
+
+	if (skp != NULL) {
+		*smack_file_overwrite(file) = NULL;
+		smk_relabel_on_release(file, skp);
+	}
+
+	if (stp->smk_preserve_inode == inode) {
+		stp->smk_preserve = NULL;
+		stp->smk_preserve_inode = NULL;
+	}
+}
+#endif
+
 /*
  * Task hooks
  */
@@ -4983,6 +5213,15 @@ static int smack_inode_copy_up(struct dentry *dentry, struct cred **new)
 	isp = smack_inode(d_inode(dentry));
 	skp = isp->smk_inode;
 	tsp->smk_task = skp;
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+	/*
+	 * The copy-up creates upper inodes through the normal create
+	 * path.  Tell smack_inode_init_security() to leave those alone:
+	 * they must keep the lower label, and a pending copy record
+	 * would otherwise be consumed by an internal overlayfs create.
+	 */
+	smack_task(current)->smk_copy_up = true;
+#endif
 	*new = new_creds;
 	return 0;
 }
@@ -5106,7 +5345,12 @@ static int smack_uring_cmd(struct io_uring_cmd *ioucmd)
 
 struct lsm_blob_sizes smack_blob_sizes __ro_after_init = {
 	.lbs_cred = sizeof(struct task_smack),
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+	.lbs_task = sizeof(struct smack_task),
+	.lbs_file = 2 * sizeof(struct smack_known *),
+#else
 	.lbs_file = sizeof(struct smack_known *),
+#endif
 	.lbs_inode = sizeof(struct inode_smack),
 	.lbs_ipc = sizeof(struct smack_known *),
 	.lbs_key = sizeof(struct smack_known *),
@@ -5137,6 +5381,9 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(sb_set_mnt_opts, smack_set_mnt_opts),
 
 	LSM_HOOK_INIT(bprm_creds_for_exec, smack_bprm_creds_for_exec),
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+	LSM_HOOK_INIT(bprm_committing_creds, smack_bprm_committing_creds),
+#endif
 
 	LSM_HOOK_INIT(inode_alloc_security, smack_inode_alloc_security),
 	LSM_HOOK_INIT(inode_init_security, smack_inode_init_security),
@@ -5172,6 +5419,9 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(file_receive, smack_file_receive),
 
 	LSM_HOOK_INIT(file_open, smack_file_open),
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+	LSM_HOOK_INIT(file_free_security, smack_file_free_security),
+#endif
 
 	LSM_HOOK_INIT(cred_alloc_blank, smack_cred_alloc_blank),
 	LSM_HOOK_INIT(cred_free, smack_cred_free),
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 6e62dcb36..5023c30fb 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -62,6 +62,9 @@ enum smk_inos {
 	SMK_NET6ADDR	= 23,	/* single label IPv6 hosts */
 #endif /* CONFIG_IPV6 */
 	SMK_RELABEL_SELF = 24, /* relabel possible without CAP_MAC_ADMIN */
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+	SMK_PRESERVE_WL	= 25,	/* preserve whitelist labels */
+#endif
 };
 
 /*
@@ -2867,6 +2870,118 @@ static const struct file_operations smk_ptrace_ops = {
 	.llseek		= default_llseek,
 };
 
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+/*
+ * Seq_file operations for /smack/preserve-whitelist.
+ * Iterates the whitelist and shows each preservable label.
+ */
+static void *preserve_wl_seq_start(struct seq_file *s, loff_t *pos)
+{
+	mutex_lock(&smk_preserve_wl_lock);
+	return seq_list_start(&smk_preserve_wl, *pos);
+}
+
+static void *preserve_wl_seq_next(struct seq_file *s, void *v, loff_t *pos)
+{
+	return seq_list_next(v, &smk_preserve_wl, pos);
+}
+
+static void preserve_wl_seq_stop(struct seq_file *s, void *v)
+{
+	mutex_unlock(&smk_preserve_wl_lock);
+}
+
+static int preserve_wl_seq_show(struct seq_file *s, void *v)
+{
+	struct smk_preserve_wl *e = list_entry(v, struct smk_preserve_wl, list);
+
+	seq_printf(s, "%s\n", e->skp->smk_known);
+	return 0;
+}
+
+static const struct seq_operations preserve_wl_seq_ops = {
+	.start = preserve_wl_seq_start,
+	.next  = preserve_wl_seq_next,
+	.stop  = preserve_wl_seq_stop,
+	.show  = preserve_wl_seq_show,
+};
+
+static int smk_open_preserve_wl(struct inode *inode, struct file *file)
+{
+	return seq_open(file, &preserve_wl_seq_ops);
+}
+
+static ssize_t smk_write_preserve_wl(struct file *file, const char __user *buf,
+				     size_t count, loff_t *ppos)
+{
+	struct smack_known *skp;
+	char *data, *label;
+	bool on = true;
+	int rc;
+
+	if (!smack_privileged(CAP_MAC_ADMIN))
+		return -EPERM;
+
+	if (*ppos != 0)
+		return -EINVAL;
+
+	if (count == 0 || count > SMK_LONGLABEL)
+		return -EINVAL;
+
+	data = memdup_user_nul(buf, count);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	label = strim(data);
+	if (!label[0]) {
+		kfree(data);
+		return -EINVAL;
+	}
+
+	/* A leading '-' removes the label (same convention as revoke-subject). */
+	if (label[0] == '-') {
+		on = false;
+		label = strim(label + 1);
+		if (!label[0]) {
+			kfree(data);
+			return -EINVAL;
+		}
+	}
+
+	/*
+	 * Import the label when adding.  Boot-time policy may name a label
+	 * that no inode, rule or mount has referenced yet, in which case it
+	 * is not in smack_known_hash.  Removal only needs the existing entry.
+	 */
+	if (on) {
+		skp = smk_import_entry(label, 0);
+		if (IS_ERR(skp)) {
+			rc = PTR_ERR(skp);
+			kfree(data);
+			return rc;
+		}
+	} else {
+		skp = smk_find_entry(label);
+		if (!skp) {
+			kfree(data);
+			return -ENOENT;
+		}
+	}
+
+	rc = smk_preserve_set(skp, on);
+	kfree(data);
+	return rc ? rc : count;
+}
+
+static const struct file_operations smk_preserve_wl_ops = {
+	.open		= smk_open_preserve_wl,
+	.read		= seq_read,
+	.write		= smk_write_preserve_wl,
+	.llseek		= seq_lseek,
+	.release	= seq_release,
+};
+#endif /* CONFIG_SECURITY_SMACK_COPYWHITELIST */
+
 /**
  * smk_fill_super - fill the smackfs superblock
  * @sb: the empty superblock
@@ -2933,6 +3048,11 @@ static int smk_fill_super(struct super_block *sb, struct fs_context *fc)
 		[SMK_RELABEL_SELF] = {
 			"relabel-self", &smk_relabel_self_ops,
 				S_IRUGO|S_IWUGO},
+#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
+		[SMK_PRESERVE_WL] = {
+			"preserve-whitelist", &smk_preserve_wl_ops,
+				S_IRUGO|S_IWUSR},
+#endif
 		/* last one */
 			{""}
 	};
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] smack: UAF and NULL deref in smack_file_free_security() on file release
- [High] smack: missing MAY_WRITE check and O_RDWR handling in smack_file_open()
- [High] smack: OverlayFS copy-up state corruption via smk_copy_up task flag
- [High] smack: ineffective FMODE_EXEC check in smack_file_open() corrupts smk_preserve on exec
- [Medium] smack: unlocked write to isp->smk_inode in smk_relabel_on_release()
- [Medium] smack: flawed source file tracking via smk_preserve_inode on file free
- [Medium] smack: smk_tskacc() == 0 check in smack_inode_init_security() and smack_file_open() breaks bringup/unconfined mode
- [Medium] smack: broken length limit and label parsing in smk_write_preserve_wl()
- [Medium] smack: smk_preserve is not cleared on label transition in do_setattr()
--

commit 1679e7f1a74f034307cf0eb6fcf662b9a39ad174
Author: Tang Pengke <tangpengke@yhkylin.cn>

smack: preserve low-integrity labels across file copy via whitelist

This commit introduces a whitelist mechanism under
CONFIG_SECURITY_SMACK_COPYWHITELIST that records a whitelisted Smack label on
read-only file opens and applies it to the next file created or overwritten
with O_TRUNC by the same task. The whitelist is managed via
/smack/preserve-whitelist.

> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8e88ac65fd7fc..ff3d0b2f76332 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -953,6 +953,23 @@ static int smack_bprm_creds_for_exec(struct linux_binprm *bprm)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
> +/**
> + * smack_bprm_committing_creds - exec is about to commit the new creds
> + * @bprm: the exec information
> + *
> + * Every failure path has been passed at this point, so a pending copy
> + * record is stale.  Clearing it here rather than in the earlier
> + * bprm_creds_for_exec hook matters: that hook also runs for
> + * AT_EXECVE_CHECK and for execs that fail afterwards, and clearing there
> + * would drop the record even though no exec happened.
> + */
> +static void smack_bprm_committing_creds(const struct linux_binprm *bprm)
> +{
> +	smack_task(current)->smk_preserve = NULL;
> +}
> +#endif

[Severity: Medium]
While smack_bprm_committing_creds() clears smk_preserve on exec, what happens
when a task changes its active Smack label without exec via do_setattr() in
security/smack/smack_lsm.c (through smack_setprocattr() on
/proc/self/attr/current or smack_setselfattr())?

Because smk_preserve lives in the per-task blob rather than struct cred,
do_setattr() updates tsp->smk_task and calls commit_creds(new) while leaving
smack_task(current)->smk_preserve and smk_preserve_inode intact across the
domain transition.

If a task running with label L1 opens a whitelisted file W with O_RDONLY
(passing the MAY_READ check for L1) and then transitions to label L2 via
do_setattr() (including unprivileged transitions permitted by
/smack/relabel-self), a subsequent file creation in
smack_inode_init_security() will only check MAY_WRITE for L2 without
verifying that L2 had MAY_READ on W. Similarly, a subsequent O_WRONLY |
O_TRUNC open in smack_file_open() will compare tgt == smk_of_task(tsp) against
the new label L2 and relabel an L2-owned file with the leftover preserve
record from L1.

Should do_setattr() also clear smk_preserve and smk_preserve_inode (and
should smack_bprm_committing_creds() clear smk_preserve_inode and smk_copy_up
as well)?

[ ... ]

> @@ -1075,6 +1128,33 @@ 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, inherit it so
> +	 * the new file keeps the source's (low-integrity) label across the
> +	 * copy.  Two cases take precedence, and the record is one-shot so
> +	 * it is dropped either way:
> +	 *
> +	 *  - a copy-up, which must keep the lower label untouched;
> +	 *  - a transmutation, otherwise the record would leak into a later
> +	 *    create in a non-transmutable directory.
> +	 *
> +	 * The record is only inherited when the task may also write the
> +	 * label.  A task that can read skp but not write it must not be
> +	 * able to mint a file carrying skp -- that would be a write into a
> +	 * domain the policy keeps it out of.
> +	 */
> +	if (!copy_up) {
> +		struct smack_known *skp = smack_task(current)->smk_preserve;
> +
> +		if (skp != NULL && !transmutes &&
> +		    smk_tskacc(tsp, skp, MAY_WRITE, NULL) == 0)
> +			issp->smk_inode = skp;
> +		smack_task(current)->smk_preserve = NULL;
> +		smack_task(current)->smk_preserve_inode = NULL;
> +	}
> +#endif

[Severity: Medium]
Does checking smk_tskacc(tsp, skp, MAY_WRITE, NULL) == 0 here in
smack_inode_init_security() (and in the overwrite check in smack_file_open())
break label preservation when CONFIG_SECURITY_SMACK_BRINGUP is enabled?

In security/smack/smack_access.c, smk_access() and smk_tskacc() return
positive values (SMACK_BRINGUP_ALLOW, SMACK_UNCONFINED_SUBJECT, or
SMACK_UNCONFINED_OBJECT) when access is granted via a bringup rule or an
unconfined label, which other hooks normalize to 0 via the smk_bu_* helpers.

When MAY_WRITE is permitted via bringup or unconfined mode, VFS permission
checks succeed, but smk_tskacc(..., MAY_WRITE, NULL) == 0 evaluates to false
and drops smk_preserve without applying the label. Should these checks test
>= 0 or pass the result through a bringup helper?

[ ... ]

> @@ -2077,10 +2157,160 @@ 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
> +	struct smack_task *stp = smack_task(current);
> +

[ ... ]

> +	if (!(file->f_mode & (FMODE_CREATED | FMODE_EXEC)) &&
> +	    !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)) {
> +			stp->smk_preserve = skp;
> +			stp->smk_preserve_inode = inode;
> +		}
> +	}

[Severity: High]
As reported by Sashiko on v3 [1], does testing file->f_mode & FMODE_EXEC in
smack_file_open() actually filter out exec opens?

In fs/exec.c, do_open_execat() sets open_flag = O_LARGEFILE | O_RDONLY |
__FMODE_EXEC. However, init_file() initializes f->f_mode using
OPEN_FMODE(flags), which masks with O_ACCMODE (0x3) and strips FMODE_EXEC:

include/linux/fs.h:
    #define OPEN_FMODE(flag) ((__force fmode_t)(((flag + 1) & O_ACCMODE) | \
                                                (flag & __FMODE_NONOTIFY)))

Because file->f_mode & FMODE_EXEC is always 0, opening a whitelisted regular
file via do_open_execat() or open_exec() still sets stp->smk_preserve and
stp->smk_preserve_inode. If a task has an open whitelisted file and invokes
execveat() with AT_EXECVE_CHECK or a failing execve() on a whitelisted file,
do_open_execat() overwrites stp->smk_preserve_inode and then closes the
temporary exec file, causing smack_file_free_security() to clear both
stp->smk_preserve and stp->smk_preserve_inode while the original source file
remains open.

Should this check test file->f_flags & __FMODE_EXEC instead (similar to
AppArmor, Landlock, and TOMOYO)?

> +
> +	/*
> +	 * An overwrite skips inode_init_security(), so hand the record to
> +	 * the file and apply it on release.  O_TRUNC is still set here:
> +	 * do_dentry_open() prunes it only after security_file_open(), and
> +	 * handle_truncate() runs later still.  That is why the
> +	 * file_truncate hook cannot serve instead -- by then the flag is
> +	 * gone and ftruncate(2) is indistinguishable.
> +	 *
> +	 * Only a file carrying the task's own label, or a preservable one,
> +	 * is relabelled; a trusted file keeps its label.
> +	 */
> +	if (stp->smk_preserve != NULL &&
> +	    !(file->f_mode & FMODE_CREATED) &&
> +	    (file->f_flags & O_ACCMODE) == O_WRONLY &&
> +	    (file->f_flags & O_TRUNC) &&
> +	    S_ISREG(inode->i_mode)) {
> +		struct smack_known *tgt = smk_of_inode(inode);
> +
> +		if ((tgt == smk_of_task(tsp) || smk_preserve_allowed(tgt)) &&
> +		    smk_tskacc(tsp, tgt, MAY_WRITE, NULL) == 0)
> +			*smack_file_overwrite(file) = stp->smk_preserve;
> +		stp->smk_preserve = NULL;
> +		stp->smk_preserve_inode = NULL;
> +	}
> +#endif
>  
>  	return rc;
>  }

[Severity: High]
As reported by Sashiko on v3 [2], doesn't the overwrite path in
smack_file_open() check MAY_WRITE against the target label tgt instead of the
preserved label stp->smk_preserve, and miss overwrites opened with O_RDWR |
O_TRUNC?

In smack_inode_init_security(), inheriting skp = stp->smk_preserve requires
MAY_WRITE on skp so a task with only MAY_READ on skp cannot mint a file in
that domain. Here in smack_file_open(), smk_tskacc(tsp, tgt, MAY_WRITE, NULL)
checks MAY_WRITE on the target file's existing label tgt (which is trivially
allowed when tgt == smk_of_task(tsp)) and never checks MAY_WRITE on
stp->smk_preserve. A task with MAY_READ but not MAY_WRITE on a whitelisted
label skp can open a file carrying skp with O_RDONLY, then open its own file
with O_WRONLY | O_TRUNC, write arbitrary content, and close it to relabel the
file to skp via __vfs_setxattr_noperm().

In addition, checking (file->f_flags & O_ACCMODE) == O_WRONLY misses existing
files overwritten with O_RDWR | O_TRUNC, leaving the overwritten target file
with its old label and leaving stp->smk_preserve armed for a later unrelated
create.

>  
> +#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
> +/**
> + * smk_relabel_on_release - give a file the label of what was copied into it
> + * @file: the file being released
> + * @skp: label to apply
> + *
> + * Write the xattr before adopting the label in memory, so a failed write
> + * leaves the inode matching the disk.
> + *
> + * __vfs_setxattr_noperm() skips security_inode_setxattr() on purpose:
> + * vfs_setxattr() would reach smack_inode_setxattr() and demand
> + * CAP_MAC_ADMIN, while the label was already vetted in smack_file_open()
> + * against a narrower rule.
> + */
> +static void smk_relabel_on_release(struct file *file, struct smack_known *skp)
> +{
> +	struct dentry *dentry = file->f_path.dentry;
> +	struct inode *inode = d_inode(dentry);
> +	struct inode_smack *isp = smack_inode(inode);
> +	int rc;
> +
> +	if (isp->smk_inode == skp)
> +		return;
> +
> +	/*
> +	 * __vfs_setxattr_noperm() requires the inode lock: filesystems such
> +	 * as UBIFS assert inode_is_locked() in their xattr_set handler.
> +	 * __fput() runs from task_work, so taking the lock here is safe.
> +	 */
> +	inode_lock(inode);
> +	rc = __vfs_setxattr_noperm(file_mnt_idmap(file), dentry,
> +				   XATTR_NAME_SMACK,
> +				   skp->smk_known, strlen(skp->smk_known), 0);
> +	inode_unlock(inode);
> +
> +	if (rc == 0)
> +		isp->smk_inode = skp;
> +}
> +#endif

[Severity: Medium]
In smk_relabel_on_release(), is it safe to assign isp->smk_inode = skp after
calling inode_unlock(inode)?

Inside __vfs_setxattr_noperm(), security_inode_post_setxattr() ->
smack_inode_post_setxattr() (or security_inode_setsecurity() ->
smack_inode_setsecurity()) already updates isp->smk_inode while holding
inode_lock(inode). Writing isp->smk_inode = skp again after inode_unlock(inode)
races with any concurrent setxattr() or removexattr() that acquires the inode
lock right after it is dropped, potentially leaving isp->smk_inode out of
sync with the on-disk xattr.

> +
> +#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
> +/**
> + * smack_file_free_security - finish a copy, drop stale records
> + * @file: the file being freed
> + *
> + * Two records can be outstanding:
> + *
> + *  - the overwrite label, kept in the file blob.  Its contents are
> + *    complete and the inode lock is gone, so this is the first safe
> + *    point to write it back;
> + *  - a source record, kept in the task blob, whose source is released
> + *    before any create consumed it.  It is stale; dropping it keeps an
> + *    unrelated later create from inheriting a label it never read.
> + *
> + * The task blob lookup uses current.  fput() normally defers to
> + * task_work on the closing task, which is correct; should it fall back
> + * to ____fput() from a workqueue the lookup misses and the stale record
> + * survives until the next open of a whitelisted file replaces it.  The
> + * overwrite record is unaffected because it lives in the file blob.
> + */
> +static void smack_file_free_security(struct file *file)
> +{
> +	struct inode *inode = file_inode(file);
> +	struct smack_task *stp = smack_task(current);
> +	struct smack_known *skp = *smack_file_overwrite(file);
> +
> +	if (skp != NULL) {
> +		*smack_file_overwrite(file) = NULL;
> +		smk_relabel_on_release(file, skp);
> +	}
> +
> +	if (stp->smk_preserve_inode == inode) {
> +		stp->smk_preserve = NULL;
> +		stp->smk_preserve_inode = NULL;
> +	}
> +}
> +#endif

[Severity: Critical]
Can running smk_relabel_on_release() from smack_file_free_security() (via the
file_free_security hook) trigger a NULL pointer dereference or use-after-free?

First, in fs/file_table.c:__fput(), the file's write access, dentry
reference, and mount reference are all dropped before file_free() calls
security_file_free() -> smack_file_free_security():

fs/file_table.c:__fput() {
    ...
    put_file_access(file);
    dput(dentry);
    if (unlikely(mode & FMODE_NEED_UNMOUNT))
        dissolve_on_fput(mnt);
    else
        mntput(mnt);
out:
    file_free(file);
}

If the overwritten file was unlinked or renamed over while open, dput(dentry)
drops the last reference to the dentry, clears dentry->d_inode to NULL, calls
iput(), and frees the dentry before smack_file_free_security() runs.
smk_relabel_on_release() then accesses the freed dentry/mnt and dereferences
d_inode(dentry) == NULL. Even when the dentry stays cached, calling
__vfs_setxattr_noperm() after put_file_access() and mntput() performs a
filesystem xattr write after mnt_want_write() protection has already been
released.

Second, in fs/open.c:do_dentry_open(), security_file_open(f) runs before
fsnotify_open_perm_and_set_mode(), break_lease(), and f->f_op->open(). If
smack_file_open() sets *smack_file_overwrite(file) = stp->smk_preserve and a
later step in do_dentry_open() fails, the cleanup_all / cleanup_file path
clears f->__f_path.dentry = NULL and f->f_inode = NULL:

fs/open.c:do_dentry_open() {
    ...
cleanup_file:
    path_put(&f->f_path);
    f->__f_path.mnt = NULL;
    f->__f_path.dentry = NULL;
    f->f_inode = NULL;
    return error;
}

When the caller then frees f via fput_close() -> file_free() ->
security_file_free(), smk_relabel_on_release() reads dentry =
file->f_path.dentry (NULL) and crashes at d_inode(dentry). And if do_open()
fails after vfs_open() (for example in handle_truncate()), fput_close() will
relabel the untouched file on disk even though the open failed.

Third, in fs/file_table.c:init_file(), security_file_alloc(f) runs before
f->f_inode = NULL is initialized; if another stacked LSM fails in
file_alloc_security, security_file_alloc() calls security_file_free(f) ->
smack_file_free_security(f), which reads uninitialized slab memory via
file_inode(file).

[Severity: Medium]
Also in smack_file_free_security(), doesn't clearing stp->smk_preserve when
stp->smk_preserve_inode == inode break copy programs that close the source
file before creating the destination file?

When a task calls close(src_fd), fput_close() schedules ____fput via
task_work_add(..., TWA_RESUME), which runs before the close() syscall returns
to userspace. A process that opens a whitelisted file O_RDONLY, reads its
contents into a buffer, closes src_fd, and then opens the destination file
with O_CREAT | O_WRONLY will have stp->smk_preserve cleared on close(src_fd)
before the destination file is created.

Similarly, if a task opens the same whitelisted inode twice (for example, one
fd to inspect headers and a second fd to copy) and closes the first fd while
the second remains open, freeing the first struct file matches
stp->smk_preserve_inode == inode and clears stp->smk_preserve. Conversely, if
the struct file is duplicated across fork() or dup() and another task drops
the final file reference, stp->smk_preserve_inode is left dangling on the
original task.

[ ... ]

> @@ -5014,6 +5244,15 @@ static int smack_inode_copy_up(struct dentry *dentry, struct cred **new)
>  	isp = smack_inode(d_inode(dentry));
>  	skp = isp->smk_inode;
>  	tsp->smk_task = skp;
> +#ifdef CONFIG_SECURITY_SMACK_COPYWHITELIST
> +	/*
> +	 * The copy-up creates upper inodes through the normal create
> +	 * path.  Tell smack_inode_init_security() to leave those alone:
> +	 * they must keep the lower label, and a pending copy record
> +	 * would otherwise be consumed by an internal overlayfs create.
> +	 */
> +	smack_task(current)->smk_copy_up = true;
> +#endif
>  	*new = new_creds;
>  	return 0;
>  }

[Severity: High]
As reported by Sashiko on v3 [3], can tracking OverlayFS copy-up with a
one-shot smk_copy_up boolean in smack_task(current) still corrupt task state
and inode labels?

1. In ovl_copy_up_workdir() and ovl_copy_up_tmpfile(),
ovl_prepare_copy_up_creds() calls security_inode_copy_up() ->
smack_inode_copy_up(), setting smack_task(current)->smk_copy_up = true. If a
subsequent step fails before security_inode_init_security() is reached (for
example in another LSM's inode_copy_up hook, ovl_start_creating_temp(),
may_create_dentry(), security_inode_create(), or inode_permission() in
vfs_tmpfile()), smack_inode_init_security() never runs to consume
smk_copy_up. The temporary copy-up creds are reverted, but smk_copy_up
remains true on current, causing the task's next unrelated file or directory
creation to skip both transmutation and smk_preserve handling.

2. When copying up a directory with OverlayFS index=on in
ovl_copy_up_workdir(), ovl_create_temp() creates the upper directory (which
consumes smk_copy_up = false in smack_copy_up_begin()) and then
ovl_create_index() calls ovl_create_temp() a second time to create the index
entry with smk_copy_up == false, consuming current's pending smk_preserve on
the internal index inode. Internal workdir creations that do not call
security_inode_copy_up() (such as ovl_whiteout() and ovl_clear_empty()) also
enter smack_inode_init_security() with smk_copy_up == false.

3. In smack_file_open(), the !(file->f_mode & (FMODE_CREATED | FMODE_EXEC))
check does not exclude FMODE_BACKING or internal OverlayFS O_RDONLY opens.
During regular file copy-up, ovl_copy_up_file() and ovl_sync_file() open and
close the lower/upper files with O_RDONLY, which runs smack_file_open() and
smack_file_free_security() and clobbers or clears current's pending
smk_preserve record. Furthermore, creating a file on OverlayFS with O_RDONLY
| O_CREAT consumes smk_preserve in smack_inode_init_security(), and then
ovl_open_realfile() opens the upper backing file via backing_file_open()
(which sets FMODE_BACKING without FMODE_CREATED), re-arming smk_preserve in
smack_file_open().

[ ... ]

> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index a72bc7fabea99..a726957c434d3 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c

[ ... ]

> +static ssize_t smk_write_preserve_wl(struct file *file, const char __user *buf,
> +				     size_t count, loff_t *ppos)
> +{
> +	struct smack_known *skp;
> +	char *data, *label;
> +	bool on = true;
> +	int rc;
> +
> +	if (!smack_privileged(CAP_MAC_ADMIN))
> +		return -EPERM;
> +
> +	if (*ppos != 0)
> +		return -EINVAL;
> +
> +	if (count == 0 || count > SMK_LONGLABEL)
> +		return -EINVAL;
> +
> +	data = memdup_user_nul(buf, count);
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	label = strim(data);
> +	if (!label[0]) {
> +		kfree(data);
> +		return -EINVAL;
> +	}
> +
> +	/* A leading '-' removes the label (same convention as revoke-subject). */
> +	if (label[0] == '-') {
> +		on = false;
> +		label = strim(label + 1);
> +		if (!label[0]) {
> +			kfree(data);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/*
> +	 * Import the label when adding.  Boot-time policy may name a label
> +	 * that no inode, rule or mount has referenced yet, in which case it
> +	 * is not in smack_known_hash.  Removal only needs the existing entry.
> +	 */
> +	if (on) {
> +		skp = smk_import_entry(label, 0);
> +		if (IS_ERR(skp)) {
> +			rc = PTR_ERR(skp);
> +			kfree(data);
> +			return rc;
> +		}
> +	} else {
> +		skp = smk_find_entry(label);
> +		if (!skp) {
> +			kfree(data);
> +			return -ENOENT;
> +		}
> +	}
> +
> +	rc = smk_preserve_set(skp, on);
> +	kfree(data);
> +	return rc ? rc : count;
> +}

[Severity: Medium]
In smk_write_preserve_wl(), does rejecting count > SMK_LONGLABEL before
stripping the '-' prefix prevent removing a maximum-length (255-character)
Smack label when written with a trailing newline?

Adding a 255-character label with a newline uses 256 bytes (SMK_LONGLABEL),
which succeeds, but removing that same label with a leading '-' and trailing
newline requires 257 bytes (SMK_LONGLABEL + 1) and fails with -EINVAL.

Also, when on is true, smk_import_entry(label, 0) calls smk_parse_label_len(),
which stops at the first whitespace or invalid character (such as '/', '"',
'\\', or '\'') and returns the prefix length rather than failing. Without
checking that the parsed label length matches strlen(label) (as do_setattr()
does in security/smack/smack_lsm.c), won't writing a string containing an
invalid character or multiple space/newline-separated labels silently
import and whitelist only the truncated prefix while returning count?

[1] https://lore.kernel.org/r/684C81A1C8289709+20261028161521.925644-1-tangpengke@yhkylin.cn
[2] https://lore.kernel.org/r/684C81A1C8289709+20261028161521.925644-1-tangpengke@yhkylin.cn
[3] https://lore.kernel.org/r/684C81A1C8289709+20261028161521.925644-1-tangpengke@yhkylin.cn

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