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

[Jeongjun Park <aha310510@gmail.com>]

> 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;
>> --