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

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

[Jann Horn]

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.

Fixes: 0eadcc7a7bc0 ("perf/core: Fix perf_uprobe_init()")
Signed-off-by: Jann Horn <jannh@google.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

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

[Masami Hiramatsu]

Hi Jann,

On Wed, 20 Feb 2019 17:54:43 +0100
Jann Horn <jannh@google.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@kernel.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@google.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@kernel.org>

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@kernel.org> wrote:
> 
> Hi Jann,
> 
> On Wed, 20 Feb 2019 17:54:43 +0100
> Jann Horn <jannh@google.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@kernel.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@google.com>

Thanks for the fix!

Acked-by: Song Liu <songliubraving@fb.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@kernel.org>

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

[Steven Rostedt]

On Thu, 21 Feb 2019 07:12:10 +0000
Song Liu <songliubraving@fb.com> wrote:

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

OK, I'll add a stable tag (but it wont go until the merge window).

It also shouldn't be called "perf/core" as its in the tracing
directory, and thus "tracing/perf" would be more appropriate. But I see
that's what the patch it fixes calls it too :-/

I'll pull it in.

Thanks everyone!

-- Steve


> > 
> > Thank you,
> >   
> >> Fixes: 0eadcc7a7bc0 ("perf/core: Fix perf_uprobe_init()")
> >> Signed-off-by: Jann Horn <jannh@google.com>  
> 
> Thanks for the fix!
> 
> Acked-by: Song Liu <songliubraving@fb.com>