Messages in this thread | | | From | Kalesh Singh <> | Date | Tue, 16 Nov 2021 20:37:12 -0800 | Subject | Re: [PATCH] tracing/histogram: Fix UAF in destroy_hist_field() |
| |
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
| |