> Vincent Donnefort <vdonnef...@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-988b81fcc...@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 <aha310...@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;
>> --
