sanitize task->comm to avoid leaking escape codes

include/linux/sched.h extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned lon · line 2121

Quoted 3 lines · Kees Cook

include/linux/sched.h +2 -1

-extern char *get_task_comm(char *to, struct task_struct *tsk);
+#define get_task_comm(buf, task) get_task_comm_size(buf, sizeof(buf), task)
+extern char *get_task_comm_size(char *to, size_t len, struct task_struct *tsk);

Oh, but this means that get_task_comm(ptr, task) doesn't work?

Quoted 7 lines · Oleg Nesterov

include/linux/sched.h extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned lon · line 2121

Quoted 3 lines · Kees Cook

include/linux/sched.h +2 -1

-extern char *get_task_comm(char *to, struct task_struct *tsk);
+#define get_task_comm(buf, task) get_task_comm_size(buf, sizeof(buf), task)
+extern char *get_task_comm_size(char *to, size_t len, struct task_struct *tsk);

Oh, but this means that get_task_comm(ptr, task) doesn't work?

The number of users is so small, and everyone uses TASK_COMM_LEN, so maybe nothing should be done or "char buf[TASK_COMM_LEN]"?

Quoted 7 lines · Oleg Nesterov

include/linux/sched.h extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned lon · line 2121

Quoted 3 lines · Kees Cook

include/linux/sched.h +2 -1

-extern char *get_task_comm(char *to, struct task_struct *tsk);
+#define get_task_comm(buf, task) get_task_comm_size(buf, sizeof(buf), task)
+extern char *get_task_comm_size(char *to, size_t len, struct task_struct *tsk);

Oh, but this means that get_task_comm(ptr, task) doesn't work?

The number of users is so small, and everyone uses TASK_COMM_LEN, so maybe nothing should be done or "char buf[TASK_COMM_LEN]"?

Surely it would be better to fix the tools which display this info rather than making the kernel tell fibs.

Quoted 2 lines · Andrew Morton

Surely it would be better to fix the tools which display this info rather than making the kernel tell fibs.

The strncpy in get_task_comm() is totally wrong -- it's testing the length of task->comm.

Why should get_task_comm not take a destination buffer length argument?

Quoted 4 lines · Kees Cook

Quoted 2 lines · Andrew Morton

Surely it would be better to fix the tools which display this info rather than making the kernel tell fibs.

Quoted 2 lines

The strncpy in get_task_comm() is totally wrong -- it's testing the length of task->comm.

It also fills not just any buffer but buffer which is TASK_COMM_LEN byte wide.

Quoted 1 line

Why should get_task_comm not take a destination buffer length argument?

If you pass too small, you needlessly truncate output.

If you pass too large, you waste space.

Quoted 5 lines · Alexey Dobriyan

Quoted 4 lines · Kees Cook

Quoted 2 lines · Andrew Morton

Surely it would be better to fix the tools which display this info rather than making the kernel tell fibs.

Quoted 8 lines

Quoted 2 lines

The strncpy in get_task_comm() is totally wrong -- it's testing the length of task->comm.

It also fills not just any buffer but buffer which is TASK_COMM_LEN byte wide.

Quoted 1 line

Why should get_task_comm not take a destination buffer length argument?

If you pass too small, you needlessly truncate output.

If you pass too small a buffer, get_task_comm will happily write all over the caller's stack past the end of the buffer if the contents of task->comm are large enough:

        strncpy(buf, tsk->comm, sizeof(tsk->comm));

The "n" argument to get_task_comm's use of strncpy is totally wrong -- it needs to be the size of the destination, not the size of the source. Luckily, everyone using get_task_comm currently uses buffers that are sizeof(task->comm).

Quoted 19 lines · On Tue, 29 Jun 2010 08:09:52 -0700 Kees Cook <[email protected]> wrote:

Quoted 5 lines · Alexey Dobriyan

Quoted 4 lines · Kees Cook

Quoted 2 lines · Andrew Morton

Surely it would be better to fix the tools which display this info rather than making the kernel tell fibs.

Quoted 8 lines

Quoted 2 lines

The strncpy in get_task_comm() is totally wrong -- it's testing the length of task->comm.

It also fills not just any buffer but buffer which is TASK_COMM_LEN byte wide.

Quoted 1 line

Why should get_task_comm not take a destination buffer length argument?

If you pass too small, you needlessly truncate output.

If you pass too small a buffer, get_task_comm will happily write all over the caller's stack past the end of the buffer if the contents of task->comm are large enough:

Quoted 6 lines

        strncpy(buf, tsk->comm, sizeof(tsk->comm));

The "n" argument to get_task_comm's use of strncpy is totally wrong -- it needs to be the size of the destination, not the size of the source. Luckily, everyone using get_task_comm currently uses buffers that are sizeof(task->comm).

It's not "totally wrong" at all. get_task_comm() *requires* that it be

passed a buffer of at least TASK_COMM_LEN bytes. sizeof(tsk->comm) equals TASK_COMM_LEN and always will do so. We could replace the sizeof with TASK_COMM_LEN for cosmetic reasons but that's utter nitpicking. But then, the comment right there says "buf must be at least sizeof(tsk->comm) in size". That's so simple that even a kernel developer could understand it?

Do we need a runtime check every time to make sure that some developer didn't misunderstand such a simple thing? Seems pretty pointless - there are a zillion such runtime checks we could add. It'd be better to do

#define get_task_comm(buf, tsk) {			\
	BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN);	\
	__get_task_comm(buf, tsk);			\
}

and save the runtime bloat. But again, what was special about this particular programmer error? There are five or six instances of strcpy(foo, current->comm). Do we need runtime checks there as well??

fs/exec.c void set_task_comm(struct task_struct *tsk, char *buf) · line 959

Quoted 5 lines · Kees Cook

fs/exec.c +0 -0

@@ -956,7 +957,15 @@ void set_task_comm(struct task_struct *tsk, char *buf)
 	 */
 	memset(tsk->comm, 0, TASK_COMM_LEN);
 	wmb();

Off-topic. I'd wish I could understand this barrier. Since the lockless reader doesn't do rmb() I don't see how this can help.

OTOH, I don't
understand why it is needed, we never change ->comm[TASK_COMM_LEN-1] == '0'.

Quoted 9 lines · Oleg Nesterov

fs/exec.c void set_task_comm(struct task_struct *tsk, char *buf) · line 959

Quoted 5 lines · Kees Cook

fs/exec.c +0 -0

@@ -956,7 +957,15 @@ void set_task_comm(struct task_struct *tsk, char *buf)
 	 */
 	memset(tsk->comm, 0, TASK_COMM_LEN);
 	wmb();

Off-topic. I'd wish I could understand this barrier. Since the lockless reader doesn't do rmb() I don't see how this can help.

This wmb() looks wrong to me as well. To achieve what the comment in this function says, it should be smp_wmb() and we should have smp_rmb() in the reading side, AFAIU.

Quoted 2 lines

OTOH, I don't
understand why it is needed, we never change ->comm[TASK_COMM_LEN-1] == '0'.

I think the idea was that readers can see incomplete names, but not messed up names, consisting of old and new ones.

Quoted 16 lines · Artem Bityutskiy

Quoted 9 lines · Oleg Nesterov

fs/exec.c void set_task_comm(struct task_struct *tsk, char *buf) · line 959

Quoted 5 lines · Kees Cook

fs/exec.c +0 -0

@@ -956,7 +957,15 @@ void set_task_comm(struct task_struct *tsk, char *buf)
 	 */
 	memset(tsk->comm, 0, TASK_COMM_LEN);
 	wmb();

Off-topic. I'd wish I could understand this barrier. Since the lockless reader doesn't do rmb() I don't see how this can help.

This wmb() looks wrong to me as well. To achieve what the comment in this function says, it should be smp_wmb() and we should have smp_rmb() in the reading side, AFAIU.

Quoted 5 lines

Quoted 2 lines

OTOH, I don't
understand why it is needed, we never change ->comm[TASK_COMM_LEN-1] == '0'.

I think the idea was that readers can see incomplete names, but not messed up names, consisting of old and new ones.

