[PATCH v2 00/10] Improve the copy of task comm

[PATCH v2 00/10] Improve the copy of task comm

[Yafang Shao]

Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the
length of task comm. Changes in the task comm could result in a destination
string that is overflow. Therefore, we should explicitly ensure the destination
string is always NUL-terminated, regardless of the task comm. This approach
will facilitate future extensions to the task comm.

As suggested by Linus [0], we can identify all relevant code with the
following git grep command:

  git grep 'memcpy.*->comm\>'
  git grep 'kstrdup.*->comm\>'
  git grep 'strncpy.*->comm\>'
  git grep 'strcpy.*->comm\>'

PATCH #2~#4:  memcpy
PATCH #5:     kstrdup
PATCH #6~#8:  strncpy
PATCH #9~#10: strcpy

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0]

Changes:
v1->v2:
- Add comment for dropping task_lock() in __get_task_comm() (Alexei)
- Drop changes in trace event (Steven)
- Fix comment on task comm (Matus)
  
v1: https://lore.kernel.org/all/20240602023754.25443-1-laoar.shao@gmail.com/

Yafang Shao (10):
  fs/exec: Drop task_lock() inside __get_task_comm()
  auditsc: Replace memcpy() with __get_task_comm()
  security: Replace memcpy() with __get_task_comm()
  bpftool: Ensure task comm is always NUL-terminated
  mm/util: Fix possible race condition in kstrdup()
  mm/kmemleak: Replace strncpy() with __get_task_comm()
  tsacct: Replace strncpy() with __get_task_comm()
  tracing: Replace strncpy() with __get_task_comm()
  net: Replace strcpy() with __get_task_comm()
  drm: Replace strcpy() with __get_task_comm()

 drivers/gpu/drm/drm_framebuffer.c     |  2 +-
 drivers/gpu/drm/i915/i915_gpu_error.c |  2 +-
 fs/exec.c                             | 10 ++++++++--
 include/linux/sched.h                 |  4 ++--
 kernel/auditsc.c                      |  6 +++---
 kernel/trace/trace.c                  |  2 +-
 kernel/trace/trace_events_hist.c      |  2 +-
 kernel/tsacct.c                       |  2 +-
 mm/kmemleak.c                         |  8 +-------
 mm/util.c                             |  4 +++-
 net/ipv6/ndisc.c                      |  2 +-
 security/lsm_audit.c                  |  4 ++--
 security/selinux/selinuxfs.c          |  2 +-
 tools/bpf/bpftool/pids.c              |  2 ++
 14 files changed, 28 insertions(+), 24 deletions(-)

-- 
2.39.1

[PATCH v2 01/10] fs/exec: Drop task_lock() inside __get_task_comm()

[Yafang Shao]

Quoted from Linus [0]:

  Since user space can randomly change their names anyway, using locking
  was always wrong for readers (for writers it probably does make sense
  to have some lock - although practically speaking nobody cares there
  either, but at least for a writer some kind of race could have
  long-term mixed results

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Matus Jokay <matus.jokay@stuba.sk>
---
 fs/exec.c             | 10 ++++++++--
 include/linux/sched.h |  4 ++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 40073142288f..fa6b61c79df8 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1238,12 +1238,18 @@ static int unshare_sighand(struct task_struct *me)
 	return 0;
 }
 
