[PATCH] ring-buffer: fix array bounds checking

[PATCH] ring-buffer: fix array bounds checking

[Jeongjun Park]

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.

Fixes: 117c39200d9d ("ring-buffer: Introducing ring-buffer mapping functions")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
 kernel/trace/ring_buffer.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 7e257e855dd1..83da74bf7bd6 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -6994,9 +6994,9 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
 {
 	unsigned long nr_subbufs, nr_pages, nr_vma_pages, pgoff = vma->vm_pgoff;
 	unsigned int subbuf_pages, subbuf_order;
-	struct page **pages;
+	struct page **pages, *page;
 	int p = 0, s = 0;
-	int err;
+	int err, off;
 
 	/* Refuse MP_PRIVATE or writable mappings */
 	if (vma->vm_flags & VM_WRITE || vma->vm_flags & VM_EXEC ||
@@ -7055,14 +7055,14 @@ 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]);
-		int off = 0;
-
 		if (WARN_ON_ONCE(s >= nr_subbufs)) {
 			err = -EINVAL;
 			goto out;
 		}
 
+		page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
+		off = 0;
+
 		for (; off < (1 << (subbuf_order)); off++, page++) {
 			if (p >= nr_pages)
 				break;
--

Re: [PATCH] ring-buffer: fix array bounds checking

[Vincent Donnefort]

On Tue, Dec 17, 2024 at 01:49:30AM +0900, Jeongjun Park 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.

Hi Jeongjun, thanks for the patch.

Did you find a reproducer for that problem or has it just been found by code
inspection?

As discussed here [1], s >= nr_subbufs should really never happen as we already
cap nr_pages. 

[1] https://lore.kernel.org/all/78e20e98-bdfc-4d7b-a59c-988b81fcc58b@redhat.com/, 

> 
> Fixes: 117c39200d9d ("ring-buffer: Introducing ring-buffer mapping functions")
> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> ---
>  kernel/trace/ring_buffer.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 7e257e855dd1..83da74bf7bd6 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -6994,9 +6994,9 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
>  {
>  	unsigned long nr_subbufs, nr_pages, nr_vma_pages, pgoff = vma->vm_pgoff;
>  	unsigned int subbuf_pages, subbuf_order;
> -	struct page **pages;
> +	struct page **pages, *page;
>  	int p = 0, s = 0;
> -	int err;
> +	int err, off;
>  
>  	/* Refuse MP_PRIVATE or writable mappings */
>  	if (vma->vm_flags & VM_WRITE || vma->vm_flags & VM_EXEC ||
> @@ -7055,14 +7055,14 @@ 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]);
> -		int off = 0;
> -

I believe we can keep the struct page and off declaration within the while loop.

>  		if (WARN_ON_ONCE(s >= nr_subbufs)) {
>  			err = -EINVAL;
>  			goto out;
>  		}
>  
> +		page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
> +		off = 0;
> +
>  		for (; off < (1 << (subbuf_order)); off++, page++) {
>  			if (p >= nr_pages)
>  				break;
> --

Re: [PATCH] ring-buffer: fix array bounds checking

[Jeongjun Park]

> Vincent Donnefort <vdonnefort@google.com> wrote:
> On Tue, Dec 17, 2024 at 01:49:30AM +0900, Jeongjun Park 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.
> 
> Hi Jeongjun, thanks for the patch.
> 
> Did you find a reproducer for that problem or has it just been found by code
> inspection?
> 
> As discussed here [1], s >= nr_subbufs should really never happen as we already
> cap nr_pages.
> 
> [1] https://lore.kernel.org/all/78e20e98-bdfc-4d7b-a59c-988b81fcc58b@redhat.com/,

I didn't find the bug caused by this separately, but I found it while analyzing
the code. However, since it has been confirmed that syzbot
has a reproducer that generates oob and uaf, this will definitely be
reproduced.

The reason I suggested this patch is because I think the logic of the code
is a bit inappropriate. Normally, a range check is performed before using
a specific variable as an index of an array. Of course, in this loop, the page
structure pointer that was oob-read will not be copied to the pages array,
but I don't think it's very appropriate to read the array using a variable
value that may be out of range as an index before the range check.
Therefore, I suggest patching it like this.

> 
>> 
>> Fixes: 117c39200d9d ("ring-buffer: Introducing ring-buffer mapping functions")
>> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
>> ---
>> kernel/trace/ring_buffer.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
>> index 7e257e855dd1..83da74bf7bd6 100644
>> --- a/kernel/trace/ring_buffer.c
>> +++ b/kernel/trace/ring_buffer.c
>> @@ -6994,9 +6994,9 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
>> {
>>    unsigned long nr_subbufs, nr_pages, nr_vma_pages, pgoff = vma->vm_pgoff;
>>    unsigned int subbuf_pages, subbuf_order;
>> -    struct page **pages;
>> +    struct page **pages, *page;
>>    int p = 0, s = 0;
>> -    int err;
>> +    int err, off;
>> 
>>    /* Refuse MP_PRIVATE or writable mappings */
>>    if (vma->vm_flags & VM_WRITE || vma->vm_flags & VM_EXEC ||
>> @@ -7055,14 +7055,14 @@ 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]);
>> -        int off = 0;
>> -
> 
> I believe we can keep the struct page and off declaration within the while loop.

The reason I modified it this way is that, since this loop will always be 
entered if there are no other issues, these variables will be used in 
many situations, so I think it is quite inefficient to continue to declare variables 
in a loop where you don't know how many times it will be repeated. 
So, I think that declaring variables in advance and then continuously initializing 
their values ​​is advantageous in terms of performance and there are 
no other issues. What do you think?

Regards,

Jeongjun Park

> 
>>        if (WARN_ON_ONCE(s >= nr_subbufs)) {
>>            err = -EINVAL;
>>            goto out;
>>        }
>> 
>> +        page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
>> +        off = 0;
>> +
>>        for (; off < (1 << (subbuf_order)); off++, page++) {
>>            if (p >= nr_pages)
>>                break;
>> --

Re: [PATCH] ring-buffer: fix array bounds checking

[Vincent Donnefort]

On Tue, Dec 17, 2024 at 10:28:49AM +0900, Jeongjun Park wrote:
> 
> 
> > Vincent Donnefort <vdonnefort@google.com> wrote:
> > On Tue, Dec 17, 2024 at 01:49:30AM +0900, Jeongjun Park 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.
> > 
> > Hi Jeongjun, thanks for the patch.
> > 
> > Did you find a reproducer for that problem or has it just been found by code
> > inspection?
> > 
> > As discussed here [1], s >= nr_subbufs should really never happen as we already
> > cap nr_pages.
> > 
> > [1] https://lore.kernel.org/all/78e20e98-bdfc-4d7b-a59c-988b81fcc58b@redhat.com/,
> 
> I didn't find the bug caused by this separately, but I found it while analyzing
> the code. However, since it has been confirmed that syzbot
> has a reproducer that generates oob and uaf, this will definitely be
> reproduced.

Could you share that reproducer? Or at least the steps. As this situation should
never happen a, follow-up fix will be necessary.

> 
> The reason I suggested this patch is because I think the logic of the code
> is a bit inappropriate. Normally, a range check is performed before using
> a specific variable as an index of an array. Of course, in this loop, the page
> structure pointer that was oob-read will not be copied to the pages array,
> but I don't think it's very appropriate to read the array using a variable
> value that may be out of range as an index before the range check.
> Therefore, I suggest patching it like this.

Of course, no question about that.

> 
> > 
> >> 
> >> Fixes: 117c39200d9d ("ring-buffer: Introducing ring-buffer mapping functions")
> >> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> >> ---
> >> kernel/trace/ring_buffer.c | 10 +++++-----
> >> 1 file changed, 5 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> >> index 7e257e855dd1..83da74bf7bd6 100644
> >> --- a/kernel/trace/ring_buffer.c
> >> +++ b/kernel/trace/ring_buffer.c
> >> @@ -6994,9 +6994,9 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
> >> {
> >>    unsigned long nr_subbufs, nr_pages, nr_vma_pages, pgoff = vma->vm_pgoff;
> >>    unsigned int subbuf_pages, subbuf_order;
> >> -    struct page **pages;
> >> +    struct page **pages, *page;
> >>    int p = 0, s = 0;
> >> -    int err;
> >> +    int err, off;
> >> 
> >>    /* Refuse MP_PRIVATE or writable mappings */
> >>    if (vma->vm_flags & VM_WRITE || vma->vm_flags & VM_EXEC ||
> >> @@ -7055,14 +7055,14 @@ 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]);
> >> -        int off = 0;
> >> -
> > 
> > I believe we can keep the struct page and off declaration within the while loop.
> 
> The reason I modified it this way is that, since this loop will always be 
> entered if there are no other issues, these variables will be used in 
> many situations, so I think it is quite inefficient to continue to declare variables 
> in a loop where you don't know how many times it will be repeated. 
> So, I think that declaring variables in advance and then continuously initializing 
> their values ​​is advantageous in terms of performance and there are 
> no other issues. What do you think?

I'm pretty sure the compiler would do the right thing here and no additional
step would result from declaring both variables inside the loop.

> 
> Regards,
> 
> Jeongjun Park
> 
> > 
> >>        if (WARN_ON_ONCE(s >= nr_subbufs)) {
> >>            err = -EINVAL;
> >>            goto out;
> >>        }
> >> 
> >> +        page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
> >> +        off = 0;
> >> +
> >>        for (; off < (1 << (subbuf_order)); off++, page++) {
> >>            if (p >= nr_pages)
> >>                break;
> >> --

Re: [PATCH] ring-buffer: fix array bounds checking

[Jeongjun Park]

Vincent Donnefort <vdonnefort@google.com> wrote:
>
> On Tue, Dec 17, 2024 at 10:28:49AM +0900, Jeongjun Park wrote:
> >
> >
> > > Vincent Donnefort <vdonnefort@google.com> wrote:
> > > On Tue, Dec 17, 2024 at 01:49:30AM +0900, Jeongjun Park 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.
> > >
> > > Hi Jeongjun, thanks for the patch.
> > >
> > > Did you find a reproducer for that problem or has it just been found by code
> > > inspection?
> > >
> > > As discussed here [1], s >= nr_subbufs should really never happen as we already
> > > cap nr_pages.
> > >
> > > [1] https://lore.kernel.org/all/78e20e98-bdfc-4d7b-a59c-988b81fcc58b@redhat.com/,
> >
> > I didn't find the bug caused by this separately, but I found it while analyzing
> > the code. However, since it has been confirmed that syzbot
> > has a reproducer that generates oob and uaf, this will definitely be
> > reproduced.
>
> Could you share that reproducer? Or at least the steps. As this situation should
> never happen a, follow-up fix will be necessary.

[1] When tested with a reproducer, pgoff was 8, subbuf_order was 0, and
subbuf_pages was 1. However, nr_subbufs was 3, so oob-read or uaf occurred.

[1] : https://syzkaller.appspot.com/text?tag=ReproC&x=14514730580000

>
> >
> > The reason I suggested this patch is because I think the logic of the code
> > is a bit inappropriate. Normally, a range check is performed before using
> > a specific variable as an index of an array. Of course, in this loop, the page
> > structure pointer that was oob-read will not be copied to the pages array,
> > but I don't think it's very appropriate to read the array using a variable
> > value that may be out of range as an index before the range check.
> > Therefore, I suggest patching it like this.
>
> Of course, no question about that.
>
> >
> > >
> > >>
> > >> Fixes: 117c39200d9d ("ring-buffer: Introducing ring-buffer mapping functions")
> > >> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> > >> ---
> > >> kernel/trace/ring_buffer.c | 10 +++++-----
> > >> 1 file changed, 5 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > >> index 7e257e855dd1..83da74bf7bd6 100644
> > >> --- a/kernel/trace/ring_buffer.c
> > >> +++ b/kernel/trace/ring_buffer.c
> > >> @@ -6994,9 +6994,9 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
> > >> {
> > >>    unsigned long nr_subbufs, nr_pages, nr_vma_pages, pgoff = vma->vm_pgoff;
> > >>    unsigned int subbuf_pages, subbuf_order;
> > >> -    struct page **pages;
> > >> +    struct page **pages, *page;
> > >>    int p = 0, s = 0;
> > >> -    int err;
> > >> +    int err, off;
> > >>
> > >>    /* Refuse MP_PRIVATE or writable mappings */
> > >>    if (vma->vm_flags & VM_WRITE || vma->vm_flags & VM_EXEC ||
> > >> @@ -7055,14 +7055,14 @@ 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]);
> > >> -        int off = 0;
> > >> -
> > >
> > > I believe we can keep the struct page and off declaration within the while loop.
> >
> > The reason I modified it this way is that, since this loop will always be
> > entered if there are no other issues, these variables will be used in
> > many situations, so I think it is quite inefficient to continue to declare variables
> > in a loop where you don't know how many times it will be repeated.
> > So, I think that declaring variables in advance and then continuously initializing
> > their values is advantageous in terms of performance and there are
> > no other issues. What do you think?
>
> I'm pretty sure the compiler would do the right thing here and no additional
> step would result from declaring both variables inside the loop.

Okay. In that case, I will just remove the variable declaration related patches
and send you the v2 patch right away.

Regards,

Jeongjun Park

>
> >
> > Regards,
> >
> > Jeongjun Park
> >
> > >
> > >>        if (WARN_ON_ONCE(s >= nr_subbufs)) {
> > >>            err = -EINVAL;
> > >>            goto out;
> > >>        }
> > >>
> > >> +        page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
> > >> +        off = 0;
> > >> +
> > >>        for (; off < (1 << (subbuf_order)); off++, page++) {
> > >>            if (p >= nr_pages)
> > >>                break;
> > >> --

Re: [PATCH] ring-buffer: fix array bounds checking

[Steven Rostedt]

On Tue, 17 Dec 2024 22:42:53 +0900
Jeongjun Park <aha310510@gmail.com> wrote:

> > Could you share that reproducer? Or at least the steps. As this situation should
> > never happen a, follow-up fix will be necessary.  
> 
> [1] When tested with a reproducer, pgoff was 8, subbuf_order was 0, and
> subbuf_pages was 1. However, nr_subbufs was 3, so oob-read or uaf occurred.
> 
> [1] : https://syzkaller.appspot.com/text?tag=ReproC&x=14514730580000

This was fixed by Edwards patch.

> Okay. In that case, I will just remove the variable declaration related patches
> and send you the v2 patch right away.
> 

I'm not sure this is needed nor is it a bug.

	while (p < nr_pages) {
		struct page *page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
		int off = 0;

		if (WARN_ON_ONCE(s >= nr_subbufs)) {
			err = -EINVAL;
			goto out;
		}

The WARN_ON_ONCE() suggests that this should never happen. And I believe it shouldn't.

I'm fine if you want to make the change to:

	while (p < nr_pages) {
		struct page *page;
		int off = 0;

		if (WARN_ON_ONCE(s >= nr_subbufs)) {
			err = -EINVAL;
			goto out;
		}

		page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);

But it's not a bug fix. It's simply a cleanup that can wait till the next
merge window.

-- Steve