linux-kernel - Re: [PATCH v2] fbdev: fix incorrect address computation in deferred IO

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Tue, 23 Apr 2024 18:56:52 +0530
From: Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>
To: Thomas Zimmermann <tzimmermann@...e.de>, Nam Cao <namcao@...utronix.de>,
        Jaya Kumar <jayalk@...works.biz>, Daniel Vetter <daniel@...ll.ch>,
        Helge Deller <deller@....de>,
        Javier Martinez Canillas <javierm@...hat.com>,
        linux-fbdev@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Cc: tiwai@...e.de, bigeasy@...utronix.de, patrik.r.jakobsson@...il.com,
        Vegard Nossum <vegard.nossum@...cle.com>,
        George Kennedy <george.kennedy@...cle.com>,
        Darren Kenny <darren.kenny@...cle.com>, chuansheng.liu@...el.com,
        stable@...r.kernel.org
Subject: Re: [PATCH v2] fbdev: fix incorrect address computation in deferred
 IO

Hi,

On 23/04/24 18:37, Thomas Zimmermann wrote:
> Hi
> 
> Am 23.04.24 um 13:50 schrieb Nam Cao:
>> With deferred IO enabled, a page fault happens when data is written to 
>> the
>> framebuffer device. Then driver determines which page is being updated by
>> calculating the offset of the written virtual address within the virtual
>> memory area, and uses this offset to get the updated page within the
>> internal buffer. This page is later copied to hardware (thus the name
>> "deferred IO").
>>
>> This offset calculation is only correct if the virtual memory area is
>> mapped to the beginning of the internal buffer. Otherwise this is wrong.
>> For example, if users do:
>>      mmap(ptr, 4096, PROT_WRITE, MAP_FIXED | MAP_SHARED, fd, 0xff000);
>>
>> Then the virtual memory area will mapped at offset 0xff000 within the
>> internal buffer. This offset 0xff000 is not accounted for, and wrong page
>> is updated.
>>
>> Correct the calculation by using vmf->pgoff instead. With this change, 
>> the
>> variable "offset" will no longer hold the exact offset value, but it is
>> rounded down to multiples of PAGE_SIZE. But this is still correct, 
>> because
>> this variable is only used to calculate the page offset.
>>
>> Reported-by: Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>
>> Closes: 
>> https://lore.kernel.org/linux-fbdev/271372d6-e665-4e7f-b088-dee5f4ab341a@oracle.com
>> Fixes: 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref struct")
>> Cc: <stable@...r.kernel.org>
>> Signed-off-by: Nam Cao <namcao@...utronix.de>
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@...e.de>
> 

Thanks everyone!

I have tested the patched kernel with the syzkaller reproducer and 
couldn't see a problem.

Regards,
Harshit

> Thank you so much. I'll take care of merging the patch later this week.
> 
> Best regards
> Thomas
> 
>> ---
>> v2:
>>    - simplify the patch by using vfg->pgoff
>>    - remove tested-by tag, as the patch is now different
>>
>>   drivers/video/fbdev/core/fb_defio.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/core/fb_defio.c 
>> b/drivers/video/fbdev/core/fb_defio.c
>> index 1ae1d35a5942..b9607d5a370d 100644
>> --- a/drivers/video/fbdev/core/fb_defio.c
>> +++ b/drivers/video/fbdev/core/fb_defio.c
>> @@ -196,7 +196,7 @@ static vm_fault_t fb_deferred_io_track_page(struct 
>> fb_info *info, unsigned long
>>    */
>>   static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, 
>> struct vm_fault *vmf)
>>   {
>> -    unsigned long offset = vmf->address - vmf->vma->vm_start;
>> +    unsigned long offset = vmf->pgoff << PAGE_SHIFT;
>>       struct page *page = vmf->page;
>>       file_update_time(vmf->vma->vm_file);
>