OK, agreed, comm[TASK_COMM_LEN-1] == '0' can't help to avoid the messed names.

But nether can help smp_rmb() in the reading (lockless) side?

Say,

	printk("comm=%s\n", current->comm);

if we add rmb() before printk, it can't make any difference. The lockless code should do something like

	get_comm_lockless(char *to, char *comm)
	{
		while (*comm++ = *to++)
			rmb();
	}

to ensure it sees the result of

	memset(tsk->comm, 0, TASK_COMM_LEN);
	wmb();
	strcpy(tsk->comm, buf);

in the right order.

otherwise printk("comm=%s\n", current->comm) can read, say, comm[1] before set_task_comm->memset(), and comm[0] after set_task_comm->strcpy().

So, afaics, set_task_comm()->wmb() buys nothing and should be removed. The last zero char in task_struct->comm[] is always here, at least this guarantees that strcpy(char *dest, tsk->comm) is always safe.

(I cc'ed the expert, Paul can correct me)

Quoted 4 lines · Oleg Nesterov

So, afaics, set_task_comm()->wmb() buys nothing and should be removed. The last zero char in task_struct->comm[] is always here, at least this guarantees that strcpy(char *dest, tsk->comm) is always safe.

Quoted 1 line

(I cc'ed the expert, Paul can correct me)

First, all of the implementations that I can see do task_lock(tsk), which should prevent readers from seeing any changes. So I am guessing that you guys want to allow readers to get at ->comm without having to acquire this lock.

[... snip a lot of good ideas ...]

Quoted 7 lines · Paul E. McKenney

Quoted 4 lines · Oleg Nesterov

So, afaics, set_task_comm()->wmb() buys nothing and should be removed. The last zero char in task_struct->comm[] is always here, at least this guarantees that strcpy(char *dest, tsk->comm) is always safe.

Quoted 6 lines

Quoted 1 line

(I cc'ed the expert, Paul can correct me)

First, all of the implementations that I can see do task_lock(tsk), which should prevent readers from seeing any changes. So I am guessing that you guys want to allow readers to get at ->comm without having to acquire this lock.

Ah, sorry for confusion.

No, we are not trying to invent the lockless get_task_comm(). I'd say it is not needed, if we really care about the precise ->comm we can take task->alloc_lock.

The only problem is that I believe that set_task_comm() wrongly pretends wmb() can help the lockless reader, it does:

	task_lock(tsk);

	/*
	 * Threads may access current->comm without holding
	 * the task lock, so write the string carefully.
	 * Readers without a lock may see incomplete new
	 * names but are safe from non-terminating string reads.
	 */
	memset(tsk->comm, 0, TASK_COMM_LEN);
	wmb();
	strlcpy(tsk->comm, buf, sizeof(tsk->comm));
	task_unlock(tsk);

but afaics this wmb() buys absolutely nothing if we race with the reader doing, say,

	printk("my name is %s\n", current->comm);

Afaics, this wmb()

- can't prevent from printing the mixture of the old/new data

- is not needed to make strcpy(somewhere, task->comm) safe, the final char is always '0', we never change it.

- adds the unnecessary confusion

Quoted 1 line

[... snip a lot of good ideas ...]

Thanks a lot, Paul ;)

Quoted 9 lines · Oleg Nesterov

fs/exec.c void set_task_comm(struct task_struct *tsk, char *buf) · line 959

Quoted 5 lines · Kees Cook

fs/exec.c +0 -0

@@ -956,7 +957,15 @@ void set_task_comm(struct task_struct *tsk, char *buf)
 	 */
 	memset(tsk->comm, 0, TASK_COMM_LEN);
 	wmb();

Off-topic. I'd wish I could understand this barrier. Since the lockless reader doesn't do rmb() I don't see how this can help.

This wmb() looks wrong to me as well. To achieve what the comment in this function says, it should be smp_wmb() and we should have smp_rmb() in the reading side, AFAIU.

Quoted 2 lines

OTOH, I don't
understand why it is needed, we never change ->comm[TASK_COMM_LEN-1] == '0'.

I think the idea was that readers can see incomplete names, but not messed up names, consisting of old and new ones.