Re: BUG: KASAN: slab-out-of-bounds in print_synth_event+0xa68/0xa78

[Sven Schnelle <svens@linux.ibm.com>]

Steven Rostedt <rostedt@goodmis.org> writes:

>> I think the problem is that the code assigns data_offset with:
>> 
>> *(u32 *)&entry->fields[*n_u64] = data_offset;
>> 
>> but reads it with:
>> 
>> offset = (u32)entry->fields[n_u64];
>> 
>> which works on LE, but not BE.
>
> Ah, that makes sense. I didn't realize (or forgot) that s390 was BE. My
> PowerPC box that was BE died years ago, and I have stopped testing BE ever
> since :-(

Ok. If you want something for testing BE i could provide you with an
s390 linux image + the commandline to run that within qemu. Linux on
s390 is not much different than other platforms, but you would need an
s390 cross-compiler.

>> 
>> I'm currently preparing the patch below, which also makes the code a bit
>> easier to read. I'm still seeing no stack traces, but at least the
>> random memory reads are gone and no KASAN warning anymore. I'll
>> continue fixing and sent a full patch as soon as everything is fixed.
>> 
>> >From 82fc673f0d3b6031b760b4217bebdb1047119041 Mon Sep 17 00:00:00 2001  
>> From: Sven Schnelle <svens@linux.ibm.com>
>> Date: Tue, 8 Aug 2023 11:35:12 +0200
>> Subject: [PATCH] tracing/synthetic: use union instead of casts
>> 
>> The current code uses a lot of casts to access the fields
>> member in struct synth_trace_events with different sizes.
>> This makes the code hard to read, and had already introduced
>> an endianess bug. Use a union and struct instead.
>> 
>> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
>> ---
>>  kernel/trace/trace_events_synth.c | 100 +++++++++++++++---------------
>>  1 file changed, 50 insertions(+), 50 deletions(-)
>> 
>> diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
>> index d6a70aff2410..1f8fe7f2b5b2 100644
>> --- a/kernel/trace/trace_events_synth.c
>> +++ b/kernel/trace/trace_events_synth.c
>> @@ -125,9 +125,22 @@ static bool synth_event_match(const char *system, const char *event,
>>  		(!system || strcmp(system, SYNTH_SYSTEM) == 0);
>>  }
>>  
>> +struct synth_trace_data {
>> +	u16 len;
>> +	u16 offset;
>> +};
>
> This is actually common throughout the tracing code (as all dynamic fields
> have this). We should probably make this more generic than just for
> synthetic events. Although, that would probably break BE user space. Hmm,
> we could have it be:

I'm not familiar with the ftrace code, so I think i would need some more
time to find all the other locations. Therefore i updated the patch to move
the structure declaration to trace.h and sent that as a first step.

Thanks,
Sven