Re: [PATCH v8 07/21] tracing/simple_ring_buffer: Add support for compressed length

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] tracing: shared-memory TOCTOU in simple_ring_buffer_reserve()
--

commit 9257cd3bebb215fd4b1122136bcc3bcfeab103ba
Author: Vincent Donnefort <vdonnefort@google.com>

tracing/simple_ring_buffer: Add support for compressed length

This commit compresses small event lengths into the ring buffer event header
type_len field and enforces ring-buffer alignment to save 4 bytes per small
event.

> diff --git a/kernel/trace/simple_ring_buffer.c b/kernel/trace/simple_ring_buffer.c
> index abfaf9bee12e1..a305331d29ba9 100644
> --- a/kernel/trace/simple_ring_buffer.c
> +++ b/kernel/trace/simple_ring_buffer.c
> @@ -207,7 +207,13 @@ static unsigned long rb_event_size(unsigned long length)
>  {
>  	struct ring_buffer_event *event;
>  
> -	return length + RB_EVNT_HDR_SIZE + sizeof(event->array[0]);
> +	if (!length)
> +		length++;
> +
> +	if (length > RB_MAX_SMALL_DATA || RB_FORCE_8BYTE_ALIGNMENT)
> +		length += sizeof(event->array[0]);
> +
> +	return ALIGN(length + RB_EVNT_HDR_SIZE, RB_ARCH_ALIGNMENT);
>  }
>  
>  static struct ring_buffer_event *
> @@ -223,12 +229,15 @@ rb_event_add_ts_extend(struct ring_buffer_event *event, u64 delta)
>  static struct ring_buffer_event *
>  simple_rb_reserve_next(struct simple_rb_per_cpu *cpu_buffer, unsigned long length, u64 timestamp)
>  {
> -	unsigned long ts_ext_size = 0, event_size = rb_event_size(length);
>  	struct simple_buffer_page *tail = cpu_buffer->tail_page;
> +	unsigned long event_size, array_size, ts_ext_size = 0;
>  	struct ring_buffer_event *event;
>  	u32 write, prev_write;
>  	u64 time_delta;
>  
> +	event_size = rb_event_size(length);
> +	array_size = event_size - RB_EVNT_HDR_SIZE;
> +
>  	time_delta = timestamp - cpu_buffer->write_stamp;
>  
>  	if (test_time_stamp(time_delta))
> @@ -259,9 +268,13 @@ simple_rb_reserve_next(struct simple_rb_per_cpu *cpu_buffer, unsigned long lengt
>  		time_delta = 0;
>  	}
>  
> -	event->type_len = 0;
> +	if (length > RB_MAX_SMALL_DATA || RB_FORCE_8BYTE_ALIGNMENT) {
> +		event->type_len = 0;
> +		event->array[0] = array_size;
> +	} else {
> +		event->type_len = DIV_ROUND_UP(array_size, RB_ALIGNMENT);
> +	}
>  	event->time_delta = time_delta;
> -	event->array[0] = event_size - RB_EVNT_HDR_SIZE;
>  
>  	return event;
>  }
> @@ -284,7 +297,7 @@ void *simple_ring_buffer_reserve(struct simple_rb_per_cpu *cpu_buffer, unsigned
>  
>  	rb_event = simple_rb_reserve_next(cpu_buffer, length, timestamp);
>  
> -	return &rb_event->array[1];
> +	return rb_event->type_len ? &rb_event->array[0] : &rb_event->array[1];

[Severity: High]
Can reading rb_event->type_len back from the shared buffer page cause a
4-byte out-of-bounds write at EL2?

When compiled into the arm64 pKVM hypervisor (arch/arm64/kvm/hyp/nvhe/trace.c),
tail->page is shared with the untrusted host kernel via hyp_pin_shared_mem(),
and RB_FORCE_8BYTE_ALIGNMENT is 0. For length <= RB_MAX_SMALL_DATA,
rb_event_size() does not reserve space for event->array[0], but
simple_ring_buffer_reserve() re-reads rb_event->type_len from shared memory
to choose between &rb_event->array[0] and &rb_event->array[1]:

CPU0 (EL2 hypervisor)             CPU1 (EL1 host)
simple_ring_buffer_reserve()
  simple_rb_reserve_next()
    event->type_len = ...
    event->time_delta = ...
                                  rb_event->type_len = 0;
  if (rb_event->type_len)
    ...
  return &rb_event->array[1];  <-- 4 bytes past reserved space

If an event is reserved at the end of a page (write == PAGE_SIZE -
BUF_PAGE_HDR_SIZE), the caller writes 4 bytes past the end of tail->page in
EL2 memory. Checking (length > RB_MAX_SMALL_DATA || RB_FORCE_8BYTE_ALIGNMENT)
instead of rb_event->type_len avoids reading back from shared memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106180657.2460376-1-vdonnefort@google.com?part=7