* [PATCH] tracing/histogram: Fix UAF in destroy_hist_field()
@ 2021-11-17 2:12 Kalesh Singh
2021-11-17 3:47 ` Steven Rostedt
0 siblings, 1 reply; 3+ messages in thread
From: Kalesh Singh @ 2021-11-17 2:12 UTC (permalink / raw)
Cc: kernel-team, rostedt, mhiramat, zanussi, Kalesh Singh,
kernel test robot, Ingo Molnar, linux-kernel
Calling destroy_hist_field() on an expression will recursively free
any operands associated with the expression. If during expression
parsing the operands of the expression are already set when an error
is encountered, there is no need to explicity free the operands. Doing
so will result in destroy_hist_field() being called twice for the
operands and lead to a use-after-free (UAF) error.
Fix this by only calling destroy_hist_field() for the expression if the
operands are already set.
Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
Fixes: 8b5d46fd7a38 ("tracing/histogram: Optimize division by constants")
Reported-by: kernel test robot <oliver.sang@intel.com>
---
kernel/trace/trace_events_hist.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 5ea2c9ec54a6..e3856eaf2ac3 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -2669,7 +2669,7 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
if (!divisor) {
hist_err(file->tr, HIST_ERR_DIVISION_BY_ZERO, errpos(str));
ret = -EDOM;
- goto free;
+ goto free_expr;
}
/*
@@ -2709,7 +2709,7 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
expr->type = kstrdup_const(operand1->type, GFP_KERNEL);
if (!expr->type) {
ret = -ENOMEM;
- goto free;
+ goto free_expr;
}
expr->name = expr_str(expr, 0);
@@ -2719,6 +2719,7 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
free:
destroy_hist_field(operand1, 0);
destroy_hist_field(operand2, 0);
+free_expr:
destroy_hist_field(expr, 0);
return ERR_PTR(ret);
base-commit: 8ab774587903771821b59471cc723bba6d893942
--
2.34.0.rc1.387.gb447b232ab-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] tracing/histogram: Fix UAF in destroy_hist_field()
2021-11-17 2:12 [PATCH] tracing/histogram: Fix UAF in destroy_hist_field() Kalesh Singh
@ 2021-11-17 3:47 ` Steven Rostedt
2021-11-17 4:37 ` Kalesh Singh
0 siblings, 1 reply; 3+ messages in thread
From: Steven Rostedt @ 2021-11-17 3:47 UTC (permalink / raw)
To: Kalesh Singh
Cc: kernel-team, mhiramat, zanussi, kernel test robot, Ingo Molnar,
linux-kernel
On Tue, 16 Nov 2021 18:12:22 -0800
Kalesh Singh <kaleshsingh@google.com> wrote:
> Calling destroy_hist_field() on an expression will recursively free
> any operands associated with the expression. If during expression
> parsing the operands of the expression are already set when an error
> is encountered, there is no need to explicity free the operands. Doing
> so will result in destroy_hist_field() being called twice for the
> operands and lead to a use-after-free (UAF) error.
>
> Fix this by only calling destroy_hist_field() for the expression if the
> operands are already set.
>
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> Fixes: 8b5d46fd7a38 ("tracing/histogram: Optimize division by constants")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> ---
> kernel/trace/trace_events_hist.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 5ea2c9ec54a6..e3856eaf2ac3 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -2669,7 +2669,7 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
> if (!divisor) {
> hist_err(file->tr, HIST_ERR_DIVISION_BY_ZERO, errpos(str));
> ret = -EDOM;
> - goto free;
> + goto free_expr;
> }
>
> /*
> @@ -2709,7 +2709,7 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
> expr->type = kstrdup_const(operand1->type, GFP_KERNEL);
> if (!expr->type) {
> ret = -ENOMEM;
> - goto free;
> + goto free_expr;
> }
>
> expr->name = expr_str(expr, 0);
> @@ -2719,6 +2719,7 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
> free:
> destroy_hist_field(operand1, 0);
> destroy_hist_field(operand2, 0);
> +free_expr:
> destroy_hist_field(expr, 0);
>
> return ERR_PTR(ret);
>
> base-commit: 8ab774587903771821b59471cc723bba6d893942
Wouldn't this be a simpler and more robust fix?
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 5ea2c9ec54a6..aab69b4ffe11 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -2717,8 +2717,10 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
return expr;
free:
- destroy_hist_field(operand1, 0);
- destroy_hist_field(operand2, 0);
+ if (!expr || expr->operand[0] != operand1)
+ destroy_hist_field(operand1, 0);
+ if (!expr || expr->operand[1] != operand2)
+ destroy_hist_field(operand2, 0);
destroy_hist_field(expr, 0);
return ERR_PTR(ret);
I'm worried about the complexity of having to know where to free what,
and not just figuring it out at the end.
-- Steve
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] tracing/histogram: Fix UAF in destroy_hist_field()
2021-11-17 3:47 ` Steven Rostedt
@ 2021-11-17 4:37 ` Kalesh Singh
0 siblings, 0 replies; 3+ messages in thread
From: Kalesh Singh @ 2021-11-17 4:37 UTC (permalink / raw)
To: Steven Rostedt
Cc: kernel-team, mhiramat, zanussi, kernel test robot, Ingo Molnar,
linux-kernel
On Tue, Nov 16, 2021 at 7:47 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 16 Nov 2021 18:12:22 -0800
> Kalesh Singh <kaleshsingh@google.com> wrote:
>
> > Calling destroy_hist_field() on an expression will recursively free
> > any operands associated with the expression. If during expression
> > parsing the operands of the expression are already set when an error
> > is encountered, there is no need to explicity free the operands. Doing
> > so will result in destroy_hist_field() being called twice for the
> > operands and lead to a use-after-free (UAF) error.
> >
> > Fix this by only calling destroy_hist_field() for the expression if the
> > operands are already set.
> >
> > Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> > Fixes: 8b5d46fd7a38 ("tracing/histogram: Optimize division by constants")
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > ---
> > kernel/trace/trace_events_hist.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > index 5ea2c9ec54a6..e3856eaf2ac3 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -2669,7 +2669,7 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
> > if (!divisor) {
> > hist_err(file->tr, HIST_ERR_DIVISION_BY_ZERO, errpos(str));
> > ret = -EDOM;
> > - goto free;
> > + goto free_expr;
> > }
> >
> > /*
> > @@ -2709,7 +2709,7 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
> > expr->type = kstrdup_const(operand1->type, GFP_KERNEL);
> > if (!expr->type) {
> > ret = -ENOMEM;
> > - goto free;
> > + goto free_expr;
> > }
> >
> > expr->name = expr_str(expr, 0);
> > @@ -2719,6 +2719,7 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
> > free:
> > destroy_hist_field(operand1, 0);
> > destroy_hist_field(operand2, 0);
> > +free_expr:
> > destroy_hist_field(expr, 0);
> >
> > return ERR_PTR(ret);
> >
> > base-commit: 8ab774587903771821b59471cc723bba6d893942
>
> Wouldn't this be a simpler and more robust fix?
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 5ea2c9ec54a6..aab69b4ffe11 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -2717,8 +2717,10 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
>
> return expr;
> free:
> - destroy_hist_field(operand1, 0);
> - destroy_hist_field(operand2, 0);
> + if (!expr || expr->operand[0] != operand1)
> + destroy_hist_field(operand1, 0);
> + if (!expr || expr->operand[1] != operand2)
> + destroy_hist_field(operand2, 0);
> destroy_hist_field(expr, 0);
Hi Steve,
Agreed. What you suggested is simpler to work with. I'll respin a new version.
Thanks,
Kalesh
>
> return ERR_PTR(ret);
>
>
> I'm worried about the complexity of having to know where to free what,
> and not just figuring it out at the end.
>
> -- Steve
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-11-17 4:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-17 2:12 [PATCH] tracing/histogram: Fix UAF in destroy_hist_field() Kalesh Singh
2021-11-17 3:47 ` Steven Rostedt
2021-11-17 4:37 ` Kalesh Singh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).