[Bug Report] drivers/video/fbdev/kyro/fbdev.c: unsigned integer wrap-around might cause unexpected behavior

[Changming Liu]

> From: Bartlomiej Zolnierkiewicz <b.zolnierkie at samsung.com>
> Sent: Tuesday, June 9, 2020 6:44 AM
> To: Changming Liu <liu.changm at northeastern.edu>
> Cc: linux-fbdev at vger.kernel.org; dri-devel at lists.freedesktop.org; Lu, Long
> <l.lu at northeastern.edu>; yaohway at gmail.com
> Subject: Re: [Bug Report] drivers/video/fbdev/kyro/fbdev.c: unsigned
> integer wrap-around might cause unexpected behavior
> 
> 
> Hi,
> 
> On 5/21/20 3:15 AM, Changming Liu wrote:
> > Hi Bartlomiej,
> > Greetings, I'm a first-year PhD student who is interested in the usage of
> UBSan for linux.
> > And after some experiments, I found that in
> > drivers/video/fbdev/kyro/fbdev.c function
> kyro_dev_overlay_viewport_set, there is an unsigned integer overflow that
> might cause unexpected behavior.
> >
> > More specifically, first at its caller, kyrofb_ioctl, after execution of
> copy_from_user at line 599, struct ol_viewport_set is filled with data from
> user space.
> > And the 4 32bit unsigned integers from it are passed into
> > kyro_dev_overlay_viewport_set. In function
> kyro_dev_overlay_viewport_set, x is added with ulWidth, y is added with
> ulHeight to transfer the length to the coordinate.
> > And the result coordinate might overflow and wrap around. And it is
> passed into function SetOverlayViewPort.
> >
> > It appears that in function SetOverlayViewPort, these values are treated as
> the coordinate of the bottom-right point and the wrap-around is not
> checked.(I might miss something).
> >
> > Due to the lack of knowledge of the interaction between this module and
> the user space, I'm not able to assess if this is a benign wrap-around or
> whether the wrap-around could happen at all.
> > I'd appreciate for you comment on this issue, this could help me
> understand linux and unsigned wrap around a lot.
> >
> > Looking forward to your valuable response!
> 
> It seems that wrap-around can indeed happen but I'm not sure what are the
> exact consequences of it (SetOverlayViewPort() is quite complicated and I
> also don't know how hardware would react to improper settings).
> 
> kyrofb driver is for legacy devices and is not actively maintained so I worry
> that without somebody with the access to hardware and time to investigate
> it further I cannot do much about the problem.
> 
Thanks for the comments! These are valuable observations which show that
hardware-driver interaction can play a role in unsigned wrap-around here.
Indeed, there is no evidence to determine the wrap around is benign or not.
Since these are just for legacy devices, I too would not take the risk to fix sth
which is not broken.

Thanks again for your feedback, I learned a lot.

Best,
Changming

> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 
> > Best,
> > Changming Liu
> >