+/*
+ * User space can randomly change their names anyway, so locking for readers
+ * doesn't make sense. For writers, locking is probably necessary, as a race
+ * condition could lead to long-term mixed results.
+ * The strscpy_pad() in __set_task_comm() can ensure that the task comm is
+ * always NUL-terminated. Therefore the race condition between reader and writer
+ * is not an issue.
+ */
 char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
 {
-	task_lock(tsk);
 	/* Always NUL terminated and zero-padded */
 	strscpy_pad(buf, tsk->comm, buf_size);
-	task_unlock(tsk);
 	return buf;
 }
 EXPORT_SYMBOL_GPL(__get_task_comm);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 61591ac6eab6..95888d1da49e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1086,9 +1086,9 @@ struct task_struct {
 	/*
 	 * executable name, excluding path.
 	 *
-	 * - normally initialized setup_new_exec()
+	 * - normally initialized begin_new_exec()
 	 * - access it with [gs]et_task_comm()
-	 * - lock it with task_lock()
+	 * - lock it with task_lock() for writing
 	 */
 	char				comm[TASK_COMM_LEN];
 
-- 
2.39.1

[PATCH v2 02/10] auditsc: Replace memcpy() with __get_task_comm()

[Yafang Shao]

Using __get_task_comm() to read the task comm ensures that the name is
always NUL-terminated, regardless of the source string. This approach also
facilitates future extensions to the task comm.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Acked-by: Paul Moore <paul@paul-moore.com>
Cc: Eric Paris <eparis@redhat.com>
---
 kernel/auditsc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 6f0d6fb6523f..0459a141dc86 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2730,7 +2730,7 @@ void __audit_ptrace(struct task_struct *t)
 	context->target_uid = task_uid(t);
 	context->target_sessionid = audit_get_sessionid(t);
 	security_task_getsecid_obj(t, &context->target_sid);
-	memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
+	__get_task_comm(context->target_comm, TASK_COMM_LEN, t);
 }
 
 /**
@@ -2757,7 +2757,7 @@ int audit_signal_info_syscall(struct task_struct *t)
 		ctx->target_uid = t_uid;
 		ctx->target_sessionid = audit_get_sessionid(t);
 		security_task_getsecid_obj(t, &ctx->target_sid);
-		memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
+		__get_task_comm(ctx->target_comm, TASK_COMM_LEN, t);
 		return 0;
 	}
 
@@ -2778,7 +2778,7 @@ int audit_signal_info_syscall(struct task_struct *t)
 	axp->target_uid[axp->pid_count] = t_uid;
 	axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
 	security_task_getsecid_obj(t, &axp->target_sid[axp->pid_count]);
-	memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
+	__get_task_comm(axp->target_comm[axp->pid_count], TASK_COMM_LEN, t);
 	axp->pid_count++;
 
 	return 0;
-- 
2.39.1

[PATCH v2 03/10] security: Replace memcpy() with __get_task_comm()

[Yafang Shao]

Quoted from Linus [0]:

  selinux never wanted a lock, and never wanted any kind of *consistent*
  result, it just wanted a *stable* result.

Using __get_task_comm() to read the task comm ensures that the name is
always NUL-terminated, regardless of the source string. This approach also
facilitates future extensions to the task comm.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
LINK: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com/ [0]
Acked-by: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Stephen Smalley <stephen.smalley.work@gmail.com>
Cc: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/lsm_audit.c         | 4 ++--
 security/selinux/selinuxfs.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 849e832719e2..a922e4339dd5 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -207,7 +207,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 	BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
 
 	audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
-	audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
+	audit_log_untrustedstring(ab, __get_task_comm(comm, sizeof(comm), current));
 
 	switch (a->type) {
 	case LSM_AUDIT_DATA_NONE:
@@ -302,7 +302,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 				char comm[sizeof(tsk->comm)];
 				audit_log_format(ab, " opid=%d ocomm=", pid);
 				audit_log_untrustedstring(ab,
-				    memcpy(comm, tsk->comm, sizeof(comm)));
+				    __get_task_comm(comm, sizeof(comm), tsk));
 			}
 		}
 		break;
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index e172f182b65c..a8a2ec742576 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -708,7 +708,7 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf,
 	if (new_value) {
 		char comm[sizeof(current->comm)];
 
-		memcpy(comm, current->comm, sizeof(comm));
+		__get_task_comm(comm, sizeof(comm), current);
 		pr_err("SELinux: %s (%d) set checkreqprot to 1. This is no longer supported.\n",
 		       comm, current->pid);
 	}
-- 
2.39.1

[PATCH v2 04/10] bpftool: Ensure task comm is always NUL-terminated

[Yafang Shao]

Let's explicitly ensure the destination string is NUL-terminated. This way,
it won't be affected by changes to the source string.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Reviewed-by: Quentin Monnet <qmo@kernel.org>
---
 tools/bpf/bpftool/pids.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
index 9b898571b49e..23f488cf1740 100644
--- a/tools/bpf/bpftool/pids.c
+++ b/tools/bpf/bpftool/pids.c
@@ -54,6 +54,7 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
 		ref = &refs->refs[refs->ref_cnt];
 		ref->pid = e->pid;
 		memcpy(ref->comm, e->comm, sizeof(ref->comm));
+		ref->comm[sizeof(ref->comm) - 1] = '\0';
 		refs->ref_cnt++;
 
 		return;
@@ -77,6 +78,7 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
 	ref = &refs->refs[0];
 	ref->pid = e->pid;
 	memcpy(ref->comm, e->comm, sizeof(ref->comm));
+	ref->comm[sizeof(ref->comm) - 1] = '\0';
 	refs->ref_cnt = 1;
 	refs->has_bpf_cookie = e->has_bpf_cookie;
 	refs->bpf_cookie = e->bpf_cookie;
-- 
2.39.1

[PATCH v2 05/10] mm/util: Fix possible race condition in kstrdup()

[Yafang Shao]

In kstrdup(), it is critical to ensure that the dest string is always
NUL-terminated. However, potential race condidtion can occur between a
writer and a reader.

Consider the following scenario involving task->comm:

    reader                    writer

  len = strlen(s) + 1;
                             strlcpy(tsk->comm, buf, sizeof(tsk->comm));
  memcpy(buf, s, len);

In this case, there is a race condition between the reader and the
writer. The reader calculate the length of the string `s` based on the
old value of task->comm. However, during the memcpy(), the string `s`
might be updated by the writer to a new value of task->comm.

If the new task->comm is larger than the old one, the `buf` might not be
NUL-terminated. This can lead to undefined behavior and potential
security vulnerabilities.

Let's fix it by explicitly adding a NUL-terminator.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 mm/util.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/util.c b/mm/util.c
index c9e519e6811f..3b383f790208 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -60,8 +60,10 @@ char *kstrdup(const char *s, gfp_t gfp)
 
 	len = strlen(s) + 1;
 	buf = kmalloc_track_caller(len, gfp);
-	if (buf)
+	if (buf) {
 		memcpy(buf, s, len);
+		buf[len - 1] = '\0';
+	}
 	return buf;
 }
 EXPORT_SYMBOL(kstrdup);
-- 
2.39.1

[PATCH v2 06/10] mm/kmemleak: Replace strncpy() with __get_task_comm()

[Yafang Shao]

Using __get_task_comm() to read the task comm ensures that the name is
always NUL-terminated, regardless of the source string. This approach also
facilitates future extensions to the task comm.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 mm/kmemleak.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index d5b6fba44fc9..ef29aaab88a0 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -663,13 +663,7 @@ static struct kmemleak_object *__alloc_object(gfp_t gfp)
 		strncpy(object->comm, "softirq", sizeof(object->comm));
 	} else {
 		object->pid = current->pid;
-		/*
-		 * There is a small chance of a race with set_task_comm(),
-		 * however using get_task_comm() here may cause locking
-		 * dependency issues with current->alloc_lock. In the worst
-		 * case, the command line is not correct.
-		 */
-		strncpy(object->comm, current->comm, sizeof(object->comm));
+		__get_task_comm(object->comm, sizeof(object->comm), current);
 	}
 
 	/* kernel backtrace */
