LKML Archive on lore.kernel.org
 help / color / Atom feed
* 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

* [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)
  To: unlisted-recipients:; (no To-header on input)
  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	[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 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 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 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, back to index

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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git