In case of errors, predicate_parse() goes to the out_free label
to free memory and to return an error code.
However, predicate_parse() does not free the predicates of the
temporary prog_stack array, thence leaking them.
Signed-off-by: Tomas Bortoli <[email protected]>
Reported-by: [email protected]
---
kernel/trace/trace_events_filter.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index d3e59312ef40..98eafad750d3 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -433,6 +433,8 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
parse_error(pe, -ENOMEM, 0);
goto out_free;
}
+ memset(prog_stack, 0, nr_preds * sizeof(*prog_stack));
+
inverts = kmalloc_array(nr_preds, sizeof(*inverts), GFP_KERNEL);
if (!inverts) {
parse_error(pe, -ENOMEM, 0);
@@ -579,6 +581,8 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
out_free:
kfree(op_stack);
kfree(inverts);
+ for (i = 0; prog_stack[i].pred; i++)
+ kfree(prog_stack[i].pred);
kfree(prog_stack);
return ERR_PTR(ret);
}
--
2.11.0
On Tue, 28 May 2019 15:46:59 +0200
Tomas Bortoli <[email protected]> wrote:
> In case of errors, predicate_parse() goes to the out_free label
> to free memory and to return an error code.
>
> However, predicate_parse() does not free the predicates of the
> temporary prog_stack array, thence leaking them.
>
>
> Signed-off-by: Tomas Bortoli <[email protected]>
> Reported-by: [email protected]
> ---
> kernel/trace/trace_events_filter.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index d3e59312ef40..98eafad750d3 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -433,6 +433,8 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
> parse_error(pe, -ENOMEM, 0);
> goto out_free;
> }
> + memset(prog_stack, 0, nr_preds * sizeof(*prog_stack));
> +
Can you instead just switch the allocation of prog_stack to use
kcalloc()?
Thanks,
-- Steve
> inverts = kmalloc_array(nr_preds, sizeof(*inverts), GFP_KERNEL);
> if (!inverts) {
> parse_error(pe, -ENOMEM, 0);
> @@ -579,6 +581,8 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
> out_free:
> kfree(op_stack);
> kfree(inverts);
> + for (i = 0; prog_stack[i].pred; i++)
> + kfree(prog_stack[i].pred);
> kfree(prog_stack);
> return ERR_PTR(ret);
> }
On 5/28/19 4:44 PM, Steven Rostedt wrote:
> On Tue, 28 May 2019 15:46:59 +0200
> Tomas Bortoli <[email protected]> wrote:
>
>> In case of errors, predicate_parse() goes to the out_free label
>> to free memory and to return an error code.
>>
>> However, predicate_parse() does not free the predicates of the
>> temporary prog_stack array, thence leaking them.
>>
>>
>> Signed-off-by: Tomas Bortoli <[email protected]>
>> Reported-by: [email protected]
>> ---
>> kernel/trace/trace_events_filter.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
>> index d3e59312ef40..98eafad750d3 100644
>> --- a/kernel/trace/trace_events_filter.c
>> +++ b/kernel/trace/trace_events_filter.c
>> @@ -433,6 +433,8 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
>> parse_error(pe, -ENOMEM, 0);
>> goto out_free;
>> }
>> + memset(prog_stack, 0, nr_preds * sizeof(*prog_stack));
>> +
>
> Can you instead just switch the allocation of prog_stack to use
> kcalloc()?
kmalloc_array() is safe against arithmetic overflow of the arguments.
Using kcalloc() directly we wouldn't check for that. Not really ideal in
my opinion. And there's no kcalloc_array() apparently!
Cheers,
Tomas
On 5/28/19 5:29 PM, Steven Rostedt wrote:
> On Tue, 28 May 2019 17:18:59 +0200
> Tomas Bortoli <[email protected]> wrote:
>
>>>> + memset(prog_stack, 0, nr_preds * sizeof(*prog_stack));
>>>> +
>>>
>>> Can you instead just switch the allocation of prog_stack to use
>>> kcalloc()?
>>
>> kmalloc_array() is safe against arithmetic overflow of the arguments.
>> Using kcalloc() directly we wouldn't check for that. Not really ideal in
>> my opinion. And there's no kcalloc_array() apparently!
>
> But doesn't kcalloc() simply call kmalloc_array() with the GFP_ZERO
> flag?
>
It does! Oops, I'll send it shortly
Tomas
On Tue, 28 May 2019 17:18:59 +0200
Tomas Bortoli <[email protected]> wrote:
> >> + memset(prog_stack, 0, nr_preds * sizeof(*prog_stack));
> >> +
> >
> > Can you instead just switch the allocation of prog_stack to use
> > kcalloc()?
>
> kmalloc_array() is safe against arithmetic overflow of the arguments.
> Using kcalloc() directly we wouldn't check for that. Not really ideal in
> my opinion. And there's no kcalloc_array() apparently!
But doesn't kcalloc() simply call kmalloc_array() with the GFP_ZERO
flag?
-- Steve
In case of errors, predicate_parse() goes to the out_free label
to free memory and to return an error code.
However, predicate_parse() does not free the predicates of the
temporary prog_stack array, thence leaking them.
Signed-off-by: Tomas Bortoli <[email protected]>
Reported-by: [email protected]
---
kernel/trace/trace_events_filter.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 05a66493a164..ecfa6f0f1c7e 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -427,7 +427,7 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
op_stack = kmalloc_array(nr_parens, sizeof(*op_stack), GFP_KERNEL);
if (!op_stack)
return ERR_PTR(-ENOMEM);
- prog_stack = kmalloc_array(nr_preds, sizeof(*prog_stack), GFP_KERNEL);
+ prog_stack = kcalloc(nr_preds, sizeof(*prog_stack), GFP_KERNEL);
if (!prog_stack) {
parse_error(pe, -ENOMEM, 0);
goto out_free;
@@ -578,6 +578,8 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
out_free:
kfree(op_stack);
kfree(inverts);
+ for (i = 0; prog_stack[i].pred; i++)
+ kfree(prog_stack[i].pred);
kfree(prog_stack);
return ERR_PTR(ret);
}
--
2.11.0
On Tue, 28 May 2019 17:43:38 +0200
Tomas Bortoli <[email protected]> wrote:
> In case of errors, predicate_parse() goes to the out_free label
> to free memory and to return an error code.
>
> However, predicate_parse() does not free the predicates of the
> temporary prog_stack array, thence leaking them.
Thanks, I applied this and I'm running it through my tests. But just an
FYI, when sending updated patches please add a "v2" to the subject:
[PATCH v2] tracing: Avoid memory leak in predicate_parse()
That way struggling maintainers like myself don't get confused about
which patch to apply ;-)
Thanks!
-- Steve
>
> Signed-off-by: Tomas Bortoli <[email protected]>
> Reported-by: [email protected]
> ---
> kernel/trace/trace_events_filter.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 05a66493a164..ecfa6f0f1c7e 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -427,7 +427,7 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
> op_stack = kmalloc_array(nr_parens, sizeof(*op_stack), GFP_KERNEL);
> if (!op_stack)
> return ERR_PTR(-ENOMEM);
> - prog_stack = kmalloc_array(nr_preds, sizeof(*prog_stack), GFP_KERNEL);
> + prog_stack = kcalloc(nr_preds, sizeof(*prog_stack), GFP_KERNEL);
> if (!prog_stack) {
> parse_error(pe, -ENOMEM, 0);
> goto out_free;
> @@ -578,6 +578,8 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
> out_free:
> kfree(op_stack);
> kfree(inverts);
> + for (i = 0; prog_stack[i].pred; i++)
> + kfree(prog_stack[i].pred);
> kfree(prog_stack);
> return ERR_PTR(ret);
> }
On 5/28/19 5:48 PM, Steven Rostedt wrote:
> On Tue, 28 May 2019 17:43:38 +0200
> Tomas Bortoli <[email protected]> wrote:
>
>> In case of errors, predicate_parse() goes to the out_free label
>> to free memory and to return an error code.
>>
>> However, predicate_parse() does not free the predicates of the
>> temporary prog_stack array, thence leaking them.
>
> Thanks, I applied this and I'm running it through my tests. But just an
> FYI, when sending updated patches please add a "v2" to the subject:
>
> [PATCH v2] tracing: Avoid memory leak in predicate_parse()
>
> That way struggling maintainers like myself don't get confused about
> which patch to apply ;-)
>
> Thanks!
>
Yeah, sorry about that, will make sure it doesn't happen again!
Thank you,
Tomas
>
>>
>> Signed-off-by: Tomas Bortoli <[email protected]>
>> Reported-by: [email protected]
>> ---
>> kernel/trace/trace_events_filter.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
>> index 05a66493a164..ecfa6f0f1c7e 100644
>> --- a/kernel/trace/trace_events_filter.c
>> +++ b/kernel/trace/trace_events_filter.c
>> @@ -427,7 +427,7 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
>> op_stack = kmalloc_array(nr_parens, sizeof(*op_stack), GFP_KERNEL);
>> if (!op_stack)
>> return ERR_PTR(-ENOMEM);
>> - prog_stack = kmalloc_array(nr_preds, sizeof(*prog_stack), GFP_KERNEL);
>> + prog_stack = kcalloc(nr_preds, sizeof(*prog_stack), GFP_KERNEL);
>> if (!prog_stack) {
>> parse_error(pe, -ENOMEM, 0);
>> goto out_free;
>> @@ -578,6 +578,8 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
>> out_free:
>> kfree(op_stack);
>> kfree(inverts);
>> + for (i = 0; prog_stack[i].pred; i++)
>> + kfree(prog_stack[i].pred);
>> kfree(prog_stack);
>> return ERR_PTR(ret);
>> }
>
On 5/28/19 10:31 PM, Steven Rostedt wrote:
> On Tue, 28 May 2019 17:43:38 +0200
> Tomas Bortoli <[email protected]> wrote:
>
>> @@ -578,6 +578,8 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
>> out_free:
>> kfree(op_stack);
>> kfree(inverts);
>> + for (i = 0; prog_stack[i].pred; i++)
>> + kfree(prog_stack[i].pred);
>> kfree(prog_stack);
>> return ERR_PTR(ret);
>> }
>
> I should have caught this, but thanks to the zero day bot, it found it
> first:
>
> kernel/trace/trace_events_filter.c:582:27-31: ERROR: prog_stack is NULL but dereferenced.
>
> I changed the patch with the following:
>
> From dfb4a6f2191a80c8b790117d0ff592fd712d3296 Mon Sep 17 00:00:00 2001
> From: Tomas Bortoli <[email protected]>
> Date: Tue, 28 May 2019 17:43:38 +0200
> Subject: [PATCH] tracing: Avoid memory leak in predicate_parse()
>
> In case of errors, predicate_parse() goes to the out_free label
> to free memory and to return an error code.
>
> However, predicate_parse() does not free the predicates of the
> temporary prog_stack array, thence leaking them.
>
> Link: http://lkml.kernel.org/r/[email protected]
>
> Cc: [email protected]
> Fixes: 80765597bc587 ("tracing: Rewrite filter logic to be simpler and faster")
> Reported-by: [email protected]
> Signed-off-by: Tomas Bortoli <[email protected]>
> [ Added protection around freeing prog_stack[i].pred ]
> Signed-off-by: Steven Rostedt (VMware) <[email protected]>
> ---
> kernel/trace/trace_events_filter.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index d3e59312ef40..5079d1db3754 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -428,7 +428,7 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
> op_stack = kmalloc_array(nr_parens, sizeof(*op_stack), GFP_KERNEL);
> if (!op_stack)
> return ERR_PTR(-ENOMEM);
> - prog_stack = kmalloc_array(nr_preds, sizeof(*prog_stack), GFP_KERNEL);
> + prog_stack = kcalloc(nr_preds, sizeof(*prog_stack), GFP_KERNEL);
> if (!prog_stack) {
> parse_error(pe, -ENOMEM, 0);
> goto out_free;
> @@ -579,7 +579,11 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
> out_free:
> kfree(op_stack);
> kfree(inverts);
> - kfree(prog_stack);
> + if (prog_stack) {
> + for (i = 0; prog_stack[i].pred; i++)
> + kfree(prog_stack[i].pred);
> + kfree(prog_stack);
> + }
> return ERR_PTR(ret);
> }
>
>
Oops again, I should have been more careful.
Thanks.
On Tue, 28 May 2019 17:43:38 +0200
Tomas Bortoli <[email protected]> wrote:
> @@ -578,6 +578,8 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
> out_free:
> kfree(op_stack);
> kfree(inverts);
> + for (i = 0; prog_stack[i].pred; i++)
> + kfree(prog_stack[i].pred);
> kfree(prog_stack);
> return ERR_PTR(ret);
> }
I should have caught this, but thanks to the zero day bot, it found it
first:
kernel/trace/trace_events_filter.c:582:27-31: ERROR: prog_stack is NULL but dereferenced.
I changed the patch with the following:
From dfb4a6f2191a80c8b790117d0ff592fd712d3296 Mon Sep 17 00:00:00 2001
From: Tomas Bortoli <[email protected]>
Date: Tue, 28 May 2019 17:43:38 +0200
Subject: [PATCH] tracing: Avoid memory leak in predicate_parse()
In case of errors, predicate_parse() goes to the out_free label
to free memory and to return an error code.
However, predicate_parse() does not free the predicates of the
temporary prog_stack array, thence leaking them.
Link: http://lkml.kernel.org/r/[email protected]
Cc: [email protected]
Fixes: 80765597bc587 ("tracing: Rewrite filter logic to be simpler and faster")
Reported-by: [email protected]
Signed-off-by: Tomas Bortoli <[email protected]>
[ Added protection around freeing prog_stack[i].pred ]
Signed-off-by: Steven Rostedt (VMware) <[email protected]>
---
kernel/trace/trace_events_filter.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index d3e59312ef40..5079d1db3754 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -428,7 +428,7 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
op_stack = kmalloc_array(nr_parens, sizeof(*op_stack), GFP_KERNEL);
if (!op_stack)
return ERR_PTR(-ENOMEM);
- prog_stack = kmalloc_array(nr_preds, sizeof(*prog_stack), GFP_KERNEL);
+ prog_stack = kcalloc(nr_preds, sizeof(*prog_stack), GFP_KERNEL);
if (!prog_stack) {
parse_error(pe, -ENOMEM, 0);
goto out_free;
@@ -579,7 +579,11 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
out_free:
kfree(op_stack);
kfree(inverts);
- kfree(prog_stack);
+ if (prog_stack) {
+ for (i = 0; prog_stack[i].pred; i++)
+ kfree(prog_stack[i].pred);
+ kfree(prog_stack);
+ }
return ERR_PTR(ret);
}
--
2.20.1