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

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

[Greg Kroah-Hartman]

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?

thanks,

greg k-h

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

[Omer Shalev]

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

Signed-off-by: Omer Shalev <omerdeshalev@gmail.com>
---
 drivers/media/usb/cpia2/cpia2_core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/cpia2/cpia2_core.c b/drivers/media/usb/cpia2/cpia2_core.c
index 20c50c2d042e..9d621cfb2d74 100644
--- a/drivers/media/usb/cpia2/cpia2_core.c
+++ b/drivers/media/usb/cpia2/cpia2_core.c
@@ -2390,18 +2390,22 @@ int cpia2_remap_buffer(struct camera_data *cam, struct vm_area_struct *vma)
 {
 	const char *adr = (const char *)vma->vm_start;
 	unsigned long size = vma->vm_end-vma->vm_start;
-	unsigned long start_offset = vma->vm_pgoff << PAGE_SHIFT;
 	unsigned long start = (unsigned long) adr;
+	unsigned long start_offset;
 	unsigned long page, pos;
 
 	DBG("mmap offset:%ld size:%ld\n", start_offset, size);
 
 	if (!video_is_registered(&cam->vdev))
 		return -ENODEV;
+
+	if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
+		return -EINVAL;
 
+	start_offset = vma->vm_pgoff << PAGE_SHIFT;
 	if (size > cam->frame_size*cam->num_frames  ||
 	    (start_offset % cam->frame_size) != 0 ||
-	    (start_offset+size > cam->frame_size*cam->num_frames))
+	    (start_offset > cam->frame_size*cam->num_frames - size))
 		return -EINVAL;
 
 	pos = ((unsigned long) (cam->frame_buffer)) + start_offset;
-- 
2.23.0

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

[Hans Verkuil]

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.

Regards,

	Hans

> 
> thanks,
> 
> greg k-h
> 

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

[Greg Kroah-Hartman]

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

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

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

[Omer Shalev]

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

Please let me know what you think.

Best regards,

Omer

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

[Omer Shalev]

On Mon, Nov 11, 2019 at 05:29:07PM +0100, Greg Kroah-Hartman wrote:
> 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

Yes I got it , and thanks again. I think that programmatically , its
better to write that this way, And therefore I suggested this patch. 

thanks,

Omer