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

[Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>]

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@oracle.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@vger.kernel.org>
>> Signed-off-by: Nam Cao <namcao@linutronix.de>
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.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);
>