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

[Song Liu]

> On Feb 20, 2019, at 4:20 PM, Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
> 
> Hi Jann,
> 
> On Wed, 20 Feb 2019 17:54:43 +0100
> Jann Horn <jannh@xxxxxxxxxx> 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@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>

Thanks for the fix!

Acked-by: Song Liu <songliubraving@xxxxxx>


>> ---
>> 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@xxxxxxxxxx>