| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <49439e9b-f738-46d9-ae47-14b45c135cd5@email.android.com>
Date: Thu, 04 Jul 2013 08:23:52 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
CC: Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Frederic Weisbecker <fweisbec@...il.com>,
Oleg Nesterov <oleg@...hat.com>, linux-kernel@...r.kernel.org,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
Andrew Morton <akpm@...ux-foundation.org>,
jovi.zhangwei@...wei.com, Jiri Olsa <jolsa@...hat.com>,
Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Subject: Re: [RFC PATCH] tracing: Atomically get refcounts of event_call and trace_array
Sorry for the top post. FYI today is a US holiday and tomorrow I'll probably only work a half day. I may not get a chance to look it till Monday.
-- Steve
Masami Hiramatsu <masami.hiramatsu.pt@...achi.com> wrote:
>Steven, Oleg,
>
>I think your patches are OK, but not enough.
>Here is an additional patch to fix the unsafe case which I found.
>Could you review this too?
>
>(2013/07/04 20:55), Masami Hiramatsu wrote:
>> Currently ftrace_open_generic_file gets an event_file from
>> inode->i_private, and then locks event_mutex and gets refcount.
>> However, this can cause a race as below scenario;
>>
>> CPU0 CPU1
>> open(kprobe_events)
>> trace_remove_event_call() open(enable)
>> lock event_mutex get event_file from inode->i_private
>> event_remove() wait for unlock event_mutex
>> ...
>> free event_file
>> unlock event_mutex
>> lock event_mutex
>> add refcount of event_file->call (*)
>>
>> So, at (*) point, the event_file is already freed and we
>> may access the corrupted object.
>> The same thing could happen on trace_array because it is also
>> directly accessed from event_file.
>>
>> To avoid this, when opening events/*/*/enable, we must atomically
>> do; ensure the ftrace_event_file object still exists on a
>trace_array,
>> and get refcounts of event_file->call and the trace_array.
>>
>>
>> CPU0 CPU1
>> open(kprobe_events)
>> trace_remove_event_call() open(enable)
>> lock event_mutex get event_file from inode->i_private
>> event_remove() wait for unlock event_mutex
>> ...
>> free event_file
>> unlock event_mutex
>> lock event_mutex
>> search the event_file and failed
>> unlock event_mutex
>> return -ENODEV
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
>> ---
>> kernel/trace/trace_events.c | 58
>+++++++++++++++++++++++++++++++++----------
>> 1 file changed, 45 insertions(+), 13 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events.c
>b/kernel/trace/trace_events.c
>> index 1a5547e..db6b107 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -391,15 +391,24 @@ static void __get_system_dir(struct
>ftrace_subsystem_dir *dir)
>> __get_system(dir->subsystem);
>> }
>>
>> -static int ftrace_event_call_get(struct ftrace_event_call *call)
>> +static int __ftrace_event_call_get(struct ftrace_event_call *call)
>> {
>> int ret = 0;
>>
>> - mutex_lock(&event_mutex);
>> if ((call->flags & TRACE_EVENT_FL_REF_MASK) ==
>TRACE_EVENT_FL_REF_MAX - 1)
>> ret = -EBUSY;
>> else
>> call->flags++;
>> +
>> + return ret;
>> +}
>> +
>> +static int ftrace_event_call_get(struct ftrace_event_call *call)
>> +{
>> + int ret = 0;
>> +
>> + mutex_lock(&event_mutex);
>> + ret = __ftrace_event_call_get(call);
>> mutex_unlock(&event_mutex);
>>
>> return ret;
>> @@ -413,6 +422,35 @@ static void ftrace_event_call_put(struct
>ftrace_event_call *call)
>> mutex_unlock(&event_mutex);
>> }
>>
>> +static int ftrace_event_file_get(struct ftrace_event_file
>*this_file)
>> +{
>> + struct ftrace_event_file *file;
>> + struct trace_array *tr;
>> + int ret = -ENODEV;
>> +
>> + mutex_lock(&event_mutex);
>> + do_for_each_event_file(tr, file) {
>> + if (file == this_file) {
>> + ret = __ftrace_event_call_get(file->event_call);
>> + if (!ret)
>> + tr->ref++;
>> + goto out_unlock;
>> + }
>> + } while_for_each_event_file();
>> + out_unlock:
>> + mutex_unlock(&event_mutex);
>> +
>> + return ret;
>> +}
>> +
>> +static void ftrace_event_file_put(struct ftrace_event_file *file)
>> +{
>> + struct trace_array *tr = file->tr;
>> +
>> + ftrace_event_call_put(file->event_call);
>> + trace_array_put(tr);
>> +}
>> +
>> static void __put_system_dir(struct ftrace_subsystem_dir *dir)
>> {
>> WARN_ON_ONCE(dir->ref_count == 0);
>> @@ -438,33 +476,27 @@ static void put_system(struct
>ftrace_subsystem_dir *dir)
>> static int tracing_open_generic_file(struct inode *inode, struct
>file *filp)
>> {
>> struct ftrace_event_file *file = inode->i_private;
>> - struct trace_array *tr = file->tr;
>> int ret;
>>
>> - if (trace_array_get(tr) < 0)
>> - return -ENODEV;
>> -
>> - ret = tracing_open_generic(inode, filp);
>> + ret = ftrace_event_file_get(file);
>> if (ret < 0)
>> - goto fail;
>> + return ret;
>>
>> - ret = ftrace_event_call_get(file->event_call);
>> + ret = tracing_open_generic(inode, filp);
>> if (ret < 0)
>> goto fail;
>>
>> return 0;
>> fail:
>> - trace_array_put(tr);
>> + ftrace_event_file_put(file);
>> return ret;
>> }
>>
>> static int tracing_release_generic_file(struct inode *inode, struct
>file *filp)
>> {
>> struct ftrace_event_file *file = inode->i_private;
>> - struct trace_array *tr = file->tr;
>>
>> - ftrace_event_call_put(file->event_call);
>> - trace_array_put(tr);
>> + ftrace_event_file_put(file);
>>
>> return 0;
>> }
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists