sanitize task->comm to avoid leaking escape codes
in [Kernel]
|
From: Kees Cook on 29 Jun 2010 11:20 On Tue, Jun 29, 2010 at 11:45:14AM +0300, Alexey Dobriyan wrote: > On Tue, Jun 29, 2010 at 12:03 AM, Kees Cook <kees.cook(a)canonical.com> wrote: > > On Mon, Jun 28, 2010 at 01:00:28PM -0700, Andrew Morton wrote: > >> 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. > > It also fills not just any buffer but buffer which is TASK_COMM_LEN byte wide. > > > 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). > If you pass too large, you waste space. Right. -Kees -- Kees Cook Ubuntu Security Team -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Paul E. McKenney on 29 Jun 2010 12:30 On Tue, Jun 29, 2010 at 03:31:31PM +0200, Oleg Nesterov wrote: > On 06/29, Artem Bityutskiy wrote: > > > > On Wed, 2010-06-23 at 21:41 +0200, Oleg Nesterov wrote: > > > On 06/23, Kees Cook wrote: > > > > > > > > @@ -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. > > > > > 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) 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. So if the reader uses no mutual exclusion, then that reader could be preempted/interrupted/NMIed/cache-missed/whatever for quite some time. The task's ->comm field might change once, twice, ten times in the meantime. The reader might then to so far as to get one character from each of the multiple names that happened in the meantime. So, how to fix? Here are some approaches that come to mind off-hand: 1. Use RCU (always my favorite, of course, but in this case...): The task structure adds a char* named ->commp that normally points to ->comm in the same task structure. The updater changes the name as follows: a. Allocate a chunk of memory to hold the new name. b. Copy the desired string into this newly allocated chunk of memory. c. rcu_assign_pointer() ->commp to point to this newly allocated chunk of memory. d. synchronize_rcu(). e. Now all readers are looking at the new name, so copy the new name into ->comm. f. rcu_assign_pointer() ->commp to point to ->comm. g. synchronize_rcu(). h. free up the newly assigned chunk of memory. Then the reader does: rcu_read_lock() p = rcu_dereference(tsk->commp); do_something_with(p); rcu_read_unlock(); This works, but is a bit complex, and adds not one but two grace periods to the updater. Yes, it is possible to optimize the second grace period away in the case of back-to-back updaters, but... 2. Use sequence locks: The updater just does the update under the sequence lock. The reader does the usual: do { seq = read_seqbegin(&tsk->commseqlock); do_something_with(tsk->comm); } while (read_seqretry(&tsk->commseqlock, seq)); Of course, this can starve readers, which can retaliate using something like the following: i = 0; do { seq = read_seqbegin(&tsk->commseqlock); do_something_with(tsk->comm); } while (read_seqretry(&tsk->commseqlock, seq) && ++i < 3); if (i >= 3) { write_seqlock(&tsk->commseqlock); do_something_with(tsk->comm); write_sequnlock(&tsk->commseqlock); } Readers might also need rcu_read_lock() to keep the task from disappearing out from under them: rcu_read_lock(); tsk = ... ... i = 0; do { seq = read_seqbegin(&tsk->commseqlock); do_something_with(tsk->comm); } while (read_seqretry(&tsk->commseqlock, seq) && ++i < 3); if (i >= 3) { write_seqlock(&tsk->commseqlock); do_something_with(tsk->comm); write_sequnlock(&tsk->commseqlock); } rcu_read_unlock(); But I have to defer to you guys on this one. For example, in the case where tsk==current, I do not believe that rcu_read_lock() is needed. 3. Various forms of reader-writer locks -- none of which seem to me to work very well in this case. 4. Your approach here! ;-) Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 29 Jun 2010 13:30 On 06/29, Paul E. McKenney wrote: > > On Tue, Jun 29, 2010 at 03:31:31PM +0200, Oleg Nesterov wrote: > > > 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) > > 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 > [... snip a lot of good ideas ...] Thanks a lot, Paul ;) Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Paul E. McKenney on 29 Jun 2010 13:40 On Tue, Jun 29, 2010 at 07:18:46PM +0200, Oleg Nesterov wrote: > On 06/29, Paul E. McKenney wrote: > > > > On Tue, Jun 29, 2010 at 03:31:31PM +0200, Oleg Nesterov wrote: > > > > > 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) > > > > 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 I agree -- I cannot see how this wmb() can help. > > [... snip a lot of good ideas ...] > > Thanks a lot, Paul ;) Glad you liked them. ;-) Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 29 Jun 2010 15:10
On Tue, 29 Jun 2010 08:09:52 -0700 Kees Cook <kees.cook(a)canonical.com> wrote: > On Tue, Jun 29, 2010 at 11:45:14AM +0300, Alexey Dobriyan wrote: > > On Tue, Jun 29, 2010 at 12:03 AM, Kees Cook <kees.cook(a)canonical.com> wrote: > > > On Mon, Jun 28, 2010 at 01:00:28PM -0700, Andrew Morton wrote: > > >> 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. > > > > It also fills not just any buffer but buffer which is TASK_COMM_LEN byte wide. > > > > > 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). 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?? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |