linux-kernel - Re: [PATCH v2] perf/core: use strndup_user() instead of buggy open-coded version

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7F433C6B-451A-4C3B-9B7E-0CB2CE4821DD@fb.com>
Date:   Thu, 21 Feb 2019 07:12:10 +0000
From:   Song Liu <songliubraving@...com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
CC:     Jann Horn <jannh@...gle.com>, Ingo Molnar <mingo@...nel.org>,
        "Steven Rostedt" <rostedt@...dmis.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] perf/core: use strndup_user() instead of buggy
 open-coded version



> On Feb 20, 2019, at 4:20 PM, Masami Hiramatsu <mhiramat@...nel.org> wrote:
> 
> Hi Jann,
> 
> On Wed, 20 Feb 2019 17:54:43 +0100
> Jann Horn <jannh@...gle.com> wrote:
> 
>> The first version of this method was missing the check for
>> `ret == PATH_MAX`; then such a check was added, but it didn't call kfree()
>> on error, so there was still a small memory leak in the error case.
>> Fix it by using strndup_user() instead of open-coding it.
>> 
> 
> This looks good to me.
> 
> Reviewed-by: Masami Hiramatsu <mhiramat@...nel.org>
> 
> BTW, for stable, this is good. For the long term, I think we should
> fix strndup_user() to return -E2BUG when the user string is longer
> than max.
> 
> Thank you,
> 
>> Fixes: 0eadcc7a7bc0 ("perf/core: Fix perf_uprobe_init()")
>> Signed-off-by: Jann Horn <jannh@...gle.com>

Thanks for the fix!

Acked-by: Song Liu <songliubraving@...com>


>> ---
>> v2:
>> - be compatible with existing error codes (Masami Hiramatsu)
>> 
>> kernel/trace/trace_event_perf.c | 16 +++++++---------
>> 1 file changed, 7 insertions(+), 9 deletions(-)
>> 
>> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
>> index 76217bbef815..4629a6104474 100644
>> --- a/kernel/trace/trace_event_perf.c
>> +++ b/kernel/trace/trace_event_perf.c
>> @@ -299,15 +299,13 @@ int perf_uprobe_init(struct perf_event *p_event,
>> 
>> 	if (!p_event->attr.uprobe_path)
>> 		return -EINVAL;
>> -	path = kzalloc(PATH_MAX, GFP_KERNEL);
>> -	if (!path)
>> -		return -ENOMEM;
>> -	ret = strncpy_from_user(
>> -		path, u64_to_user_ptr(p_event->attr.uprobe_path), PATH_MAX);
>> -	if (ret == PATH_MAX)
>> -		return -E2BIG;
>> -	if (ret < 0)
>> -		goto out;
>> +
>> +	path = strndup_user(u64_to_user_ptr(p_event->attr.uprobe_path),
>> +			    PATH_MAX);
>> +	if (IS_ERR(path)) {
>> +		ret = PTR_ERR(path);
>> +		return (ret == -EINVAL) ? -E2BIG : ret;
>> +	}
>> 	if (path[0] == '\0') {
>> 		ret = -EINVAL;
>> 		goto out;
>> -- 
>> 2.21.0.rc0.258.g878e2cd30e-goog
>> 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@...nel.org>