Since my commit 4614a696bd1c3a9af3a08f0e5874830a85b889d4, the
current->comm value could be changed by other threads.
This changed the comm locking rules, which previously allowed for
unlocked current->comm access, since only the thread itself could
change its comm.
While this was brought up at the time, it was not considered
problematic, as the comm writing was done in such a way that
only null or incomplete comms could be read. However, recently
folks have made it clear they want to see this issue resolved.
So fair enough, as I opened this can of worms, I should work
to resolve it and this patchset is my initial attempt.
The proposed solution here is to introduce a new seqlock that
exclusively protects the comm value. We use it to serialize
access via get_task_comm() and set_task_comm(). Since some
comm access is open-coded using the task lock, we preserve
the task locking in set_task_comm for now. Once all comm
access is converted to using get_task_comm, we can clean that
up as well.
I've also introduced a printk %ptc accessor, which makes the
conversion to locked access simpler (as most uses are for printks).
And new in this version: I've added a checkpatch rule to try
to catch any new current->comm users from being introduced.
Although I suspect the script will need some additional work.
Hopefully this will allow for a smooth transition, where we can
slowly fix up the unlocked current->comm access bit by bit,
reducing the race window with each patch, while not making the
situation any worse then it was yesterday.
Thanks for the comments and feedback so far.
Any additional comments/feedback would still be appreciated.
thanks
-john
CC: Ted Ts'o <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: David Rientjes <[email protected]>
CC: Dave Hansen <[email protected]>
CC: Andrew Morton <[email protected]>
CC: [email protected]
John Stultz (3):
comm: Introduce comm_lock seqlock to protect task->comm access
printk: Add %ptc to safely print a task's comm
checkpatch.pl: Add check for current->comm references
fs/exec.c | 25 ++++++++++++++++++++-----
include/linux/init_task.h | 1 +
include/linux/sched.h | 5 ++---
lib/vsprintf.c | 27 +++++++++++++++++++++++++++
scripts/checkpatch.pl | 4 ++++
5 files changed, 54 insertions(+), 8 deletions(-)
--
1.7.3.2.146.gca209
The implicit rules for current->comm access being safe without locking
are no longer true. Accessing current->comm without holding the task
lock may result in null or incomplete strings (however, access won't
run off the end of the string).
In order to properly fix this, I've introduced a comm_lock seqlock
which will protect comm access and modified get_task_comm() and
set_task_comm() to use it.
Since there are a number of cases where comm access is open-coded
safely grabbing the task_lock(), we preserve the task locking in
set_task_comm, so those users are also safe.
With this patch, users that access current->comm without a lock
are still prone to null/incomplete comm strings, but it should
be no worse then it is now.
The next step is to go through and convert all comm accesses to
use get_task_comm(). This is substantial, but can be done bit by
bit, reducing the race windows with each patch.
CC: Ted Ts'o <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: David Rientjes <[email protected]>
CC: Dave Hansen <[email protected]>
CC: Andrew Morton <[email protected]>
CC: [email protected]
Acked-by: David Rientjes <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
fs/exec.c | 25 ++++++++++++++++++++-----
include/linux/init_task.h | 1 +
include/linux/sched.h | 5 ++---
3 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 5e62d26..fcd056a 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -998,18 +998,32 @@ static void flush_old_files(struct files_struct * files)
char *get_task_comm(char *buf, struct task_struct *tsk)
{
- /* buf must be at least sizeof(tsk->comm) in size */
- task_lock(tsk);
- strncpy(buf, tsk->comm, sizeof(tsk->comm));
- task_unlock(tsk);
+ unsigned long seq;
+
+ do {
+ seq = read_seqbegin(&tsk->comm_lock);
+
+ strncpy(buf, tsk->comm, sizeof(tsk->comm));
+
+ } while (read_seqretry(&tsk->comm_lock, seq));
+
return buf;
}
void set_task_comm(struct task_struct *tsk, char *buf)
{
- task_lock(tsk);
+ unsigned long flags;
/*
+ * XXX - Even though comm is protected by comm_lock,
+ * we take the task_lock here to serialize against
+ * current users that directly access comm.
+ * Once those users are removed, we can drop the
+ * task locking & memsetting.
+ */
+ task_lock(tsk);
+ write_seqlock_irqsave(&tsk->comm_lock, flags);
+ /*
* Threads may access current->comm without holding
* the task lock, so write the string carefully.
* Readers without a lock may see incomplete new
@@ -1018,6 +1032,7 @@ void set_task_comm(struct task_struct *tsk, char *buf)
memset(tsk->comm, 0, TASK_COMM_LEN);
wmb();
strlcpy(tsk->comm, buf, sizeof(tsk->comm));
+ write_sequnlock_irqrestore(&tsk->comm_lock, flags);
task_unlock(tsk);
perf_event_comm(tsk);
}
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index caa151f..b4f7584 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -161,6 +161,7 @@ extern struct cred init_cred;
.group_leader = &tsk, \
RCU_INIT_POINTER(.real_cred, &init_cred), \
RCU_INIT_POINTER(.cred, &init_cred), \
+ .comm_lock = SEQLOCK_UNLOCKED, \
.comm = "swapper", \
.thread = INIT_THREAD, \
.fs = &init_fs, \
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 18d63ce..f9324e4 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1333,10 +1333,9 @@ struct task_struct {
const struct cred __rcu *cred; /* effective (overridable) subjective task
* credentials (COW) */
struct cred *replacement_session_keyring; /* for KEYCTL_SESSION_TO_PARENT */
-
+ seqlock_t comm_lock; /* protect's comm */
char comm[TASK_COMM_LEN]; /* executable name excluding path
- - access with [gs]et_task_comm (which lock
- it with task_lock())
+ - access with [gs]et_task_comm
- initialized normally by setup_new_exec */
/* file system info */
int link_count, total_link_count;
--
1.7.3.2.146.gca209
Accessing task->comm requires proper locking. However in the past
access to current->comm could be done without locking. This
is no longer the case, so all comm access needs to be done
while holding the comm_lock.
In my attempt to clean up unprotected comm access, I've noticed
most comm access is done for printk output. To simplify correct
locking in these cases, I've introduced a new %ptc format,
which will print the corresponding task's comm.
Example use:
printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
CC: Ted Ts'o <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: David Rientjes <[email protected]>
CC: Dave Hansen <[email protected]>
CC: Andrew Morton <[email protected]>
CC: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
lib/vsprintf.c | 27 +++++++++++++++++++++++++++
1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index bc0ac6b..5abbe94 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -797,6 +797,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
return string(buf, end, uuid, spec);
}
+static noinline_for_stack
+char *task_comm_string(char *buf, char *end, void *addr,
+ struct printf_spec spec, const char *fmt)
+{
+ struct task_struct *tsk = addr;
+ char *ret;
+ unsigned long seq;
+
+ do {
+ seq = read_seqbegin(&tsk->comm_lock);
+
+ ret = string(buf, end, tsk->comm, spec);
+
+ } while (read_seqretry(&tsk->comm_lock, seq));
+
+ return ret;
+}
+
+
+
int kptr_restrict = 1;
/*
@@ -864,6 +884,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
}
switch (*fmt) {
+ case 't':
+ switch (fmt[1]) {
+ case 'c':
+ return task_comm_string(buf, end, ptr, spec, fmt);
+ }
+ break;
case 'F':
case 'f':
ptr = dereference_function_descriptor(ptr);
@@ -1151,6 +1177,7 @@ qualifier:
* http://tools.ietf.org/html/draft-ietf-6man-text-addr-representation-00
* %pU[bBlL] print a UUID/GUID in big or little endian using lower or upper
* case.
+ * %ptc outputs the task's comm name
* %n is ignored
*
* The return value is the number of characters which would
--
1.7.3.2.146.gca209
Now that accessing current->comm needs to be protected,
avoid new current->comm usage by adding a warning to
checkpatch.pl.
Fair warning: I know zero perl, so this was written in the
style of "monkey see, monkey do". It does appear to work
in my testing though.
Close review and feedback would be appreciated.
CC: Ted Ts'o <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: David Rientjes <[email protected]>
CC: Dave Hansen <[email protected]>
CC: Andrew Morton <[email protected]>
CC: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
scripts/checkpatch.pl | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d867081..9d2eab5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2868,6 +2868,10 @@ sub process {
WARN("usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc\n" . $herecurr);
}
+# check for current->comm usage
+ if ($line =~ /current->comm/) {
+ WARN("comm access needs to be protected. Use get_task_comm, or printk's \%ptc formatting.\n" . $herecurr);
+ }
# check for %L{u,d,i} in strings
my $string;
while ($line =~ /(?:^|")([X\t]*)(?:"|$)/g) {
--
1.7.3.2.146.gca209
On 05/13/2011 01:02 AM, John Stultz wrote:
> Now that accessing current->comm needs to be protected,
> avoid new current->comm usage by adding a warning to
> checkpatch.pl.
>
> Fair warning: I know zero perl, so this was written in the
> style of "monkey see, monkey do". It does appear to work
> in my testing though.
>
> Close review and feedback would be appreciated.
>
> CC: Ted Ts'o <[email protected]>
> CC: KOSAKI Motohiro <[email protected]>
> CC: David Rientjes <[email protected]>
> CC: Dave Hansen <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: [email protected]
> Signed-off-by: John Stultz <[email protected]>
> ---
> scripts/checkpatch.pl | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d867081..9d2eab5 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2868,6 +2868,10 @@ sub process {
> WARN("usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc\n" . $herecurr);
> }
>
> +# check for current->comm usage
> + if ($line =~ /current->comm/) {
This should be something like \b(current|task|tsk|t)->comm\b to catch
also non-current comm accesses...
> + WARN("comm access needs to be protected. Use get_task_comm, or printk's \%ptc formatting.\n" . $herecurr);
> + }
> # check for %L{u,d,i} in strings
> my $string;
> while ($line =~ /(?:^|")([X\t]*)(?:"|$)/g) {
--
js
> On 05/13/2011 01:02 AM, John Stultz wrote:
>> @@ -2868,6 +2868,10 @@ sub process {
>> WARN("usage of NR_CPUS is often wrong - consider using
>> cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc\n" .
>> $herecurr);
>> }
>>
>> +# check for current->comm usage
>> + if ($line =~ /current->comm/) {
On Fri, 13 May 2011 08:33:39 +0200, Jiri Slaby wrote:
> This should be something like \b(current|task|tsk|t)->comm\b to catch
> also non-current comm accesses...
Or \b(?:current|task|tsk|t)\s*->\s*comm\b.
>> + WARN("comm access needs to be protected. Use get_task_comm, or
>> printk's \%ptc formatting.\n" . $herecurr);
>> + }
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +-----<email/xmpp: [email protected]>-----ooO--(_)--Ooo--
Hi
Sorry for the long delay.
> char *get_task_comm(char *buf, struct task_struct *tsk)
> {
> - /* buf must be at least sizeof(tsk->comm) in size */
> - task_lock(tsk);
> - strncpy(buf, tsk->comm, sizeof(tsk->comm));
> - task_unlock(tsk);
> + unsigned long seq;
> +
> + do {
> + seq = read_seqbegin(&tsk->comm_lock);
> +
> + strncpy(buf, tsk->comm, sizeof(tsk->comm));
> +
> + } while (read_seqretry(&tsk->comm_lock, seq));
> +
> return buf;
> }
Can you please explain why we should use seqlock? That said,
we didn't use seqlock for /proc items. because, plenty seqlock
write may makes readers busy wait. Then, if we don't have another
protection, we give the local DoS attack way to attackers.
task->comm is used for very fundamentally. then, I doubt we can
assume write is enough rare. Why can't we use normal spinlock?
> scripts/checkpatch.pl | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d867081..9d2eab5 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2868,6 +2868,10 @@ sub process {
> WARN("usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc\n" . $herecurr);
> }
>
> +# check for current->comm usage
> + if ($line =~ /current->comm/) {
> + WARN("comm access needs to be protected. Use get_task_comm, or printk's \%ptc formatting.\n" . $herecurr);
I think we should convert all of task->comm usage. not only current. At least, you plan to remove task_lock() from
%ptc patch later.
thanks.
On Fri, 2011-05-13 at 20:13 +0900, KOSAKI Motohiro wrote:
> Hi
>
> Sorry for the long delay.
>
> > char *get_task_comm(char *buf, struct task_struct *tsk)
> > {
> > - /* buf must be at least sizeof(tsk->comm) in size */
> > - task_lock(tsk);
> > - strncpy(buf, tsk->comm, sizeof(tsk->comm));
> > - task_unlock(tsk);
> > + unsigned long seq;
> > +
> > + do {
> > + seq = read_seqbegin(&tsk->comm_lock);
> > +
> > + strncpy(buf, tsk->comm, sizeof(tsk->comm));
> > +
> > + } while (read_seqretry(&tsk->comm_lock, seq));
> > +
> > return buf;
> > }
>
> Can you please explain why we should use seqlock? That said,
> we didn't use seqlock for /proc items. because, plenty seqlock
> write may makes readers busy wait. Then, if we don't have another
> protection, we give the local DoS attack way to attackers.
So you're saying that heavy write contention can cause reader
starvation?
> task->comm is used for very fundamentally. then, I doubt we can
> assume write is enough rare. Why can't we use normal spinlock?
I think writes are likely to be fairly rare. Tasks can only name
themselves or sibling threads, so I'm not sure I see the risk here.
Mind going into more detail?
thanks
-john
On Fri, 2011-05-13 at 20:13 +0900, KOSAKI Motohiro wrote:
> > scripts/checkpatch.pl | 4 ++++
> > 1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index d867081..9d2eab5 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2868,6 +2868,10 @@ sub process {
> > WARN("usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc\n" . $herecurr);
> > }
> >
> > +# check for current->comm usage
> > + if ($line =~ /current->comm/) {
> > + WARN("comm access needs to be protected. Use get_task_comm, or printk's \%ptc formatting.\n" . $herecurr);
>
> I think we should convert all of task->comm usage. not only current. At least, you plan to remove task_lock() from
> %ptc patch later.
Yea, I'll be updating the patch to try to catch more then just
current->comm.
thanks
-john
>> Can you please explain why we should use seqlock? That said,
>> we didn't use seqlock for /proc items. because, plenty seqlock
>> write may makes readers busy wait. Then, if we don't have another
>> protection, we give the local DoS attack way to attackers.
>
> So you're saying that heavy write contention can cause reader
> starvation?
Yes.
>> task->comm is used for very fundamentally. then, I doubt we can
>> assume write is enough rare. Why can't we use normal spinlock?
>
> I think writes are likely to be fairly rare. Tasks can only name
> themselves or sibling threads, so I'm not sure I see the risk here.
reader starvation may cause another task's starvation if reader have
an another lock.
And, "only sibling" don't make any security gurantee as I said past.
Thanks.
On Sat, 2011-05-14 at 20:12 +0900, KOSAKI Motohiro wrote:
> >> Can you please explain why we should use seqlock? That said,
> >> we didn't use seqlock for /proc items. because, plenty seqlock
> >> write may makes readers busy wait. Then, if we don't have another
> >> protection, we give the local DoS attack way to attackers.
> >
> > So you're saying that heavy write contention can cause reader
> > starvation?
>
> Yes.
>
> >> task->comm is used for very fundamentally. then, I doubt we can
> >> assume write is enough rare. Why can't we use normal spinlock?
> >
> > I think writes are likely to be fairly rare. Tasks can only name
> > themselves or sibling threads, so I'm not sure I see the risk here.
>
> reader starvation may cause another task's starvation if reader have
> an another lock.
So the risk is a thread rewriting its own comm over and over could
starve some other critical task trying to read the comm.
Ok. It makes it a little more costly, but fair enough.
thanks
-john
On Fri, 13 May 2011, Michal Nazarewicz wrote:
> On Fri, 13 May 2011 08:33:39 +0200, Jiri Slaby wrote:
> > This should be something like \b(current|task|tsk|t)->comm\b to catch
> > also non-current comm accesses...
>
> Or \b(?:current|task|tsk|t)\s*->\s*comm\b.
>
Add 'p' to the mix :)
$ grep -r "struct task_struct \*p[; ]" * | wc -l
119