[PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Jan Engelhardt]

I found that the kernel BUG()s out, already during boot, when bumping
TASK_COMM_LEN to a value larger than 16 (and I can imagine the same
problem unfolding as well if it is set to something smaller).

Various places do insufficient length checks, simply assume certain
sizes or hardcode things. Even though e.g. get_task_comm clearly
documents that its buffer ought to be TASK_COMM_LEN long, I do believe
that an extra size parameter, such as added in this patch, is a lot
more robust than relying on callers getting the buffer size right.

With this patch, I no longer experience crashes, but that is not to
say that there are not any further places (e.g. in modules I never
use) with flakey ->comm handling.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 arch/x86/ia32/sys_ia32.c    |    2 +-
 drivers/connector/cn_proc.c |    3 ++-
 drivers/misc/pti.c          |    2 +-
 drivers/scsi/sg.c           |    4 ++--
 drivers/tty/tty_audit.c     |    2 +-
 drivers/tty/tty_ldisc.c     |    4 +++-
 fs/binfmt_elf.c             |    2 +-
 fs/exec.c                   |    7 +++----
 fs/proc/array.c             |    4 ++--
 include/linux/sched.h       |    2 +-
 kernel/auditsc.c            |    9 +++++----
 kernel/capability.c         |    4 ++--
 kernel/hrtimer.c            |    2 +-
 kernel/sched/core.c         |    3 ++-
 kernel/sys.c                |    6 +++---
 net/core/sock.c             |    4 ++--
 net/ipv6/ndisc.c            |    5 +++--
 17 files changed, 35 insertions(+), 30 deletions(-)

diff --git a/arch/x86/ia32/sys_ia32.c b/arch/x86/ia32/sys_ia32.c
index f6f5c53..1c9d77e 100644
--- a/arch/x86/ia32/sys_ia32.c
+++ b/arch/x86/ia32/sys_ia32.c
@@ -506,7 +506,7 @@ long sys32_vm86_warning(void)
 		compat_printk(KERN_INFO
 			      "%s: vm86 mode not supported on 64 bit kernel\n",
 			      me->comm);
-		strncpy(lastcomm, me->comm, sizeof(lastcomm));
+		strlcpy(lastcomm, me->comm, sizeof(lastcomm));
 	}
 	return -ENOSYS;
 }
diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 77e1e6c..b4482d7 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -223,7 +223,8 @@ void proc_comm_connector(struct task_struct *task)
 	ev->what = PROC_EVENT_COMM;
 	ev->event_data.comm.process_pid  = task->pid;
 	ev->event_data.comm.process_tgid = task->tgid;
-	get_task_comm(ev->event_data.comm.comm, task);
+	get_task_comm(ev->event_data.comm.comm,
+		      sizeof(ev->event_data.comm.comm), task);
 
 	memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
 	msg->ack = 0; /* not used */
diff --git a/drivers/misc/pti.c b/drivers/misc/pti.c
index 0b56e3f..943ca09 100644
--- a/drivers/misc/pti.c
+++ b/drivers/misc/pti.c
@@ -178,7 +178,7 @@ static void pti_control_frame_built_and_sent(struct pti_masterchannel *mc,
 
 	if (!thread_name) {
 		if (!in_interrupt())
-			get_task_comm(comm, current);
+			get_task_comm(comm, sizeof(comm), current);
 		else
 			strncpy(comm, "Interrupt", TASK_COMM_LEN);
 
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index eacd46b..f61a7bb 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -627,7 +627,7 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 	 */
 	if (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV) {
 		static char cmd[TASK_COMM_LEN];
-		if (strcmp(current->comm, cmd)) {
+		if (strncmp(cmd, current->comm, sizeof(cmd))) {
 			printk_ratelimited(KERN_WARNING
 					   "sg_write: data in/out %d/%d bytes "
 					   "for SCSI command 0x%x-- guessing "
@@ -636,7 +636,7 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 					   old_hdr.reply_len - (int)SZ_SG_HEADER,
 					   input_size, (unsigned int) cmnd[0],
 					   current->comm);
-			strcpy(cmd, current->comm);
+			strlcpy(cmd, current->comm, sizeof(cmd));
 		}
 	}
 	k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking);
diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
index 7c58669..3e6e5bd 100644
--- a/drivers/tty/tty_audit.c
+++ b/drivers/tty/tty_audit.c
@@ -75,7 +75,7 @@ static void tty_audit_log(const char *description, struct task_struct *tsk,
 				 "major=%d minor=%d comm=", description,
 				 tsk->pid, uid, loginuid, sessionid,
 				 major, minor);
-		get_task_comm(name, tsk);
+		get_task_comm(name, sizeof(name), tsk);
 		audit_log_untrustedstring(ab, name);
 		audit_log_format(ab, " data=");
 		audit_log_n_hex(ab, data, size);
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 24b95db..bf2f831 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -836,7 +836,9 @@ retry:
 				timeout = MAX_SCHEDULE_TIMEOUT;
 				printk_ratelimited(KERN_WARNING
 					"%s: waiting (%s) for %s took too long, but we keep waiting...\n",
-					__func__, get_task_comm(cur_n, current),
+					__func__,
+					get_task_comm(cur_n, sizeof(cur_n),
+						      current),
 					tty_name(tty, tty_n));
 			}
 			mutex_unlock(&tty->ldisc_mutex);
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index bcb884e..2bb6da1 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1345,7 +1345,7 @@ static int fill_psinfo(struct elf_prpsinfo *psinfo, struct task_struct *p,
 	SET_UID(psinfo->pr_uid, cred->uid);
 	SET_GID(psinfo->pr_gid, cred->gid);
 	rcu_read_unlock();
-	strncpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname));
+	strlcpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname));
 	
 	return 0;
 }
diff --git a/fs/exec.c b/fs/exec.c
index aeb135c..52f1a08 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1042,11 +1042,10 @@ static void flush_old_files(struct files_struct * files)
 	spin_unlock(&files->file_lock);
 }
 
-char *get_task_comm(char *buf, struct task_struct *tsk)
+char *get_task_comm(char *buf, size_t s, struct task_struct *tsk)
 {
-	/* buf must be at least sizeof(tsk->comm) in size */
 	task_lock(tsk);
-	strncpy(buf, tsk->comm, sizeof(tsk->comm));
+	strlcpy(buf, tsk->comm, s);
 	task_unlock(tsk);
 	return buf;
 }
@@ -1064,7 +1063,7 @@ void set_task_comm(struct task_struct *tsk, char *buf)
 	 * Readers without a lock may see incomplete new
 	 * names but are safe from non-terminating string reads.
 	 */
-	memset(tsk->comm, 0, TASK_COMM_LEN);
+	memset(tsk->comm, 0, sizeof(tsk->comm));
 	wmb();
 	strlcpy(tsk->comm, buf, sizeof(tsk->comm));
 	task_unlock(tsk);
diff --git a/fs/proc/array.c b/fs/proc/array.c
index c602b8d..60fb78c 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -93,7 +93,7 @@ static inline void task_name(struct seq_file *m, struct task_struct *p)
 	char *name;
 	char tcomm[sizeof(p->comm)];
 
-	get_task_comm(tcomm, p);
+	get_task_comm(tcomm, sizeof(tcomm), p);
 
 	seq_puts(m, "Name:\t");
 	end = m->buf + m->size;
@@ -390,7 +390,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 		}
 	}
 
-	get_task_comm(tcomm, task);
+	get_task_comm(tcomm, sizeof(tcomm), task);
 
 	sigemptyset(&sigign);
 	sigemptyset(&sigcatch);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 4032ec1..d2ba148 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2288,7 +2288,7 @@ extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned lon
 struct task_struct *fork_idle(int);
 
 extern void set_task_comm(struct task_struct *tsk, char *from);
-extern char *get_task_comm(char *to, struct task_struct *tsk);
+extern char *get_task_comm(char *to, size_t tosize, struct task_struct *tsk);
 
 #ifdef CONFIG_SMP
 void scheduler_ipi(void);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index caaea6e..5523440 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1161,7 +1161,7 @@ static void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk
 
 	/* tsk == current */
 
-	get_task_comm(name, tsk);
+	get_task_comm(name, sizeof(name), tsk);
 	audit_log_format(ab, " comm=");
 	audit_log_untrustedstring(ab, name);
 
@@ -2528,7 +2528,7 @@ void __audit_ptrace(struct task_struct *t)
 	context->target_uid = task_uid(t);
 	context->target_sessionid = audit_get_sessionid(t);
 	security_task_getsecid(t, &context->target_sid);
-	memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
+	strlcpy(context->target_comm, t->comm, sizeof(context->target_comm));
 }
 
 /**
@@ -2567,7 +2567,7 @@ int __audit_signal_info(int sig, struct task_struct *t)
 		ctx->target_uid = t_uid;
 		ctx->target_sessionid = audit_get_sessionid(t);
 		security_task_getsecid(t, &ctx->target_sid);
-		memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
+		strlcpy(ctx->target_comm, t->comm, sizeof(ctx->target_comm));
 		return 0;
 	}
 
@@ -2588,7 +2588,8 @@ int __audit_signal_info(int sig, 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(t, &axp->target_sid[axp->pid_count]);
-	memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
+	strlcpy(axp->target_comm[axp->pid_count], t->comm,
+		sizeof(*axp->target_comm));
 	axp->pid_count++;
 
 	return 0;
diff --git a/kernel/capability.c b/kernel/capability.c
index 3f1adb6..a68a788 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -48,7 +48,7 @@ static void warn_legacy_capability_use(void)
 
 		printk(KERN_INFO "warning: `%s' uses 32-bit capabilities"
 		       " (legacy support in use)\n",
-		       get_task_comm(name, current));
+		       get_task_comm(name, sizeof(name), current));
 		warned = 1;
 	}
 }
@@ -78,7 +78,7 @@ static void warn_deprecated_v2(void)
 
 		printk(KERN_INFO "warning: `%s' uses deprecated v2"
 		       " capabilities in a way that may be insecure.\n",
-		       get_task_comm(name, current));
+		       get_task_comm(name, sizeof(name), current));
 		warned = 1;
 	}
 }
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index ae34bf5..8806c64 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -774,7 +774,7 @@ static inline void timer_stats_hrtimer_set_start_info(struct hrtimer *timer)
 	if (timer->start_site)
 		return;
 	timer->start_site = __builtin_return_address(0);
-	memcpy(timer->start_comm, current->comm, TASK_COMM_LEN);
+	strlcpy(timer->start_comm, current->comm, sizeof(timer->start_comm));
 	timer->start_pid = current->pid;
 #endif
 }
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index df00cb0..c968185 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4881,7 +4881,8 @@ void __cpuinit init_idle(struct task_struct *idle, int cpu)
 	idle->sched_class = &idle_sched_class;
 	ftrace_graph_init_idle_task(idle, cpu);
 #if defined(CONFIG_SMP)
-	sprintf(idle->comm, "%s/%d", INIT_TASK_COMM, cpu);
+	snprintf(idle->comm, sizeof(idle->comm), "%s/%d",
+		 INIT_TASK_COMM, cpu);
 #endif
 }
 
diff --git a/kernel/sys.c b/kernel/sys.c
index 4070153..666c380 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1875,15 +1875,15 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 			break;
 
 		case PR_SET_NAME:
-			comm[sizeof(me->comm)-1] = 0;
+			comm[sizeof(comm)-1] = 0;
 			if (strncpy_from_user(comm, (char __user *)arg2,
-					      sizeof(me->comm) - 1) < 0)
+					      sizeof(comm) - 1) < 0)
 				return -EFAULT;
 			set_task_comm(me, comm);
 			proc_comm_connector(me);
 			return 0;
 		case PR_GET_NAME:
-			get_task_comm(comm, me);
+			get_task_comm(comm, sizeof(comm), me);
 			if (copy_to_user((char __user *)arg2, comm,
 					 sizeof(comm)))
 				return -EFAULT;
diff --git a/net/core/sock.c b/net/core/sock.c
index 5c5af998..a5c284c 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -313,8 +313,8 @@ static void sock_warn_obsolete_bsdism(const char *name)
 {
 	static int warned;
 	static char warncomm[TASK_COMM_LEN];
-	if (strcmp(warncomm, current->comm) && warned < 5) {
-		strcpy(warncomm,  current->comm);
+	if (strncmp(warncomm, current->comm, sizeof(warncomm)) && warned < 5) {
+		strlcpy(warncomm,  current->comm, sizeof(warncomm));
 		printk(KERN_WARNING "process `%s' is using obsolete "
 		       "%s SO_BSDCOMPAT\n", warncomm, name);
 		warned++;
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index d8f02ef..5efb524 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1748,8 +1748,9 @@ 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);
+	if (strncmp(warncomm, current->comm, sizeof(warncomm)) &&
+	    warned < 5) {
+		strlcpy(warncomm, current->comm, sizeof(warncomm));
 		printk(KERN_WARNING
 			"process `%s' is using deprecated sysctl (%s) "
 			"net.ipv6.neigh.%s.%s; "
-- 
1.7.7

[PATCH 2/2] treewide: avoid truncation of process name

[Jan Engelhardt]

While producing the previous commit, "treewide: fix memory corruptions
when TASK_COMM_LEN != 16", I noticed

1. the process name is truncated for no apparent reason in a number of
places by means of the precision for the %s specifier
(t->comm is always NUL-terminated).
2. padded with an odd size (8) in another
3. redundant instance of: t->tcomm ? t->comm : ""
(t->comm is always non-NULL because it is an array)

Cc: Ingo Molnar <mingo@redhat.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Steve French <sfrench@samba.org>
Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 arch/x86/kernel/dumpstack.c  |    2 +-
 arch/x86/kernel/process.c    |    2 +-
 arch/x86/kernel/process_64.c |    2 +-
 kernel/cred.c                |    4 ++--
 kernel/irq/manage.c          |    2 +-
 kernel/sched/core.c          |    2 +-
 net/dns_resolver/internal.h  |    2 +-
 7 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 1aae78f..88d1c92 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -185,7 +185,7 @@ void dump_stack(void)
 	unsigned long stack;
 
 	bp = stack_frame(current, NULL);
-	printk("Pid: %d, comm: %.20s %s %s %.*s\n",
+	printk("Pid: %d, comm: %s %s %s %.*s\n",
 		current->pid, current->comm, print_tainted(),
 		init_utsname()->release,
 		(int)strcspn(init_utsname()->version, " "),
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 15763af..c7a73ad 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -105,7 +105,7 @@ void show_regs_common(void)
 	board = dmi_get_system_info(DMI_BOARD_NAME);
 
 	printk(KERN_CONT "\n");
-	printk(KERN_DEFAULT "Pid: %d, comm: %.20s %s %s %.*s",
+	printk(KERN_DEFAULT "Pid: %d, comm: %s %s %s %.*s",
 		current->pid, current->comm, print_tainted(),
 		init_utsname()->release,
 		(int)strcspn(init_utsname()->version, " "),
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 9b9fe4a..075610b 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -225,7 +225,7 @@ void release_thread(struct task_struct *dead_task)
 {
 	if (dead_task->mm) {
 		if (dead_task->mm->context.size) {
-			printk("WARNING: dead process %8s still has LDT? <%p/%d>\n",
+			printk("WARNING: dead process %s still has LDT? <%p/%d>\n",
 					dead_task->comm,
 					dead_task->mm->context.ldt,
 					dead_task->mm->context.size);
diff --git a/kernel/cred.c b/kernel/cred.c
index 5791612..7f10154 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -20,10 +20,10 @@
 
 #if 0
 #define kdebug(FMT, ...) \
-	printk("[%-5.5s%5u] "FMT"\n", current->comm, current->pid ,##__VA_ARGS__)
+	printk("[%s/%u] "FMT"\n", current->comm, current->pid ,##__VA_ARGS__)
 #else
 #define kdebug(FMT, ...) \
-	no_printk("[%-5.5s%5u] "FMT"\n", current->comm, current->pid ,##__VA_ARGS__)
+	no_printk("[%s/%u] "FMT"\n", current->comm, current->pid ,##__VA_ARGS__)
 #endif
 
 static struct kmem_cache *cred_jar;
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index a9a9dbe..7de16ae 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -838,7 +838,7 @@ void exit_irq_thread(void)
 
 	printk(KERN_ERR
 	       "exiting task \"%s\" (%d) is an active IRQ thread (irq %d)\n",
-	       tsk->comm ? tsk->comm : "", tsk->pid, tsk->irqaction->irq);
+	       tsk->comm, tsk->pid, tsk->irqaction->irq);
 
 	desc = irq_to_desc(tsk->irqaction->irq);
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index c968185..6caf7cf 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4769,7 +4769,7 @@ void sched_show_task(struct task_struct *p)
 	unsigned state;
 
 	state = p->state ? __ffs(p->state) + 1 : 0;
-	printk(KERN_INFO "%-15.15s %c", p->comm,
+	printk(KERN_INFO "%s %c", p->comm,
 		state < sizeof(stat_nam) - 1 ? stat_nam[state] : '?');
 #if BITS_PER_LONG == 32
 	if (state == TASK_RUNNING)
diff --git a/net/dns_resolver/internal.h b/net/dns_resolver/internal.h
index 189ca9e..8341712 100644
--- a/net/dns_resolver/internal.h
+++ b/net/dns_resolver/internal.h
@@ -36,7 +36,7 @@ extern unsigned dns_resolver_debug;
 #define	kdebug(FMT, ...)				\
 do {							\
 	if (unlikely(dns_resolver_debug))		\
-		printk(KERN_DEBUG "[%-6.6s] "FMT"\n",	\
+		printk(KERN_DEBUG "[%s] "FMT"\n",	\
 		       current->comm, ##__VA_ARGS__);	\
 } while (0)
 
-- 
1.7.7

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Andrew Morton]

On Sat, 21 Jan 2012 23:09:44 +0100
Jan Engelhardt <jengelh@medozas.de> wrote:

> I found that the kernel BUG()s out, already during boot, when bumping
> TASK_COMM_LEN to a value larger than 16

We can never increase TASK_COMM_LEN - it is part of the kernel ABI/API.
Doing so would destroy existing userspace which uses 16-byte buffers.

> (and I can imagine the same
> problem unfolding as well if it is set to something smaller).

hm, that's a surprise.  Decreasing TASK_COMM_LEN is at least slightly
possible but it's hard to see why we should do so.

> Various places do insufficient length checks, simply assume certain
> sizes or hardcode things. Even though e.g. get_task_comm clearly
> documents that its buffer ought to be TASK_COMM_LEN long, I do believe
> that an extra size parameter, such as added in this patch, is a lot
> more robust than relying on callers getting the buffer size right.
> 
> With this patch, I no longer experience crashes, but that is not to
> say that there are not any further places (e.g. in modules I never
> use) with flakey ->comm handling.

You do seem to have found a few warts around the task->comm handling. 
But I don't believe that addressing them justifies adding new code
(adding another argument to get_task_comm).

If you're interested in working on this stuff I'd suggest that we
confine ourselves to cleaning things up (without adding code) rather
than permitting a different TASK_COMM_LEN.  Things like replacing "16"
with TASK_COMM_LEN.

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Jan Engelhardt]

On Tuesday 2012-01-24 22:54, Andrew Morton wrote:

>On Sat, 21 Jan 2012 23:09:44 +0100
>Jan Engelhardt <jengelh@medozas.de> wrote:
>
>> I found that the kernel BUG()s out, already during boot, when bumping
>> TASK_COMM_LEN to a value larger than 16
>
>We can never increase TASK_COMM_LEN - it is part of the kernel ABI/API.
>Doing so would destroy existing userspace which uses 16-byte buffers.

I do not see TASK_COMM_LEN being exposed to userspace. In fact, it is 
behind a #ifdef __KERNEL__.

There is nothing wrong with userspace using a buffer with a size 
different from the object's size within the kernel. It's done all the 
time, in fact. readlink(2) for example also has a size argument rather 
than declaring to all parties they have to use PATH_MAX everytime.

>If you're interested in working on this stuff I'd suggest that we
>confine ourselves to cleaning things up (without adding code) rather
>than permitting a different TASK_COMM_LEN.  Things like replacing "16"
>with TASK_COMM_LEN.

There is what I would call the "Plate of Tunable Macros". #defines
that invite the reader to change them (think PID_MAX_DEFAULT).

If there is a piece of kernel code that
assumes/requests that userspace use a 16-byte buffer (such as
cn_proc as mentioned), then it should use a file-level define or
something with a comment above it that this is a fixed user value.

I would therefore say that changing TASK_COMM_LEN is possible without 
breaking any userprogram.

(In other words, TASK_COMM_LEN can just remain what it is, and 
comm[TASK_COMM_LEN] in struct task_struct could be changed to 
e.g. comm[32]. Using sizeof(x.comm) also seems more proof in general.)

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Andrew Morton]

On Wed, 1 Feb 2012 02:15:10 +0100 (CET) Jan Engelhardt <jengelh@medozas.de> wrote:

> On Tuesday 2012-01-24 22:54, Andrew Morton wrote:
> 
> >On Sat, 21 Jan 2012 23:09:44 +0100
> >Jan Engelhardt <jengelh@medozas.de> wrote:
> >
> >> I found that the kernel BUG()s out, already during boot, when bumping
> >> TASK_COMM_LEN to a value larger than 16
> >
> >We can never increase TASK_COMM_LEN - it is part of the kernel ABI/API.
> >Doing so would destroy existing userspace which uses 16-byte buffers.
> 
> I do not see TASK_COMM_LEN being exposed to userspace. In fact, it is 
> behind a #ifdef __KERNEL__.

Userspace uses "16".  Because it knows that what's the kernel uses. 
It's part of the ABI.

> There is nothing wrong with userspace using a buffer with a size 
> different from the object's size within the kernel. It's done all the 
> time, in fact. readlink(2) for example also has a size argument rather 
> than declaring to all parties they have to use PATH_MAX everytime.

prctl(PR_GET_NAME) unconditionally copies out 16 bytes.

> >If you're interested in working on this stuff I'd suggest that we
> >confine ourselves to cleaning things up (without adding code) rather
> >than permitting a different TASK_COMM_LEN.  Things like replacing "16"
> >with TASK_COMM_LEN.
> 
> There is what I would call the "Plate of Tunable Macros". #defines
> that invite the reader to change them (think PID_MAX_DEFAULT).
> 
> If there is a piece of kernel code that
> assumes/requests that userspace use a 16-byte buffer (such as
> cn_proc as mentioned), then it should use a file-level define or
> something with a comment above it that this is a fixed user value.
> 
> I would therefore say that changing TASK_COMM_LEN is possible without 
> breaking any userprogram.
> 
> (In other words, TASK_COMM_LEN can just remain what it is, and 
> comm[TASK_COMM_LEN] in struct task_struct could be changed to 
> e.g. comm[32]. Using sizeof(x.comm) also seems more proof in general.)

Well yes, we could increase the size and provide new and better APIs
for accessing it, while teaching the old APIs to truncate.  That might
cause some problems for old-API-using userspace during the transition
period, but I doubt if they would be large problems.

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Jan Engelhardt]

On Wednesday 2012-02-01 02:23, Andrew Morton wrote:
>>
>> If there is a piece of kernel code that
>> assumes/requests that userspace use a 16-byte buffer (such as
>> cn_proc as mentioned), then it should use a file-level define or
>> something with a comment above it that this is a fixed user value.
>> 
>> I would therefore say that changing TASK_COMM_LEN is possible without 
>> breaking any userprogram.
>> 
>> (In other words, TASK_COMM_LEN can just remain what it is, and 
>> comm[TASK_COMM_LEN] in struct task_struct could be changed to 
>> e.g. comm[32]. Using sizeof(x.comm) also seems more proof in general.)
>
>Well yes, we could increase the size and provide new and better APIs
>for accessing it, while teaching the old APIs to truncate.  That might
>cause some problems for old-API-using userspace during the transition
>period, but I doubt if they would be large problems.

Did my patch not change the existing code sites using ->comm
to always copy at most min(userbufsize aka 16, sizeof(t->comm)) bytes,
thereby keeping the promise to userspace while at the same time
making TASK_COMM_LEN's value freely choosable?

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Andrew Morton]

On Wed, 1 Feb 2012 02:35:06 +0100 (CET) Jan Engelhardt <jengelh@medozas.de> wrote:

> 
> On Wednesday 2012-02-01 02:23, Andrew Morton wrote:
> >>
> >> If there is a piece of kernel code that
> >> assumes/requests that userspace use a 16-byte buffer (such as
> >> cn_proc as mentioned), then it should use a file-level define or
> >> something with a comment above it that this is a fixed user value.
> >> 
> >> I would therefore say that changing TASK_COMM_LEN is possible without 
> >> breaking any userprogram.
> >> 
> >> (In other words, TASK_COMM_LEN can just remain what it is, and 
> >> comm[TASK_COMM_LEN] in struct task_struct could be changed to 
> >> e.g. comm[32]. Using sizeof(x.comm) also seems more proof in general.)
> >
> >Well yes, we could increase the size and provide new and better APIs
> >for accessing it, while teaching the old APIs to truncate.  That might
> >cause some problems for old-API-using userspace during the transition
> >period, but I doubt if they would be large problems.
> 
> Did my patch not change the existing code sites using ->comm
> to always copy at most min(userbufsize aka 16, sizeof(t->comm)) bytes,
> thereby keeping the promise to userspace while at the same time
> making TASK_COMM_LEN's value freely choosable?

That change is pretty pointless as long as we don't provide APIs to let
userspace access the expanded size.  And I've explained why we cannot
alter the existing APIs.

So until and unless we decide to add the new APIs I think that the
cleanup parts of the patch are good, but the parts which increase the
kernel text footprint are not.

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Jan Engelhardt]

On Wednesday 2012-02-01 02:49, Andrew Morton wrote:
>> 
>> Did my patch not change the existing code sites using ->comm
>> to always copy at most min(userbufsize aka 16, sizeof(t->comm)) bytes,
>> thereby keeping the promise to userspace while at the same time
>> making TASK_COMM_LEN's value freely choosable?
>
>That change is pretty pointless as long as we don't provide APIs to let
>userspace access the expanded size.  And I've explained why we cannot
>alter the existing APIs.

Ah yes, indeed. My reason for augmenting the size of t->comm was so
that `ps afx` could show a more complete name of certain kernel
threads' names. In this case, the kernel delivers the name via
procfs via seq_printf("%s, t->comm), as do a few debug statements
in the fashion of pr_debug("%s/%u ate my CPU", t->comm, t->pid).
So maybe it was not /completely/ pointless.

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Andrew Morton]

On Wed, 1 Feb 2012 03:15:50 +0100 (CET) Jan Engelhardt <jengelh@medozas.de> wrote:

> 
> On Wednesday 2012-02-01 02:49, Andrew Morton wrote:
> >> 
> >> Did my patch not change the existing code sites using ->comm
> >> to always copy at most min(userbufsize aka 16, sizeof(t->comm)) bytes,
> >> thereby keeping the promise to userspace while at the same time
> >> making TASK_COMM_LEN's value freely choosable?
> >
> >That change is pretty pointless as long as we don't provide APIs to let
> >userspace access the expanded size.  And I've explained why we cannot
> >alter the existing APIs.
> 
> Ah yes, indeed. My reason for augmenting the size of t->comm was so
> that `ps afx` could show a more complete name of certain kernel
> threads' names. In this case, the kernel delivers the name via
> procfs via seq_printf("%s, t->comm),

Where does procfs do this?

> as do a few debug statements
> in the fashion of pr_debug("%s/%u ate my CPU", t->comm, t->pid).
> So maybe it was not /completely/ pointless.

I agree that the 16-char thing is irritatingly small.  But if we were
to increase it and to then utilise that increase, those userspace apps
which are still using the legacy prctl(PR_GET_NAME) would produce
pretty bad output.  Instead of "migration/0" and "migration/1" you'd
get "migration_threa" and "migration_threa".  And "flush-8:32" would
maddeningly become "flusher_thread-".

I suppose that would help motivate people to update their tools ;)

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Jan Engelhardt]

On Wednesday 2012-02-01 04:01, Andrew Morton wrote:
>> 
>> Ah yes, indeed. My reason for augmenting the size of t->comm was so
>> that `ps afx` could show a more complete name of certain kernel
>> threads' names. In this case, the kernel delivers the name via
>> procfs via seq_printf("%s, t->comm),
>
>Where does procfs do this?

In the handler for /proc/n/stat and status.

Meanwhile I have approached this from a different angle by using a
union. (And then I discovered that even psutils has a dumb 16-byte
buffer as well such that output is truncated -.- but that's another
story.)

parent b01543dfe67bb1d191998e90d20534dc354de059 (v3.3-rc4)
commit 726ff573fe3f2722ec7bd765b84750ceda5e518e
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Tue Feb 21 06:21:38 2012 +0100

task: provide a larger task command buffer
---
 fs/exec.c               |   16 +++++++++++++---
 fs/proc/array.c         |    8 ++++----
 include/linux/binfmts.h |    5 ++++-
 include/linux/sched.h   |   20 ++++++++++++++++----
 kernel/exit.c           |    2 +-
 kernel/kthread.c        |    4 ++--
 6 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 92ce83a..1191a52 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1052,6 +1052,15 @@ char *get_task_comm(char *buf, struct task_struct *tsk)
 }
 EXPORT_SYMBOL_GPL(get_task_comm);
 
+char *get_task_comm_ex(char *buf, size_t z, struct task_struct *tsk)
+{
+	task_lock(tsk);
+	strlcpy(buf, tsk->comm_ex, z);
+	task_unlock(tsk);
+	return buf;
+}
+EXPORT_SYMBOL_GPL(get_task_comm_ex);
+
 void set_task_comm(struct task_struct *tsk, char *buf)
 {
 	task_lock(tsk);
@@ -1064,9 +1073,9 @@ void set_task_comm(struct task_struct *tsk, char *buf)
 	 * Readers without a lock may see incomplete new
 	 * names but are safe from non-terminating string reads.
 	 */
