[GIT PULL] tracing: Fix double free bug

[GIT PULL] tracing: Fix double free bug

[Steven Rostedt]

Linus,

Fix double free bug in tracing histograms

On error, the operands and the histogram expression are destroyed,
but since the destruction is recursive, do not destroy the operands
if they already belong to the expression that is about to be destroyed.


Please pull the latest trace-v5.16-rc1 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-v5.16-rc1

Tag SHA1: a34f97bfa65691c91eed55ce5f09cfaa78e05908
Head SHA1: b0cb20110d4562bcc39f49f2de4020f0caa84bdd


Kalesh Singh (1):
      tracing/histogram: Fix UAF in destroy_hist_field()

----
 kernel/trace/trace_events_hist.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
---------------------------
commit b0cb20110d4562bcc39f49f2de4020f0caa84bdd
Author: Kalesh Singh <kaleshsingh@google.com>
Date:   Tue Nov 16 23:34:14 2021 -0800

    tracing/histogram: Fix UAF in destroy_hist_field()
    
    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 operands if they
    are not associated with the expression hist_field.
    
    Link: https://lkml.kernel.org/r/20211117073415.2584751-1-kaleshsingh@google.com
    
    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>
    Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 5ea2c9ec54a6..b53ee8d566f6 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->operands[0] != operand1)
+		destroy_hist_field(operand1, 0);
+	if (!expr || expr->operands[1] != operand2)
+		destroy_hist_field(operand2, 0);
 	destroy_hist_field(expr, 0);
 
 	return ERR_PTR(ret);

Re: [GIT PULL] tracing: Fix double free bug

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

On Wed, Nov 17, 2021 at 3:19 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On error, the operands and the histogram expression are destroyed,
> but since the destruction is recursive, do not destroy the operands
> if they already belong to the expression that is about to be destroyed.

Honestly, this seems horribly ugly.

The problem seems to be that the "goto error" cases are simply just wrong.

Why isn't the fix to make the error cases be the right ones, instead
of having one odd error case that then has to do some magic things to
not free the wrong things?

The patch ends up a bit bigger, mainly because I renamed the different
"free" cases, and because I made the non-freeing ones just return the
error directly.

Something like this (UNTESTED!) patch, IOW?

          Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 3339 bytes --]

 kernel/trace/trace_events_hist.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 5ea2c9ec54a6..42ee3e95deb7 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -2576,28 +2576,27 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
 
 	/* Split the expression string at the root operator */
 	if (!sep)
-		goto free;
+		return ERR_PTR(-EINVAL);
+
 	*sep = '\0';
 	operand1_str = str;
 	str = sep+1;
 
 	/* Binary operator requires both operands */
 	if (*operand1_str == '\0' || *str == '\0')
-		goto free;
+		return ERR_PTR(-EINVAL);
 
 	operand_flags = 0;
 
 	/* LHS of string is an expression e.g. a+b in a+b+c */
 	operand1 = parse_expr(hist_data, file, operand1_str, operand_flags, NULL, n_subexprs);
-	if (IS_ERR(operand1)) {
-		ret = PTR_ERR(operand1);
-		operand1 = NULL;
-		goto free;
-	}
+	if (IS_ERR(operand1))
+		return ERR_CAST(operand1);
+
 	if (operand1->flags & HIST_FIELD_FL_STRING) {
 		hist_err(file->tr, HIST_ERR_INVALID_STR_OPERAND, errpos(operand1_str));
 		ret = -EINVAL;
-		goto free;
+		goto free_op1;
 	}
 
 	/* RHS of string is another expression e.g. c in a+b+c */
@@ -2606,12 +2605,12 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
 	if (IS_ERR(operand2)) {
 		ret = PTR_ERR(operand2);
 		operand2 = NULL;
-		goto free;
+		goto free_op1;
 	}
 	if (operand2->flags & HIST_FIELD_FL_STRING) {
 		hist_err(file->tr, HIST_ERR_INVALID_STR_OPERAND, errpos(str));
 		ret = -EINVAL;
-		goto free;
+		goto free_operands;
 	}
 
 	switch (field_op) {
@@ -2629,12 +2628,12 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
 		break;
 	default:
 		ret = -EINVAL;
-		goto free;
+		goto free_operands;
 	}
 
 	ret = check_expr_operands(file->tr, operand1, operand2, &var1, &var2);
 	if (ret)
