Re: [RESEND] tracing/synthetic: Handle dynamic string read faults

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] tracing: TOCTOU ring buffer overflow in trace_string() [1]
--

commit f7b85a2fa215ca47a63174ad2034f4bca609b931
Author: Lee Jones <lee@kernel.org>

tracing/synthetic: Handle dynamic string read faults

This commit updates synthetic event dynamic string handling to skip
non-positive lengths in get_field_size(), record a 0-length dynamic field
in trace_string() when fetching a string fails, and print FAULT_STRING in
print_synth_event() when the dynamic string length is zero.

> diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
> index e2870212e58d4..8492a5f285c4e 100644
> --- a/kernel/trace/trace_events_synth.c
> +++ b/kernel/trace/trace_events_synth.c
[ ... ]
> @@ -438,11 +440,20 @@ static unsigned int trace_string(struct synth_trace_event *entry,
>  	if (is_dynamic) {
>  		union trace_synth_field *data = &entry->fields[*n_u64];
>  
> -		len = fetch_store_strlen((unsigned long)str_val);
>  		data->as_dynamic.offset = struct_size(entry, fields, event->n_u64) + data_size;
> -		data->as_dynamic.len = len;
> +
> +		ret = fetch_store_strlen((unsigned long)str_val);
> +		if (ret <= 0) {
> +			data->as_dynamic.len = 0;
> +			(*n_u64)++;
> +			return 0;
> +		}
> +
> +		data->as_dynamic.len = ret;
>  
>  		ret = fetch_store_string((unsigned long)str_val, &entry->fields[*n_u64], entry);
> +		if (ret > 0)
> +			len = ret;
>  
>  		(*n_u64)++;
>  	} else {
> @@ -522,6 +533,8 @@ static __always_inline int get_field_size(struct synth_event *event,
>  			len *= sizeof(unsigned long);
>  		} else {
>  			len = fetch_store_strlen((unsigned long)str_val);
> +			if (len <= 0)
> +				continue;
>  		}
>  
>  		fields_size += len;

[Severity: Critical]
This problem wasn't introduced by this patch, but can recalculating the
string length in trace_string() overflow the reserved ring buffer entry
[1]?

trace_event_raw_event_synth() calls get_field_size() to measure each
dynamic string with fetch_store_strlen() and reserves a ring buffer entry
of that size.  write_synth_entry() -> trace_string() then calls
fetch_store_strlen() a second time on str_val and sets
data->as_dynamic.len to the new return value before calling
fetch_store_string():

CPU0                                    CPU1
trace_event_raw_event_synth()
  get_field_size()
    fetch_store_strlen(str_val) -> 4
  trace_event_buffer_reserve(...,
    sizeof(*entry) + fields_size)
                                        event_hist_trigger()
                                          /* updates shared hist elt
                                             str_val to a longer string */
  write_synth_entry()
    trace_string()
      ret = fetch_store_strlen(str_val) -> 32
      data->as_dynamic.len = ret
      fetch_store_string(str_val, ...)  <-- writes 32 bytes past
                                            4-byte reservation

Because fetch_store_string() reads its maximum copy length from
data->as_dynamic.len rather than the length reserved in get_field_size(),
concurrent growth of the source string writes past the end of the reserved
ring buffer entry and corrupts the ring buffer.

[1] https://sashiko.dev/#/bug/linux-99ac32b2-e9bc-4529-bfdc-62d0b5345672

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109155758.3330810-1-lee@kernel.org?part=1