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

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

[Lee Jones]

Commit 0934ae9977c2 ("tracing: Fix reading strings from synthetic
events") switched synthetic event dynamic strings to use
fetch_store_strlen() and fetch_store_string(), relying on commit
2e9906f84fc7 ("tracing: Add "(fault)" name injection to kernel probes")
to reserve and store FAULT_STRING when a string address faults.

When commit 4ed8f337dee3 ("Revert "tracing: Add "(fault)" name injection
to kernel probes"") and commit 797311bce5c2 ("tracing/probes: Fix to
record 0-length data_loc in fetch_store_string*() if fails") later
reverted FAULT_STRING injection in favour of recording a 0-length
data_loc on fault, synthetic events were left unchanged.

Consequently, a faulting kernel string address causes get_field_size()
to add a negative error code (-EFAULT) to fields_size, under-reserving
the trace buffer entry, while trace_string() casts the negative return
value to an unsigned length and passes it to fetch_store_string() and
write_synth_entry(), corrupting the ring buffer.

Skip non-positive lengths in get_field_size(), record a 0-length
dynamic field in trace_string() when fetching a dynamic string fails,
and print FAULT_STRING in print_synth_event() when a dynamic string has
zero length, matching trace_probe.

Fixes: 4ed8f337dee3 ("Revert "tracing: Add "(fault)" name injection to kernel probes"")
Cc: stable@vger.kernel.org
Signed-off-by: Lee Jones <lee@kernel.org>
---
 kernel/trace/trace_events_synth.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index dc15658a887c..7506e2e1f64a 100644
--- a/kernel/trace/trace_events_synth.c
+++ b/kernel/trace/trace_events_synth.c
@@ -376,7 +376,9 @@ static enum print_line_t print_synth_event(struct trace_iterator *iter,
 				union trace_synth_field *data = &entry->fields[n_u64];
 
 				trace_seq_printf(s, print_fmt, se->fields[i]->name,
-						 (char *)entry + data->as_dynamic.offset,
+						 data->as_dynamic.len ?
+						 (char *)entry + data->as_dynamic.offset :
+						 FAULT_STRING,
 						 i == se->n_fields - 1 ? "" : " ");
 				n_u64++;
 			} else {
@@ -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;
-- 
2.56.0.506.g506a7eabc5-goog

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

[sashiko-bot]

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