ring-buffer: fix incorrect boundary check order

Commit Message

Jeongjun Park Jan. 10, 2025, 4:26 p.m. UTC

If there is a case where the variable s is greater than or equal to nr_subbufs
before entering the loop, oob read or use-after-free will occur. This problem
occurs because the variable s is used as an index to dereference the
struct page before the variable value range check. This logic prevents the
wrong address value from being copied to the pages array through the subsequent
range check, but oob read still occurs, so the code needs to be modified.

Signed-off-by: Jeongjun Park <[e-mail]>
---
 kernel/trace/ring_buffer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--

Comments

Steven Rostedt Jan. 10, 2025, 5:27 p.m. UTC | #1

On Sat, 11 Jan 2025 01:26:12 +0900
Jeongjun Park <[e-mail]> wrote:

> If there is a case where the variable s is greater than or equal to nr_subbufs
> before entering the loop, oob read or use-after-free will occur. This problem
> occurs because the variable s is used as an index to dereference the
> struct page before the variable value range check. This logic prevents the
> wrong address value from being copied to the pages array through the subsequent
> range check, but oob read still occurs, so the code needs to be modified.

This is *not* a fix!

As this should never happen. The logic before it prevents it from happening.
The s is calculated from the same logic that calculates nr_pages. If s were
to be greater than nr_subbufs it is a bug!

> 
> Signed-off-by: Jeongjun Park <[e-mail]>
> ---
>  kernel/trace/ring_buffer.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 60210fb5b211..6804ab126802 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -7059,7 +7059,7 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
>  	}
>  
>  	while (p < nr_pages) {
> -		struct page *page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
> +		struct page *page;
>  		int off = 0;
>  
>  		if (WARN_ON_ONCE(s >= nr_subbufs)) {

Notice that there's a WARN_ON_ONCE() here. That means if this is true, the
code is buggy. User space should never be able to trigger this either.

That said, I'm not against the patch and will add it for the next merge
window. But I will modify the subject and change log to state that this
should never happen and if it does the code is buggy and needs to be fixed.
That this is just a clean up in case the code is buggy we can trigger the
WARN_ON() and not worry about an out of bounds hit. Something like:

 ring-buffer: Make reading page consistent with the code logic

 In the loop of __rb_map_vma(), the 's' variable is calculated from the
 same logic that nr_pages is and they both come from nr_subbufs. But the
 relationship is not obvious and there's a WARN_ON_ONCE() around the 's'
 variable to make sure it never becomes equal to nr_subbufs within the
 loop. If that happens, then the code is buggy and needs to be fixed.

 The 'page' variable is calculated from cpu_buffer->subbuf_ids[s] which is
 an array of 'nr_subbufs' entries. If the code becomes buggy and 's'
 becomes equal to or greater than 'nr_subbufs' then this will be an out of
 bounds hit before the WARN_ON() is triggered and the code exiting safely.

 Make the 'page' initialization consistent with the code logic and assign
 it after the out of bounds check.

-- Steve


> @@ -7067,6 +7067,8 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
>  			goto out;
>  		}
>  
> +		page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
> +
>  		for (; off < (1 << (subbuf_order)); off++, page++) {
>  			if (p >= nr_pages)
>  				break;
> --

Patch

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 60210fb5b211..6804ab126802 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -7059,7 +7059,7 @@  static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
 	}
 
 	while (p < nr_pages) {
-		struct page *page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
+		struct page *page;
 		int off = 0;
 
 		if (WARN_ON_ONCE(s >= nr_subbufs)) {
@@ -7067,6 +7067,8 @@  static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
 			goto out;
 		}
 
+		page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
+
 		for (; off < (1 << (subbuf_order)); off++, page++) {
 			if (p >= nr_pages)
 				break;