-	memset(tsk->comm, 0, TASK_COMM_LEN);
+	memset(tsk->comm_ex, 0, sizeof(tsk->comm_ex));
 	wmb();
-	strlcpy(tsk->comm, buf, sizeof(tsk->comm));
+	strlcpy(tsk->comm_ex, buf, sizeof(tsk->comm_ex));
 	task_unlock(tsk);
 	perf_event_comm(tsk);
 }
@@ -1100,7 +1109,8 @@ int flush_old_exec(struct linux_binprm * bprm)
 
 	set_mm_exe_file(bprm->mm, bprm->file);
 
-	filename_to_taskname(bprm->tcomm, bprm->filename, sizeof(bprm->tcomm));
+	filename_to_taskname(bprm->tcomm_ex, bprm->filename,
+			     sizeof(bprm->tcomm_ex));
 	/*
 	 * Release all of the old mmap stuff
 	 */
diff --git a/fs/proc/array.c b/fs/proc/array.c
index c602b8d..4bdbeb0 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -91,9 +91,9 @@ static inline void task_name(struct seq_file *m, struct task_struct *p)
 	int i;
 	char *buf, *end;
 	char *name;
-	char tcomm[sizeof(p->comm)];
+	char tcomm[sizeof(p->comm_ex)];
 
