* [PATCH v2] perf/core: use strndup_user() instead of buggy open-coded version
@ 2019-02-20 16:54 Jann Horn
2019-02-21 0:20 ` Masami Hiramatsu
0 siblings, 1 reply; 4+ messages in thread
From: Jann Horn @ 2019-02-20 16:54 UTC (permalink / raw)
To: Ingo Molnar, Steven Rostedt, jannh
Cc: linux-kernel, Song Liu, Masami Hiramatsu
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] perf/core: use strndup_user() instead of buggy open-coded version
2019-02-20 16:54 [PATCH v2] perf/core: use strndup_user() instead of buggy open-coded version Jann Horn
@ 2019-02-21 0:20 ` Masami Hiramatsu
2019-02-21 7:12 ` Song Liu
0 siblings, 1 reply; 4+ messages in thread
From: Masami Hiramatsu @ 2019-02-21 0:20 UTC (permalink / raw)
To: Jann Horn
Cc: Ingo Molnar, Steven Rostedt, linux-kernel, Song Liu, 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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] perf/core: use strndup_user() instead of buggy open-coded version
2019-02-21 0:20 ` Masami Hiramatsu
@ 2019-02-21 7:12 ` Song Liu
2019-02-21 15:33 ` Steven Rostedt
0 siblings, 1 reply; 4+ messages in thread
From: Song Liu @ 2019-02-21 7:12 UTC (permalink / raw)
To: Masami Hiramatsu; +Cc: Jann Horn, Ingo Molnar, Steven Rostedt, linux-kernel
> 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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] perf/core: use strndup_user() instead of buggy open-coded version
2019-02-21 7:12 ` Song Liu
@ 2019-02-21 15:33 ` Steven Rostedt
0 siblings, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2019-02-21 15:33 UTC (permalink / raw)
To: Song Liu; +Cc: Masami Hiramatsu, Jann Horn, Ingo Molnar, linux-kernel
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-02-21 15:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-20 16:54 [PATCH v2] perf/core: use strndup_user() instead of buggy open-coded version Jann Horn
2019-02-21 0:20 ` Masami Hiramatsu
2019-02-21 7:12 ` Song Liu
2019-02-21 15:33 ` Steven Rostedt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.