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

[Mathias Krause]

On 26.07.24 16:52, Steven Rostedt wrote:
> On Fri, 26 Jul 2024 12:16:16 +0200
> Mathias Krause <minipli@xxxxxxxxxxxxxx> wrote:
> 
>>>
>>> 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.

>>> 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.

>>>
>>> Link: https://lore.kernel.org/all/20240719204701.1605950-1-minipli@xxxxxxxxxxxxxx/
>>>
>>> Cc: stable@xxxxxxxxxxxxxxx
>>> Reported-by: Mathias Krause <minipli@xxxxxxxxxxxxxx>
>>> 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.

>>
>> 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