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

[Harshit Mogalapalli]

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@xxxxxxxxxx>
> > Closes: 
> > https://lore.kernel.org/linux-fbdev/271372d6-e665-4e7f-b088-dee5f4ab341a@xxxxxxxxxx
> > Fixes: 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref struct")
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Nam Cao <namcao@xxxxxxxxxxxxx>
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

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