-- 
2.39.1

[PATCH v2 07/10] tsacct: Replace strncpy() with __get_task_comm()

[Yafang Shao]

Using __get_task_comm() to read the task comm ensures that the name is
always NUL-terminated, regardless of the source string. This approach also
facilitates future extensions to the task comm.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 kernel/tsacct.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/tsacct.c b/kernel/tsacct.c
index 4252f0645b9e..6b094d5d9135 100644
--- a/kernel/tsacct.c
+++ b/kernel/tsacct.c
@@ -76,7 +76,7 @@ void bacct_add_tsk(struct user_namespace *user_ns,
 	stats->ac_minflt = tsk->min_flt;
 	stats->ac_majflt = tsk->maj_flt;
 
-	strncpy(stats->ac_comm, tsk->comm, sizeof(stats->ac_comm));
+	__get_task_comm(stats->ac_comm, sizeof(stats->ac_comm), tsk);
 }
 
 
-- 
2.39.1

[PATCH v2 08/10] tracing: Replace strncpy() with __get_task_comm()

[Yafang Shao]

Using __get_task_comm() to read the task comm ensures that the name is
always NUL-terminated, regardless of the source string. This approach also
facilitates future extensions to the task comm.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 kernel/trace/trace.c             | 2 +-
 kernel/trace/trace_events_hist.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 578a49ff5c32..ce94a86154a2 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1907,7 +1907,7 @@ __update_max_tr(struct trace_array *tr, struct task_struct *tsk, int cpu)
 	max_data->critical_start = data->critical_start;
 	max_data->critical_end = data->critical_end;
 
