* [PATCH] media:usb:cpia2: Properly check framebuffer mmap offsets @ 2019-11-08 21:50 Omer Shalev 2019-11-08 20:49 ` Greg Kroah-Hartman 0 siblings, 1 reply; 7+ messages in thread From: Omer Shalev @ 2019-11-08 21:50 UTC (permalink / raw) Cc: omerdeshalev, Mauro Carvalho Chehab, Greg Kroah-Hartman, Kate Stewart, Richard Fontana, Allison Randal, Thomas Gleixner, linux-media, linux-kernel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] media:usb:cpia2: Properly check framebuffer mmap offsets 2019-11-08 21:50 [PATCH] media:usb:cpia2: Properly check framebuffer mmap offsets Omer Shalev @ 2019-11-08 20:49 ` Greg Kroah-Hartman 2019-11-09 11:39 ` Hans Verkuil 0 siblings, 1 reply; 7+ messages in thread From: Greg Kroah-Hartman @ 2019-11-08 20:49 UTC (permalink / raw) To: Omer Shalev Cc: Mauro Carvalho Chehab, Kate Stewart, Richard Fontana, Allison Randal, Thomas Gleixner, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media:usb:cpia2: Properly check framebuffer mmap offsets 2019-11-08 20:49 ` Greg Kroah-Hartman @ 2019-11-09 11:39 ` Hans Verkuil 2019-11-11 11:46 ` Greg Kroah-Hartman 0 siblings, 1 reply; 7+ messages in thread From: Hans Verkuil @ 2019-11-09 11:39 UTC (permalink / raw) To: Greg Kroah-Hartman, Omer Shalev Cc: Mauro Carvalho Chehab, Kate Stewart, Richard Fontana, Allison Randal, Thomas Gleixner, linux-media, linux-kernel 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media:usb:cpia2: Properly check framebuffer mmap offsets 2019-11-09 11:39 ` Hans Verkuil @ 2019-11-11 11:46 ` Greg Kroah-Hartman 2019-11-11 18:24 ` Omer Shalev 0 siblings, 1 reply; 7+ messages in thread From: Greg Kroah-Hartman @ 2019-11-11 11:46 UTC (permalink / raw) To: Hans Verkuil Cc: Omer Shalev, Mauro Carvalho Chehab, Kate Stewart, Richard Fontana, Allison Randal, Thomas Gleixner, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media:usb:cpia2: Properly check framebuffer mmap offsets 2019-11-11 11:46 ` Greg Kroah-Hartman @ 2019-11-11 18:24 ` Omer Shalev 2019-11-11 16:29 ` Greg Kroah-Hartman 0 siblings, 1 reply; 7+ messages in thread From: Omer Shalev @ 2019-11-11 18:24 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Hans Verkuil, Mauro Carvalho Chehab, Kate Stewart, Richard Fontana, Allison Randal, Thomas Gleixner, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media:usb:cpia2: Properly check framebuffer mmap offsets 2019-11-11 18:24 ` Omer Shalev @ 2019-11-11 16:29 ` Greg Kroah-Hartman 2019-11-11 18:53 ` Omer Shalev 0 siblings, 1 reply; 7+ messages in thread From: Greg Kroah-Hartman @ 2019-11-11 16:29 UTC (permalink / raw) To: Omer Shalev Cc: Hans Verkuil, Mauro Carvalho Chehab, Kate Stewart, Richard Fontana, Allison Randal, Thomas Gleixner, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media:usb:cpia2: Properly check framebuffer mmap offsets 2019-11-11 16:29 ` Greg Kroah-Hartman @ 2019-11-11 18:53 ` Omer Shalev 0 siblings, 0 replies; 7+ messages in thread From: Omer Shalev @ 2019-11-11 18:53 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Hans Verkuil, Mauro Carvalho Chehab, Kate Stewart, Richard Fontana, Allison Randal, Thomas Gleixner, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-11-11 16:53 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-08 21:50 [PATCH] media:usb:cpia2: Properly check framebuffer mmap offsets Omer Shalev 2019-11-08 20:49 ` Greg Kroah-Hartman 2019-11-09 11:39 ` Hans Verkuil 2019-11-11 11:46 ` Greg Kroah-Hartman 2019-11-11 18:24 ` Omer Shalev 2019-11-11 16:29 ` Greg Kroah-Hartman 2019-11-11 18:53 ` Omer Shalev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).