Re: BUG: KASAN: slab-out-of-bounds in print_synth_event+0xa68/0xa78
From: Steven Rostedt
Date: Tue Aug 08 2023 - 13:36:29 EST
On Tue, 08 Aug 2023 11:44:26 +0200
Sven Schnelle <svens@xxxxxxxxxxxxx> wrote:
> Steven Rostedt <rostedt@xxxxxxxxxxx> writes:
>
> > On Fri, 04 Aug 2023 08:20:23 +0200
> > Sven Schnelle <svens@xxxxxxxxxxxxx> wrote:
> >
> >> Hi Steven,
> >>
> >> i noticed the following KASAN splat in CI (on s390):
> >
> > Could this actually be a bug in KASAN?
> >
> > The reason I ask, is because of the report.
>
> 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 :-(
>
> 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@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxx>
> ---
> 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:
struct trace_dynamic {
#ifdef CONFIG_CPU_BIG_ENDIAN
u16 offset;
u16 len;
#else
u16 len;
u16 offset
#endif
};
So that we don't break user space (which still does the:
offset = value & 0xffff;
len = value >> 16;
The rest seems good to me for just synthetic event.
Thanks!
-- Steve
> +
> +union synth_trace_field {
> + u8 as8;
> + u16 as16;
> + u32 as32;
> + u64 as64;
> + struct synth_trace_data as_data;
> +};
> +
> struct synth_trace_event {
> struct trace_entry ent;
> - u64 fields[];
> + union synth_trace_field fields[];
> };
>
> static int synth_event_define_fields(struct trace_event_call *call)
> @@ -321,19 +334,19 @@ static const char *synth_field_fmt(char *type)
>
> static void print_synth_event_num_val(struct trace_seq *s,
> char *print_fmt, char *name,
> - int size, u64 val, char *space)
> + int size, union synth_trace_field *val, char *space)
> {
> switch (size) {
> case 1:
> - trace_seq_printf(s, print_fmt, name, (u8)val, space);
> + trace_seq_printf(s, print_fmt, name, val->as8, space);
> break;
>
> case 2:
> - trace_seq_printf(s, print_fmt, name, (u16)val, space);
> + trace_seq_printf(s, print_fmt, name, val->as16, space);
> break;
>
> case 4:
> - trace_seq_printf(s, print_fmt, name, (u32)val, space);
> + trace_seq_printf(s, print_fmt, name, val->as32, space);
> break;
>
> default:
> @@ -374,36 +387,26 @@ static enum print_line_t print_synth_event(struct trace_iterator *iter,
> /* parameter values */
> if (se->fields[i]->is_string) {
> if (se->fields[i]->is_dynamic) {
> - u32 offset, data_offset;
> - char *str_field;
> -
> - offset = (u32)entry->fields[n_u64];
> - data_offset = offset & 0xffff;
> -
> - str_field = (char *)entry + data_offset;
> + struct synth_trace_data *data = &entry->fields[n_u64].as_data;
>
> trace_seq_printf(s, print_fmt, se->fields[i]->name,
> STR_VAR_LEN_MAX,
> - str_field,
> + (char *)entry + data->offset,
> i == se->n_fields - 1 ? "" : " ");
> n_u64++;
> } else {
> trace_seq_printf(s, print_fmt, se->fields[i]->name,
> STR_VAR_LEN_MAX,
> - (char *)&entry->fields[n_u64],
> + (char *)&entry->fields[n_u64].as64,
> i == se->n_fields - 1 ? "" : " ");
> n_u64 += STR_VAR_LEN_MAX / sizeof(u64);
> }
> } else if (se->fields[i]->is_stack) {
> - u32 offset, data_offset, len;
> unsigned long *p, *end;
> + struct synth_trace_data *data = &entry->fields[n_u64].as_data;
>
> - offset = (u32)entry->fields[n_u64];
> - data_offset = offset & 0xffff;
> - len = offset >> 16;
> -
> - p = (void *)entry + data_offset;
> - end = (void *)p + len - (sizeof(long) - 1);
> + p = (void *)entry + data->offset;
> + end = (void *)p + data->len - (sizeof(long) - 1);
>
> trace_seq_printf(s, "%s=STACK:\n", se->fields[i]->name);
>
> @@ -419,13 +422,13 @@ static enum print_line_t print_synth_event(struct trace_iterator *iter,
> print_synth_event_num_val(s, print_fmt,
> se->fields[i]->name,
> se->fields[i]->size,
> - entry->fields[n_u64],
> + &entry->fields[n_u64],
> space);
>
> if (strcmp(se->fields[i]->type, "gfp_t") == 0) {
> trace_seq_puts(s, " (");
> trace_print_flags_seq(s, "|",
> - entry->fields[n_u64],
> + entry->fields[n_u64].as64,
> __flags);
> trace_seq_putc(s, ')');
> }
> @@ -454,21 +457,16 @@ static unsigned int trace_string(struct synth_trace_event *entry,
> int ret;
>
> if (is_dynamic) {
> - u32 data_offset;
> + struct synth_trace_data *data = &entry->fields[*n_u64].as_data;
>
> - data_offset = struct_size(entry, fields, event->n_u64);
> - data_offset += data_size;
> -
> - len = fetch_store_strlen((unsigned long)str_val);
> -
> - data_offset |= len << 16;
> - *(u32 *)&entry->fields[*n_u64] = data_offset;
> + data->offset = struct_size(entry, fields, event->n_u64) + data_size;
> + data->len = fetch_store_strlen((unsigned long)str_val);
>
> ret = fetch_store_string((unsigned long)str_val, &entry->fields[*n_u64], entry);
>
> (*n_u64)++;
> } else {
> - str_field = (char *)&entry->fields[*n_u64];
> + str_field = (char *)&entry->fields[*n_u64].as64;
>
> #ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> if ((unsigned long)str_val < TASK_SIZE)
> @@ -492,6 +490,7 @@ static unsigned int trace_stack(struct synth_trace_event *entry,
> unsigned int data_size,
> unsigned int *n_u64)
> {
> + struct synth_trace_data *data = &entry->fields[*n_u64].as_data;
> unsigned int len;
> u32 data_offset;
> void *data_loc;
> @@ -515,8 +514,9 @@ static unsigned int trace_stack(struct synth_trace_event *entry,
> memcpy(data_loc, stack, len);
>
> /* Fill in the field that holds the offset/len combo */
> - data_offset |= len << 16;
> - *(u32 *)&entry->fields[*n_u64] = data_offset;
> +
> + data->offset = data_offset;
> + data->len = len;
>
> (*n_u64)++;
>
> @@ -592,19 +592,19 @@ static notrace void trace_event_raw_event_synth(void *__data,
>
> switch (field->size) {
> case 1:
> - *(u8 *)&entry->fields[n_u64] = (u8)val;
> + entry->fields[n_u64].as8 = (u8)val;
> break;
>
> case 2:
> - *(u16 *)&entry->fields[n_u64] = (u16)val;
> + entry->fields[n_u64].as16 = (u16)val;
> break;
>
> case 4:
> - *(u32 *)&entry->fields[n_u64] = (u32)val;
> + entry->fields[n_u64].as32 = (u32)val;
> break;
>
> default:
> - entry->fields[n_u64] = val;
> + entry->fields[n_u64].as64 = val;
> break;
> }
> n_u64++;
> @@ -1790,19 +1790,19 @@ int synth_event_trace(struct trace_event_file *file, unsigned int n_vals, ...)
>
> switch (field->size) {
> case 1:
> - *(u8 *)&state.entry->fields[n_u64] = (u8)val;
> + state.entry->fields[n_u64].as8 = (u8)val;
> break;
>
> case 2:
> - *(u16 *)&state.entry->fields[n_u64] = (u16)val;
> + state.entry->fields[n_u64].as16 = (u16)val;
> break;
>
> case 4:
> - *(u32 *)&state.entry->fields[n_u64] = (u32)val;
> + state.entry->fields[n_u64].as32 = (u32)val;
> break;
>
> default:
> - state.entry->fields[n_u64] = val;
> + state.entry->fields[n_u64].as64 = val;
> break;
> }
> n_u64++;
> @@ -1883,19 +1883,19 @@ int synth_event_trace_array(struct trace_event_file *file, u64 *vals,
>
> switch (field->size) {
> case 1:
> - *(u8 *)&state.entry->fields[n_u64] = (u8)val;
> + state.entry->fields[n_u64].as8 = (u8)val;
> break;
>
> case 2:
> - *(u16 *)&state.entry->fields[n_u64] = (u16)val;
> + state.entry->fields[n_u64].as16 = (u16)val;
> break;
>
> case 4:
> - *(u32 *)&state.entry->fields[n_u64] = (u32)val;
> + state.entry->fields[n_u64].as32 = (u32)val;
> break;
>
> default:
> - state.entry->fields[n_u64] = val;
> + state.entry->fields[n_u64].as64 = val;
> break;
> }
> n_u64++;
> @@ -2030,19 +2030,19 @@ static int __synth_event_add_val(const char *field_name, u64 val,
> } else {
> switch (field->size) {
> case 1:
> - *(u8 *)&trace_state->entry->fields[field->offset] = (u8)val;
> + trace_state->entry->fields[field->offset].as8 = (u8)val;
> break;
>
> case 2:
> - *(u16 *)&trace_state->entry->fields[field->offset] = (u16)val;
> + trace_state->entry->fields[field->offset].as16 = (u16)val;
> break;
>
> case 4:
> - *(u32 *)&trace_state->entry->fields[field->offset] = (u32)val;
> + trace_state->entry->fields[field->offset].as32 = (u32)val;
> break;
>
> default:
> - trace_state->entry->fields[field->offset] = val;
> + trace_state->entry->fields[field->offset].as64 = val;
> break;
> }
> }