-	strncpy(max_data->comm, tsk->comm, TASK_COMM_LEN);
+	__get_task_comm(max_data->comm, TASK_COMM_LEN, tsk);
 	max_data->pid = tsk->pid;
 	/*
 	 * If tsk == current, then use current_uid(), as that does not use
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 6ece1308d36a..721d4758a79f 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1599,7 +1599,7 @@ static inline void save_comm(char *comm, struct task_struct *task)
 		return;
 	}
 
-	strncpy(comm, task->comm, TASK_COMM_LEN);
+	__get_task_comm(comm, TASK_COMM_LEN, task);
 }
 
 static void hist_elt_data_free(struct hist_elt_data *elt_data)
-- 
2.39.1

[PATCH v2 09/10] net: Replace strcpy() with __get_task_comm()

[Yafang Shao]

To prevent errors from occurring when the src string is longer than the dst
string in strcpy(), we should use __get_task_comm() instead. This approach
also facilitates future extensions to the task comm.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: David Ahern <dsahern@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
---
 net/ipv6/ndisc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index d914b23256ce..37fa3b69df45 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1942,7 +1942,7 @@ static void ndisc_warn_deprecated_sysctl(struct ctl_table *ctl,
 	static char warncomm[TASK_COMM_LEN];
 	static int warned;
 	if (strcmp(warncomm, current->comm) && warned < 5) {
-		strcpy(warncomm, current->comm);
+		__get_task_comm(warncomm, TASK_COMM_LEN, current);
 		pr_warn("process `%s' is using deprecated sysctl (%s) net.ipv6.neigh.%s.%s - use net.ipv6.neigh.%s.%s_ms instead\n",
 			warncomm, func,
 			dev_name, ctl->procname,
-- 
2.39.1

[PATCH v2 10/10] drm: Replace strcpy() with __get_task_comm()

[Yafang Shao]

To prevent erros from occurring when the src string is longer than the
dst string in strcpy(), we should use __get_task_comm() instead. This
approach also facilitates future extensions to the task comm.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/drm_framebuffer.c     | 2 +-
 drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 888aadb6a4ac..25262b07ffaf 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -868,7 +868,7 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 	INIT_LIST_HEAD(&fb->filp_head);
 
 	fb->funcs = funcs;
-	strcpy(fb->comm, current->comm);
+	__get_task_comm(fb->comm, sizeof(fb->comm), current);
 
 	ret = __drm_mode_object_add(dev, &fb->base, DRM_MODE_OBJECT_FB,
 				    false, drm_framebuffer_free);
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 625b3c024540..b2c16a53bd24 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1411,7 +1411,7 @@ static bool record_context(struct i915_gem_context_coredump *e,
 	rcu_read_lock();
 	task = pid_task(ctx->pid, PIDTYPE_PID);
 	if (task) {
-		strcpy(e->comm, task->comm);
+		__get_task_comm(e->comm, sizeof(e->comm), task);
 		e->pid = task->pid;
 	}
 	rcu_read_unlock();
-- 
2.39.1

Re: [PATCH v2 06/10] mm/kmemleak: Replace strncpy() with __get_task_comm()

[Catalin Marinas]

On Thu, Jun 13, 2024 at 10:30:40AM +0800, Yafang Shao wrote:
> Using __get_task_comm() to read the task comm ensures that the name is
> always NUL-terminated, regardless of the source string. This approach also
> facilitates future extensions to the task comm.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  mm/kmemleak.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index d5b6fba44fc9..ef29aaab88a0 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -663,13 +663,7 @@ static struct kmemleak_object *__alloc_object(gfp_t gfp)
>  		strncpy(object->comm, "softirq", sizeof(object->comm));
>  	} else {
>  		object->pid = current->pid;
> -		/*
> -		 * There is a small chance of a race with set_task_comm(),
> -		 * however using get_task_comm() here may cause locking
> -		 * dependency issues with current->alloc_lock. In the worst
> -		 * case, the command line is not correct.
> -		 */
> -		strncpy(object->comm, current->comm, sizeof(object->comm));
> +		__get_task_comm(object->comm, sizeof(object->comm), current);
>  	}

