Re: [PATCH] tracing: Have format file honor EVENT_FILE_FL_FREED

On 26.07.24 16:52, Steven Rostedt wrote:

On Fri, 26 Jul 2024 12:16:16 +0200
Mathias Krause [off-list ref] wrote:

quoted

quoted

With KASAN memory checking, it would trigger a use-after-free bug. This was  

The UAF bug is there even without KASAN. It's just that KASAN makes it
much easier to detect and catch early.

Well the bug happens without KASAN but the "tigger" is shown by KASAN.
I was assuming people understood that.

Ahh, okay. I'd written "report" instead of "trigger" to make it less
ambiguous.

quoted

quoted

because the format file was not checking the file's meta data flag
"EVENT_FILE_FL_FREED", so it would access the event that the file meta data
pointed to after it was freed.

The second bug is that the dynamic "format" file also registered a callback
to decrement the meta data, but the "data" pointer passed to the callback
was the event itself. Not the meta data to free. This would either cause a
memory leak (the meta data never was freed) or a crash as it could have
incorrectly freed the event itself.

I need to remove the above, as I realized the release callback doesn't
get called for the "filter" but for only the "enable". That doesn't get
called until all files have no more references. So there's only one bug
here.

Yeah, all files are covered by the same event_file and only "enable"
does the final put.

quoted

quoted

Link: https://lore.kernel.org/all/20240719204701.1605950-1-minipli@grsecurity.net/ (local)

Cc: stable@vger.kernel.org
Reported-by: Mathias Krause <redacted>
Fixes: b63db58e2fa5d ("eventfs/tracing: Add callback for release of an eventfs_inode")  

That fixes tag looks odd as it didn't introduce the bug. It's some late
change to v6.9 but my bisect run showed, it's triggering as early as in
v6.6 (commit 27152bceea1d ("eventfs: Move tracing/events to eventfs")).

git blame points to 5790b1fb3d67 ("eventfs: Remove eventfs_file and just
use eventfs_inode"), which is still too young, as it's v6.7.

But if you look at the commit I posted. It has:

  Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode")

And you need to add that to apply this patch as it has that as the
dependency. If you try to apply this to the change that had the
original bug, it will not apply. I basically say that this patch is a
fix to the previous fix.

So far so good.

quoted

IMHO, this needs at least the following additional fixes tags to ensure
all stable kernels get covered:

Fixes: 5790b1fb3d67 ("eventfs: Remove eventfs_file and just use
eventfs_inode")
Fixes: 27152bceea1d ("eventfs: Move tracing/events to eventfs")

Even if 27152bceea1d is not the real cause, just the commit making the
bug reachable. But from looking at the history, this was always wrong?

All stable kernels should get covered as 27152bceea1d has both a Cc
stable tag and a Fixes tag for 5790b1fb3d67. And the stable kernels
look at what commits have been backported to determine what other
commits should be backported.

Now you lost me. Neither has 27152bceea1d a Cc stable tag, nor a Fixes
tag for 5790b1fb3d67. It simply cannot, because it's from July 2023
(v6.6-rc1) and 5790b1fb3d67 is from October 2024 (v6.7-rc1).

                              By saying this fixes 27152bceea1d, it
should all work out correctly.

That would be fine with me, as that's what my git bisect run pointed at
as well -- the oldest commit triggering the bug. However, in your v2
it's still b63db58e2fa5d (which has a Fixes tag for 5790b1fb3d672 but
not 27152bceea1d) which would suggest only kernels down to v6.7 are
affected.

Thanks,
Mathias