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 <[email protected]>
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/[email protected]
Signed-off-by: Kalesh Singh <[email protected]>
Fixes: 8b5d46fd7a38 ("tracing/histogram: Optimize division by constants")
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Steven Rostedt (VMware) <[email protected]>
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);
On Wed, Nov 17, 2021 at 3:19 PM Steven Rostedt <[email protected]> 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
On Wed, 17 Nov 2021 15:38:59 -0800
Linus Torvalds <[email protected]> wrote:
> On Wed, Nov 17, 2021 at 3:19 PM Steven Rostedt <[email protected]> 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/[email protected]/
>
> 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
On Wed, Nov 17, 2021 at 4:00 PM Steven Rostedt <[email protected]> wrote:
>
> On Wed, 17 Nov 2021 15:38:59 -0800
> Linus Torvalds <[email protected]> wrote:
>
> > On Wed, Nov 17, 2021 at 3:19 PM Steven Rostedt <[email protected]> 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/[email protected]/
>
> >
> > 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