You deleted the comment stating why it does not use get_task_comm()
without explaining why it would be safe now. I don't recall the details
but most likely lockdep warned of some potential deadlocks with this
function being called with the task_lock held.

So, you either show why this is safe or just use strscpy() directly here
(not sure we'd need strscpy_pad(); I think strscpy() would do, we just
need the NUL-termination).

-- 
Catalin

Re: [PATCH v2 06/10] mm/kmemleak: Replace strncpy() with __get_task_comm()

[Yafang Shao]

On Thu, Jun 13, 2024 at 4:37 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Thu, Jun 13, 2024 at 10:30:40AM +0800, Yafang Shao wrote:
> > Using __get_task_comm() to read the task comm ensures that the name is
> > always NUL-terminated, regardless of the source string. This approach also
> > facilitates future extensions to the task comm.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >  mm/kmemleak.c | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> >
> > diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> > index d5b6fba44fc9..ef29aaab88a0 100644
> > --- a/mm/kmemleak.c
> > +++ b/mm/kmemleak.c
> > @@ -663,13 +663,7 @@ static struct kmemleak_object *__alloc_object(gfp_t gfp)
> >               strncpy(object->comm, "softirq", sizeof(object->comm));
> >       } else {
> >               object->pid = current->pid;
> > -             /*
> > -              * There is a small chance of a race with set_task_comm(),
> > -              * however using get_task_comm() here may cause locking
> > -              * dependency issues with current->alloc_lock. In the worst
> > -              * case, the command line is not correct.
> > -              */
> > -             strncpy(object->comm, current->comm, sizeof(object->comm));
> > +             __get_task_comm(object->comm, sizeof(object->comm), current);
> >       }
>
> You deleted the comment stating why it does not use get_task_comm()
> without explaining why it would be safe now. I don't recall the details
> but most likely lockdep warned of some potential deadlocks with this
> function being called with the task_lock held.
>
> So, you either show why this is safe or just use strscpy() directly here
> (not sure we'd need strscpy_pad(); I think strscpy() would do, we just
> need the NUL-termination).

The task_lock was dropped in patch #1 [0]. My apologies for not
including you in the CC for that change. After this modification, it
is now safe to use __get_task_comm().

[0] https://lore.kernel.org/all/20240613023044.45873-2-laoar.shao@gmail.com/

-- 
Regards
Yafang

Re: [PATCH v2 05/10] mm/util: Fix possible race condition in kstrdup()

[Andrew Morton]

On Thu, 13 Jun 2024 10:30:39 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:

> In kstrdup(), it is critical to ensure that the dest string is always
> NUL-terminated. However, potential race condidtion can occur between a
> writer and a reader.
> 
> Consider the following scenario involving task->comm:
> 
>     reader                    writer
> 
>   len = strlen(s) + 1;
>                              strlcpy(tsk->comm, buf, sizeof(tsk->comm));
>   memcpy(buf, s, len);
> 
> In this case, there is a race condition between the reader and the
> writer. The reader calculate the length of the string `s` based on the
> old value of task->comm. However, during the memcpy(), the string `s`
> might be updated by the writer to a new value of task->comm.
> 
> If the new task->comm is larger than the old one, the `buf` might not be
> NUL-terminated. This can lead to undefined behavior and potential
> security vulnerabilities.
> 
> Let's fix it by explicitly adding a NUL-terminator.