-	get_task_comm(tcomm, p);
+	get_task_comm_ex(tcomm, sizeof(tcomm), p);
 
 	seq_puts(m, "Name:\t");
 	end = m->buf + m->size;
@@ -375,7 +375,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 	cputime_t cutime, cstime, utime, stime;
 	cputime_t cgtime, gtime;
 	unsigned long rsslim = 0;
-	char tcomm[sizeof(task->comm)];
+	char tcomm[sizeof(task->comm_ex)];
 	unsigned long flags;
 
 	state = *get_task_state(task);
@@ -390,7 +390,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 		}
 	}
 
-	get_task_comm(tcomm, task);
+	get_task_comm_ex(tcomm, sizeof(tcomm), task);
 
 	sigemptyset(&sigign);
 	sigemptyset(&sigcatch);
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 0092102..a19d1d1 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -58,7 +58,10 @@ struct linux_binprm {
 	unsigned interp_flags;
 	unsigned interp_data;
 	unsigned long loader, exec;
-	char tcomm[TASK_COMM_LEN];
+	union {
+		char tcomm[TASK_COMM_LEN];
+		char tcomm_ex[32];
+	};
 };
 
 #define BINPRM_FLAGS_ENFORCE_NONDUMP_BIT 0
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 7d379a6..977eced 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1378,10 +1378,21 @@ struct task_struct {
 					 * credentials (COW) */
 	struct cred *replacement_session_keyring; /* for KEYCTL_SESSION_TO_PARENT */
 
-	char comm[TASK_COMM_LEN]; /* executable name excluding path
-				     - access with [gs]et_task_comm (which lock
-				       it with task_lock())
-				     - initialized normally by setup_new_exec */
+	union {
+		/*
+		 * Executable name excluding path. Access it with
+		 * [gs]et_task_comm (which lock it with task_lock()).
+		 * Initialized normally by setup_new_exec.
+		 */
+		char comm[TASK_COMM_LEN];
+		/*
+		 * The .comm member is kept, while we try to identify the codes
+		 * that unfortunately rely on its exact size of 16. Apparently
+		 * not all those sites have been spotted in previous attempts
+		 * of mine to enlarge comm.
+		 */
+		char comm_ex[32];
+	};
 /* file system info */
 	int link_count, total_link_count;
 #ifdef CONFIG_SYSVIPC
@@ -2295,6 +2306,7 @@ struct task_struct *fork_idle(int);
 
 extern void set_task_comm(struct task_struct *tsk, char *from);
 extern char *get_task_comm(char *to, struct task_struct *tsk);
+extern char *get_task_comm_ex(char *to, size_t z, struct task_struct *tsk);
 
 #ifdef CONFIG_SMP
 void scheduler_ipi(void);
diff --git a/kernel/exit.c b/kernel/exit.c
index 4b4042f..2c9569a 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -414,7 +414,7 @@ void daemonize(const char *name, ...)
 	sigset_t blocked;
 
 	va_start(args, name);
-	vsnprintf(current->comm, sizeof(current->comm), name, args);
+	vsnprintf(current->comm_ex, sizeof(current->comm_ex), name, args);
 	va_end(args);
 
 	/*
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 3d3de63..09dba54 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -196,8 +196,8 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
 		va_list args;
 
 		va_start(args, namefmt);
-		vsnprintf(create.result->comm, sizeof(create.result->comm),
-			  namefmt, args);
+		vsnprintf(create.result->comm_ex,
+			  sizeof(create.result->comm_ex), namefmt, args);
 		va_end(args);
 		/*
 		 * root may have changed our (kthreadd's) priority or CPU mask.
-- 
# Created with git-export-patch

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Andrew Morton]

On Wed, 22 Feb 2012 13:48:08 +0100 (CET)
Jan Engelhardt <jengelh@medozas.de> wrote:

> task: provide a larger task command buffer

<scratches head>

Why are we bothering ourselves about this?

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Jan Engelhardt]

On Wednesday 2012-02-22 21:58, Andrew Morton wrote:

>On Wed, 22 Feb 2012 13:48:08 +0100 (CET)
>Jan Engelhardt <jengelh@medozas.de> wrote:
>
>> task: provide a larger task command buffer
>
><scratches head>
>
>Why are we bothering ourselves about this?

Some prefer to know what's going on in the system. Every other or 
so kernel release there are some new happy kthreads, such as

24930 ?        S      0:00  \_ [btrfs-endio-1]
24931 ?        S      0:00  \_ [btrfs-endio-met]
24932 ?        S      0:00  \_ [btrfs-endio-met]
24933 ?        S      0:00  \_ [btrfs-endio-wri]
24934 ?        S      0:00  \_ [btrfs-freespace]

at which point one is curious to find out the rest of the met and why 
there are two of them. If expanded one actually sees they are different 
kthreads (rather than just per-cpu instances for a WQ, for example)

$ grep Name /proc/{29431,29432}/stat*
/proc/29431/status:Name: btrfs-endio-meta-1
/proc/29432/status:Name: btrfs-endio-meta-write-1

That's all.

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Andrew Morton]

On Thu, 23 Feb 2012 10:09:33 +0100 (CET) Jan Engelhardt <jengelh@medozas.de> wrote:

> On Wednesday 2012-02-22 21:58, Andrew Morton wrote:
> 
> >On Wed, 22 Feb 2012 13:48:08 +0100 (CET)
> >Jan Engelhardt <jengelh@medozas.de> wrote:
> >
> >> task: provide a larger task command buffer
> >
> ><scratches head>
> >
> >Why are we bothering ourselves about this?
> 
> Some prefer to know what's going on in the system. Every other or 
> so kernel release there are some new happy kthreads, such as
> 
> 24930 ?        S      0:00  \_ [btrfs-endio-1]
> 24931 ?        S      0:00  \_ [btrfs-endio-met]
> 24932 ?        S      0:00  \_ [btrfs-endio-met]
> 24933 ?        S      0:00  \_ [btrfs-endio-wri]
> 24934 ?        S      0:00  \_ [btrfs-freespace]
> 
> at which point one is curious to find out the rest of the met and why 
> there are two of them. If expanded one actually sees they are different 
> kthreads (rather than just per-cpu instances for a WQ, for example)
> 
> $ grep Name /proc/{29431,29432}/stat*
> /proc/29431/status:Name: btrfs-endio-meta-1
> /proc/29432/status:Name: btrfs-endio-meta-write-1
> 
> That's all.

doh.  The fix for that is to have less clueless btrfs developers.

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Jan Engelhardt]

On Thursday 2012-02-23 10:57, Andrew Morton wrote:
>>
But there's more,
>> 
>> 24931 ?        S      0:00  \_ [btrfs-endio-met]
                               \_ [kconservative/5]
                               \_ [ext4-dio-unwrit]
>> 
>> [with a wondersome patch:] $ grep Name /proc/{29431,29432}/stat*
>> /proc/29431/status:Name: btrfs-endio-meta-1
>> /proc/29432/status:Name: btrfs-endio-meta-write-1
                      Name: kconservative/512
                      Name: ext4-dio-unwritten
>
>doh.  The fix for that is to have less clueless btrfs developers.

And truncate their names to SUNWbtfs, ORCLintg and EXT4diou?
I think not :)

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Andrew Morton]

On Thu, 23 Feb 2012 12:19:28 +0100 (CET) Jan Engelhardt <jengelh@medozas.de> wrote:

> 
> On Thursday 2012-02-23 10:57, Andrew Morton wrote:
> >>
> But there's more,
> >> 
> >> 24931 ?        S      0:00  \_ [btrfs-endio-met]
>                                \_ [kconservative/5]
>                                \_ [ext4-dio-unwrit]
> >> 
> >> [with a wondersome patch:] $ grep Name /proc/{29431,29432}/stat*
> >> /proc/29431/status:Name: btrfs-endio-meta-1
> >> /proc/29432/status:Name: btrfs-endio-meta-write-1
>                       Name: kconservative/512
>                       Name: ext4-dio-unwritten
> >
> >doh.  The fix for that is to have less clueless btrfs developers.
> 
> And truncate their names to SUNWbtfs, ORCLintg and EXT4diou?
> I think not :)

Teach ps(1) to look in /proc/pid/status for kernel threads?

Re: [PATCH 1/2] treewide: fix memory corruptions when TASK_COMM_LEN != 16

[Jan Engelhardt]

On Thursday 2012-02-23 18:30, Andrew Morton wrote:
>
>Teach ps(1) to look in /proc/pid/status for kernel threads?

To what end? The name in /proc/pid/status was also limited to
TASK_COMM_LEN.