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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <24508411-0980-43EE-8224-C3B81E456AFF@gmail.com>
Date: Tue, 17 Dec 2024 10:28:49 +0900
From: Jeongjun Park <aha310510@...il.com>
To: Vincent Donnefort <vdonnefort@...gle.com>
Cc: rostedt@...dmis.org, mhiramat@...nel.org, mathieu.desnoyers@...icios.com,
 david@...hat.com, linux-kernel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH] ring-buffer: fix array bounds checking



> Vincent Donnefort <vdonnefort@...gle.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@...il.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;
>> --