The concept sounds a little strange.  If some code takes a copy of a
string while some other code is altering it, yes, the result will be a
mess.  This is why get_task_comm() exists, and why it uses locking.

I get that "your copy is a mess" is less serious than "your string
isn't null-terminated" but still.  Whichever outcome we get, the
calling code is buggy and should be fixed.

Are there any other problematic scenarios we're defending against here?

>
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -60,8 +60,10 @@ char *kstrdup(const char *s, gfp_t gfp)
>  
>  	len = strlen(s) + 1;
>  	buf = kmalloc_track_caller(len, gfp);
> -	if (buf)
> +	if (buf) {
>  		memcpy(buf, s, len);
> +		buf[len - 1] = '\0';
> +	}
>  	return buf;
>  }

Now I'll start receiving patches to remove this again.  Let's have a
code comment please.

And kstrdup() is now looking awfully similar to kstrndup().  Perhaps
there's a way to reduce duplication?

Re: [PATCH v2 05/10] mm/util: Fix possible race condition in kstrdup()

[Linus Torvalds]

On Thu, 13 Jun 2024 at 14:14, Andrew Morton <akpm@linux-foundation.org> wrote:
>
> The concept sounds a little strange.  If some code takes a copy of a
> string while some other code is altering it, yes, the result will be a
> mess.  This is why get_task_comm() exists, and why it uses locking.

The thing is, get_task_comm() is terminally broken.

Nobody sane uses it, and sometimes it's literally _because_ it uses locking.

Let's look at the numbers:

 - 39 uses of get_task_comm()

 - 2 uses of __get_task_comm() because the locking doesn't work

 - 447 uses of raw "current->comm"

 - 112 uses of raw 'ta*sk->comm' (and possibly

IOW, we need to just accept the fact that nobody actually wants to use
"get_task_comm()". It's a broken interface. It's inconvenient, and the
locking makes it worse.

Now, I'm not convinced that kstrdup() is what anybody should use
should, but of the 600 "raw" uses of ->comm, four of them do seem to
be kstrdup.

Not great, I think they could be removed, but they are examples of
people doing this. And I think it *would* be good to have the
guarantee that yes, the kstrdup() result is always a proper string,
even if it's used for unstable sources. Who knows what other unstable
sources exist?

I do suspect that most of the raw uses of 'xyz->comm' is for
printouts. And I think we would be better with a '%pTSK' vsnprintf()
format thing for that.

Sadly, I don't think coccinelle can do the kinds of transforms that
involve printf format strings.

And no, a printk() string still couldn't use the locking version.

               Linus

Re: [PATCH v2 05/10] mm/util: Fix possible race condition in kstrdup()

[Yafang Shao]

On Fri, Jun 14, 2024 at 5:14 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 13 Jun 2024 10:30:39 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
>
> > In kstrdup(), it is critical to ensure that the dest string is always
> > NUL-terminated. However, potential race condidtion can occur between a
> > writer and a reader.
> >
> > Consider the following scenario involving task->comm:
> >
> >     reader                    writer
> >
> >   len = strlen(s) + 1;
> >                              strlcpy(tsk->comm, buf, sizeof(tsk->comm));
> >   memcpy(buf, s, len);
> >
> > In this case, there is a race condition between the reader and the
> > writer. The reader calculate the length of the string `s` based on the
> > old value of task->comm. However, during the memcpy(), the string `s`
> > might be updated by the writer to a new value of task->comm.
> >
> > If the new task->comm is larger than the old one, the `buf` might not be
> > NUL-terminated. This can lead to undefined behavior and potential
> > security vulnerabilities.
> >
> > Let's fix it by explicitly adding a NUL-terminator.
>
> The concept sounds a little strange.  If some code takes a copy of a
> string while some other code is altering it, yes, the result will be a
> mess.  This is why get_task_comm() exists, and why it uses locking.
>
> I get that "your copy is a mess" is less serious than "your string
> isn't null-terminated" but still.  Whichever outcome we get, the
> calling code is buggy and should be fixed.
>
> Are there any other problematic scenarios we're defending against here?
>
> >
> > --- a/mm/util.c
> > +++ b/mm/util.c
> > @@ -60,8 +60,10 @@ char *kstrdup(const char *s, gfp_t gfp)
> >
> >       len = strlen(s) + 1;
> >       buf = kmalloc_track_caller(len, gfp);
> > -     if (buf)
> > +     if (buf) {
> >               memcpy(buf, s, len);
> > +             buf[len - 1] = '\0';
> > +     }
> >       return buf;
> >  }
>
> Now I'll start receiving patches to remove this again.  Let's have a
> code comment please.

