Re: [PATCH] media:usb:cpia2: Properly check framebuffer mmap offsets

[Greg Kroah-Hartman]

On Mon, Nov 11, 2019 at 06:24:42PM +0000, Omer Shalev wrote:
> On Mon, Nov 11, 2019 at 12:46:15PM +0100, Greg Kroah-Hartman wrote:
> > On Sat, Nov 09, 2019 at 12:39:43PM +0100, Hans Verkuil wrote:
> > > Hi Greg,
> > > 
> > > On 11/8/19 9:49 PM, Greg Kroah-Hartman wrote:
> > > > On Fri, Nov 08, 2019 at 09:50:36PM +0000, Omer Shalev wrote:
> > > >> The cpai2 driver's mmap implementation wasn't properly check for all
> > > >> possible offset values. Given a huge offset value , the calculation
> > > >> start_offset + size can wrap around to a low value and pass the check
> > > > 
> > > > I thought we checked that in the core of the kernel now, to keep all
> > > > drivers from not having to do this type of thing (as they obviously all
> > > > forgot to.)  Why is this still needed here as well?
> > > 
> > > Where is that checked in the core? I couldn't find anything, but I might
> > > have been looking in the wrong place.
> > 
> > Sorry, took me a while to find it.  Look at be83bbf80682 ("mmap:
> > introduce sane default mmap limits") as I think this should handle the
> > problem already.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Thanks Greg. But All other drivers I've seen implement it like that: if(size > total_size || offset >
> total_size - size). Which I think, is a better way to write this code, and generally more
> secure. Plus, no extra code is needed (just changing this line).

The point of the above commit that is in the tree is that no driver has
to do this check at all, it's already been done before the driver ever
gets called, right?

So yes, there's lots of history of drivers doing the check themselves
(and getting it wrong as you point out), but that should not matter
anymore.

Can you verify that your change isn't even needed due to the above
mentioned core check for valid values?

thanks,

greg k-h