-		goto free;
+		goto free_operands;
 
 	operand_flags = var1 ? var1->flags : operand1->flags;
 	operand2_flags = var2 ? var2->flags : operand2->flags;
@@ -2653,12 +2652,13 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
 	expr = create_hist_field(hist_data, NULL, flags, var_name);
 	if (!expr) {
 		ret = -ENOMEM;
-		goto free;
+		goto free_operands;
 	}
 
 	operand1->read_once = true;
 	operand2->read_once = true;
 
+	/* The operands are now owned and free'd by 'expr' */
 	expr->operands[0] = operand1;
 	expr->operands[1] = operand2;
 
@@ -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,18 +2709,22 @@ 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);
 	}
 
 	return expr;
-free:
-	destroy_hist_field(operand1, 0);
+
+free_operands:
 	destroy_hist_field(operand2, 0);
-	destroy_hist_field(expr, 0);
+free_op1:
+	destroy_hist_field(operand1, 0);
+	return ERR_PTR(ret);
 
+free_expr:
+	destroy_hist_field(expr, 0);
 	return ERR_PTR(ret);
 }
 

Re: [GIT PULL] tracing: Fix double free bug

[Steven Rostedt]

On Wed, 17 Nov 2021 15:38:59 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, Nov 17, 2021 at 3:19 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On error, the operands and the histogram expression are destroyed,
> > but since the destruction is recursive, do not destroy the operands
> > if they already belong to the expression that is about to be destroyed.  
> 
> Honestly, this seems horribly ugly.

I guess we have a difference in opinion to what is ugly, as the v1 version
of Kalesh's patch was closer to yours, and I hated the complexity of having
to know when to to call what. Because the logic is not that simple.

See https://lore.kernel.org/all/20211117021223.2137117-1-kaleshsingh@google.com/

> 
> The problem seems to be that the "goto error" cases are simply just wrong.
> 
> Why isn't the fix to make the error cases be the right ones, instead
> of having one odd error case that then has to do some magic things to
> not free the wrong things?
> 
> The patch ends up a bit bigger, mainly because I renamed the different
> "free" cases, and because I made the non-freeing ones just return the
> error directly.

I agree with the first part of your patch, which was not the reason for
being the cause of the bug.

> 
> Something like this (UNTESTED!) patch, IOW?

But the part after the expr is allocated gets a bit more tricky, and that
is why I requested that logic, namely due to the "combine_consts" case. But
as the expr->operand[]s are NULL'd and the operand*s are destroyed, I guess
it's still fine with just freeing the expr if we add an error case there.

Kalesh, care to spin a v3 implementing Linus's solution?

-- Steve

Re: [GIT PULL] tracing: Fix double free bug

[Kalesh Singh]

On Wed, Nov 17, 2021 at 4:00 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 17 Nov 2021 15:38:59 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > On Wed, Nov 17, 2021 at 3:19 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > On error, the operands and the histogram expression are destroyed,
> > > but since the destruction is recursive, do not destroy the operands
> > > if they already belong to the expression that is about to be destroyed.
> >
> > Honestly, this seems horribly ugly.
>
> I guess we have a difference in opinion to what is ugly, as the v1 version
> of Kalesh's patch was closer to yours, and I hated the complexity of having
> to know when to to call what. Because the logic is not that simple.
>
> See https://lore.kernel.org/all/20211117021223.2137117-1-kaleshsingh@google.com/
>
> >
> > The problem seems to be that the "goto error" cases are simply just wrong.
> >
> > Why isn't the fix to make the error cases be the right ones, instead
> > of having one odd error case that then has to do some magic things to
> > not free the wrong things?
> >
> > The patch ends up a bit bigger, mainly because I renamed the different
> > "free" cases, and because I made the non-freeing ones just return the
> > error directly.
>
> I agree with the first part of your patch, which was not the reason for
> being the cause of the bug.
>
> >
> > Something like this (UNTESTED!) patch, IOW?
>
> But the part after the expr is allocated gets a bit more tricky, and that
> is why I requested that logic, namely due to the "combine_consts" case. But
> as the expr->operand[]s are NULL'd and the operand*s are destroyed, I guess
> it's still fine with just freeing the expr if we add an error case there.
>
> Kalesh, care to spin a v3 implementing Linus's solution?

Hi Steve,

Yes I'll resend a new version, more in line to Linus suggestion.

Thanks,
Kalesh
>
> -- Steve