I will add a comment for it.

>
> And kstrdup() is now looking awfully similar to kstrndup().  Perhaps
> there's a way to reduce duplication?

Yes, I believe we can add a common helper for them :

  static char *__kstrndup(const char *s, size_t max, gfp_t gfp)

-- 
Regards
Yafang

Re: [PATCH v2 05/10] mm/util: Fix possible race condition in kstrdup()

[Yafang Shao]

On Fri, Jun 14, 2024 at 6:18 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, 13 Jun 2024 at 14:14, Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > The concept sounds a little strange.  If some code takes a copy of a
> > string while some other code is altering it, yes, the result will be a
> > mess.  This is why get_task_comm() exists, and why it uses locking.
>
> The thing is, get_task_comm() is terminally broken.
>
> Nobody sane uses it, and sometimes it's literally _because_ it uses locking.
>
> Let's look at the numbers:
>
>  - 39 uses of get_task_comm()
>
>  - 2 uses of __get_task_comm() because the locking doesn't work
>
>  - 447 uses of raw "current->comm"
>
>  - 112 uses of raw 'ta*sk->comm' (and possibly
>
> IOW, we need to just accept the fact that nobody actually wants to use
> "get_task_comm()". It's a broken interface. It's inconvenient, and the
> locking makes it worse.
>
> Now, I'm not convinced that kstrdup() is what anybody should use
> should, but of the 600 "raw" uses of ->comm, four of them do seem to
> be kstrdup.
>
> Not great, I think they could be removed, but they are examples of
> people doing this. And I think it *would* be good to have the
> guarantee that yes, the kstrdup() result is always a proper string,
> even if it's used for unstable sources. Who knows what other unstable
> sources exist?
>
> I do suspect that most of the raw uses of 'xyz->comm' is for
> printouts. And I think we would be better with a '%pTSK' vsnprintf()
> format thing for that.

I will implement this change in the next step if no one else handles it.

>
> Sadly, I don't think coccinelle can do the kinds of transforms that
> involve printf format strings.

Yes, we need to carefully check them one by one.

>
> And no, a printk() string still couldn't use the locking version.
>
>                Linus



-- 
Regards
Yafang

Re: [PATCH v2 06/10] mm/kmemleak: Replace strncpy() with __get_task_comm()

[Catalin Marinas]

On Thu, Jun 13, 2024 at 08:10:17PM +0800, Yafang Shao wrote:
> On Thu, Jun 13, 2024 at 4:37 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Thu, Jun 13, 2024 at 10:30:40AM +0800, Yafang Shao wrote:
> > > Using __get_task_comm() to read the task comm ensures that the name is
> > > always NUL-terminated, regardless of the source string. This approach also
> > > facilitates future extensions to the task comm.
> > >
> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > ---
> > >  mm/kmemleak.c | 8 +-------
> > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > >
> > > diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> > > index d5b6fba44fc9..ef29aaab88a0 100644
> > > --- a/mm/kmemleak.c
> > > +++ b/mm/kmemleak.c
> > > @@ -663,13 +663,7 @@ static struct kmemleak_object *__alloc_object(gfp_t gfp)
> > >               strncpy(object->comm, "softirq", sizeof(object->comm));
> > >       } else {
> > >               object->pid = current->pid;
> > > -             /*
> > > -              * There is a small chance of a race with set_task_comm(),
> > > -              * however using get_task_comm() here may cause locking
> > > -              * dependency issues with current->alloc_lock. In the worst
> > > -              * case, the command line is not correct.
> > > -              */
> > > -             strncpy(object->comm, current->comm, sizeof(object->comm));
> > > +             __get_task_comm(object->comm, sizeof(object->comm), current);
> > >       }
> >
> > You deleted the comment stating why it does not use get_task_comm()
> > without explaining why it would be safe now. I don't recall the details
> > but most likely lockdep warned of some potential deadlocks with this
> > function being called with the task_lock held.
> >
> > So, you either show why this is safe or just use strscpy() directly here
> > (not sure we'd need strscpy_pad(); I think strscpy() would do, we just
> > need the NUL-termination).
> 
> The task_lock was dropped in patch #1 [0]. My apologies for not
> including you in the CC for that change. After this modification, it
> is now safe to use __get_task_comm().
> 
> [0] https://lore.kernel.org/all/20240613023044.45873-2-laoar.shao@gmail.com/

Ah, great. For this patch:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

You may want to add a comment in the commit log that since
__get_task_comm() no longer takes a long, it's safe to call it from
kmemleak.

-- 
Catalin

Re: [PATCH v2 06/10] mm/kmemleak: Replace strncpy() with __get_task_comm()

[Yafang Shao]

On Fri, Jun 14, 2024 at 6:57 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Thu, Jun 13, 2024 at 08:10:17PM +0800, Yafang Shao wrote:
> > On Thu, Jun 13, 2024 at 4:37 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > On Thu, Jun 13, 2024 at 10:30:40AM +0800, Yafang Shao wrote:
> > > > Using __get_task_comm() to read the task comm ensures that the name is
> > > > always NUL-terminated, regardless of the source string. This approach also
> > > > facilitates future extensions to the task comm.
> > > >
> > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > ---
> > > >  mm/kmemleak.c | 8 +-------
> > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > >
> > > > diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> > > > index d5b6fba44fc9..ef29aaab88a0 100644
> > > > --- a/mm/kmemleak.c
> > > > +++ b/mm/kmemleak.c
> > > > @@ -663,13 +663,7 @@ static struct kmemleak_object *__alloc_object(gfp_t gfp)
> > > >               strncpy(object->comm, "softirq", sizeof(object->comm));
> > > >       } else {
> > > >               object->pid = current->pid;
> > > > -             /*
> > > > -              * There is a small chance of a race with set_task_comm(),
> > > > -              * however using get_task_comm() here may cause locking
> > > > -              * dependency issues with current->alloc_lock. In the worst
> > > > -              * case, the command line is not correct.
> > > > -              */
> > > > -             strncpy(object->comm, current->comm, sizeof(object->comm));
> > > > +             __get_task_comm(object->comm, sizeof(object->comm), current);
> > > >       }
> > >
> > > You deleted the comment stating why it does not use get_task_comm()
> > > without explaining why it would be safe now. I don't recall the details
> > > but most likely lockdep warned of some potential deadlocks with this
> > > function being called with the task_lock held.
> > >
> > > So, you either show why this is safe or just use strscpy() directly here
> > > (not sure we'd need strscpy_pad(); I think strscpy() would do, we just
> > > need the NUL-termination).
> >
> > The task_lock was dropped in patch #1 [0]. My apologies for not
> > including you in the CC for that change. After this modification, it
> > is now safe to use __get_task_comm().
> >
> > [0] https://lore.kernel.org/all/20240613023044.45873-2-laoar.shao@gmail.com/
>
> Ah, great. For this patch:
>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>
> You may want to add a comment in the commit log that since
> __get_task_comm() no longer takes a long, it's safe to call it from
> kmemleak.

I will add it. Thanks for your suggestion.

-- 
Regards
Yafang