Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly
Laurent Pinchart wrote: > Hi Mauro, > > On Wednesday 10 June 2009 23:58:31 Laurent Pinchart wrote: >> On Wednesday 10 June 2009 15:53:57 Mauro Carvalho Chehab wrote: >>> Em Wed, 10 Jun 2009 10:52:28 -0300 >>> >>> Mauro Carvalho Chehab escreveu: Em Mon, 25 May 2009 11:16:34 -0300 Mauro Carvalho Chehab escreveu: > Em Mon, 25 May 2009 13:17:02 +0200 > > Laurent Pinchart escreveu: >> Hi everybody, >> >> Márton Németh found an integer overflow bug in the extended control >> ioctl handling code. This affects both video_usercopy and >> video_ioctl2. See http://bugzilla.kernel.org/show_bug.cgi?id=13357 >> for a detailed description of the problem. >> >> >> Restricting v4l2_ext_controls::count to values smaller than >> KMALLOC_MAX_SIZE / sizeof(struct v4l2_ext_control) should be >> enough, but we might want to restrict the value even further. I'd >> like opinions on this. > Seems fine to my eyes, but being so close to kmalloc size doesn't > seem to be a good idea. It seems better to choose an arbitrary size > big enough to handle all current needs. I'll apply the current version, but I still think we should restrict it to a lower value. >>> Hmm... SOB is missing. Márton and Laurent, could you please sign it >> Signed-off-by: Laurent Pinchart > > Márton reminded me that the patch has still not been applied. > > Please replace the above SOB line with > > Signed-off-by: Laurent Pinchart Tested-by: Márton Németh -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly
Hi Mauro, On Wednesday 10 June 2009 23:58:31 Laurent Pinchart wrote: > On Wednesday 10 June 2009 15:53:57 Mauro Carvalho Chehab wrote: > > Em Wed, 10 Jun 2009 10:52:28 -0300 > > > > Mauro Carvalho Chehab escreveu: > > > Em Mon, 25 May 2009 11:16:34 -0300 > > > > > > Mauro Carvalho Chehab escreveu: > > > > Em Mon, 25 May 2009 13:17:02 +0200 > > > > > > > > Laurent Pinchart escreveu: > > > > > Hi everybody, > > > > > > > > > > Márton Németh found an integer overflow bug in the extended control > > > > > ioctl handling code. This affects both video_usercopy and > > > > > video_ioctl2. See http://bugzilla.kernel.org/show_bug.cgi?id=13357 > > > > > for a detailed description of the problem. > > > > > > > > > > > > > > > Restricting v4l2_ext_controls::count to values smaller than > > > > > KMALLOC_MAX_SIZE / sizeof(struct v4l2_ext_control) should be > > > > > enough, but we might want to restrict the value even further. I'd > > > > > like opinions on this. > > > > > > > > Seems fine to my eyes, but being so close to kmalloc size doesn't > > > > seem to be a good idea. It seems better to choose an arbitrary size > > > > big enough to handle all current needs. > > > > > > I'll apply the current version, but I still think we should restrict it > > > to a lower value. > > > > Hmm... SOB is missing. Márton and Laurent, could you please sign it > > Signed-off-by: Laurent Pinchart Márton reminded me that the patch has still not been applied. Please replace the above SOB line with Signed-off-by: Laurent Pinchart -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly
On Wednesday 10 June 2009 15:53:57 Mauro Carvalho Chehab wrote: > Em Wed, 10 Jun 2009 10:52:28 -0300 > > Mauro Carvalho Chehab escreveu: > > Em Mon, 25 May 2009 11:16:34 -0300 > > > > Mauro Carvalho Chehab escreveu: > > > Em Mon, 25 May 2009 13:17:02 +0200 > > > > > > Laurent Pinchart escreveu: > > > > Hi everybody, > > > > > > > > Márton Németh found an integer overflow bug in the extended control > > > > ioctl handling code. This affects both video_usercopy and > > > > video_ioctl2. See http://bugzilla.kernel.org/show_bug.cgi?id=13357 > > > > for a detailed description of the problem. > > > > > > > > > > > > Restricting v4l2_ext_controls::count to values smaller than > > > > KMALLOC_MAX_SIZE / sizeof(struct v4l2_ext_control) should be enough, > > > > but we might want to restrict the value even further. I'd like > > > > opinions on this. > > > > > > Seems fine to my eyes, but being so close to kmalloc size doesn't seem > > > to be a good idea. It seems better to choose an arbitrary size big > > > enough to handle all current needs. > > > > I'll apply the current version, but I still think we should restrict it > > to a lower value. > > Hmm... SOB is missing. Márton and Laurent, could you please sign it Signed-off-by: Laurent Pinchart Cheers, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly
Mauro Carvalho Chehab wrote: > Em Wed, 10 Jun 2009 10:52:28 -0300 > Mauro Carvalho Chehab escreveu: > >> Em Mon, 25 May 2009 11:16:34 -0300 >> Mauro Carvalho Chehab escreveu: >> >>> Em Mon, 25 May 2009 13:17:02 +0200 >>> Laurent Pinchart escreveu: >>> Hi everybody, Márton Németh found an integer overflow bug in the extended control ioctl handling code. This affects both video_usercopy and video_ioctl2. See http://bugzilla.kernel.org/show_bug.cgi?id=13357 for a detailed description of the problem. Restricting v4l2_ext_controls::count to values smaller than KMALLOC_MAX_SIZE / sizeof(struct v4l2_ext_control) should be enough, but we might want to restrict the value even further. I'd like opinions on this. >>> Seems fine to my eyes, but being so close to kmalloc size doesn't seem to >>> be a >>> good idea. It seems better to choose an arbitrary size big enough to handle >>> all current needs. >> I'll apply the current version, but I still think we should restrict it to a >> lower value. > > > Hmm... SOB is missing. Márton and Laurent, could you please sign it As I wrote at http://bugzilla.kernel.org/show_bug.cgi?id=13357#c6 : Tested-by: Márton Németh Regards, Márton Németh -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly
Em Wed, 10 Jun 2009 10:52:28 -0300 Mauro Carvalho Chehab escreveu: > Em Mon, 25 May 2009 11:16:34 -0300 > Mauro Carvalho Chehab escreveu: > > > Em Mon, 25 May 2009 13:17:02 +0200 > > Laurent Pinchart escreveu: > > > > > Hi everybody, > > > > > > Márton Németh found an integer overflow bug in the extended control ioctl > > > handling code. This affects both video_usercopy and video_ioctl2. See > > > http://bugzilla.kernel.org/show_bug.cgi?id=13357 for a detailed > > > description of > > > the problem. > > > > > > > > Restricting v4l2_ext_controls::count to values smaller than > > > KMALLOC_MAX_SIZE / > > > sizeof(struct v4l2_ext_control) should be enough, but we might want to > > > restrict the value even further. I'd like opinions on this. > > > > Seems fine to my eyes, but being so close to kmalloc size doesn't seem to > > be a > > good idea. It seems better to choose an arbitrary size big enough to handle > > all current needs. > > I'll apply the current version, but I still think we should restrict it to a > lower value. Hmm... SOB is missing. Márton and Laurent, could you please sign it Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly
Em Mon, 25 May 2009 11:16:34 -0300 Mauro Carvalho Chehab escreveu: > Em Mon, 25 May 2009 13:17:02 +0200 > Laurent Pinchart escreveu: > > > Hi everybody, > > > > Márton Németh found an integer overflow bug in the extended control ioctl > > handling code. This affects both video_usercopy and video_ioctl2. See > > http://bugzilla.kernel.org/show_bug.cgi?id=13357 for a detailed description > > of > > the problem. > > > > > Restricting v4l2_ext_controls::count to values smaller than > > KMALLOC_MAX_SIZE / > > sizeof(struct v4l2_ext_control) should be enough, but we might want to > > restrict the value even further. I'd like opinions on this. > > Seems fine to my eyes, but being so close to kmalloc size doesn't seem to be a > good idea. It seems better to choose an arbitrary size big enough to handle > all current needs. I'll apply the current version, but I still think we should restrict it to a lower value. Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly
Laurent Pinchart worte: > Could a very large number of control requests be used as a DoS attack vector > ? > A userspace application could kmalloc large amounts of memory without any > restriction. Memory would be reclaimed eventually, but after performing a > large number of USB requests, which could take quite a long time. A DoS attacker could open the /dev/video0 several times even from one single process (from different threads) and could kmalloc() as much memory as the attacker wants. Maybe even one file descriptor would be enough using it from different threads. This could force the system to swap out pages to get the necessary memory. I don't know if more than one instance of the VIDIOC_G_EXT_CTRLS requests can actively keep memory allocated or only one can run at a time forcing the other requests to sleep until the previous one hadn't been finished. This is also true for VIDIOC_S_EXT_CTRLS and VIDIOC_TRY_EXT_CTRLS. Regards, Márton Németh -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC, PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly
> On Monday 25 May 2009 21:22:06 Trent Piepho wrote:
>> On Mon, 25 May 2009, Laurent Pinchart wrote:
>> > diff -r e0d881b21bc9 linux/drivers/media/video/v4l2-ioctl.c
>> > --- a/linux/drivers/media/video/v4l2-ioctl.c Tue May 19 15:12:17 2009
>> > +0200 +++ b/linux/drivers/media/video/v4l2-ioctl.c Sun May 24 18:26:29
>> > 2009 +0200 @@ -402,6 +402,10 @@
>> > a specific control that caused it. */
>> >p->error_idx = p->count;
>> >user_ptr = (void __user *)p->controls;
>> > + if (p->count > KMALLOC_MAX_SIZE / sizeof(p->controls[0])) {
>> > + err = -ENOMEM;
>> > + goto out_ext_ctrl;
>> > + }
>> >if (p->count) {
>> >ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
>> >/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is
>> > still NULL.
>> */
>> > @@ -1859,6 +1863,10 @@
>> > a specific control that caused it. */
>> >p->error_idx = p->count;
>> >user_ptr = (void __user *)p->controls;
>> > + if (p->count > KMALLOC_MAX_SIZE / sizeof(p->controls[0])) {
>> > + err = -ENOMEM;
>> > + goto out_ext_ctrl;
>> > + }
>> >if (p->count) {
>> >ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
>> >/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is
>> > still NULL.
>> */
>> >
>> > Restricting v4l2_ext_controls::count to values smaller than
>> > KMALLOC_MAX_SIZE / sizeof(struct v4l2_ext_control) should be enough,
>> but
>> > we might want to restrict the value even further. I'd like opinions on
>> > this.
>>
>> One thing that could be done is to call access_ok() on the range before
>> kmalloc'ing a buffer. If p->count is too high, then it's possible that
>> the
>> copy_from_user will fail because the process does not have the address
>> space to copy.
>
> arch/x86/include/asm/uaccess.h, about access_ok():
>
> * Note that, depending on architecture, this function probably just
> * checks that the pointer is in the user space range - after calling
> * this function, memory access functions may still return -EFAULT.
>
> I don't think it's worth it. Let's just kmalloc (or kzalloc) and
> copy_from_user. If one of them fails we'll return an error.
>
> Could a very large number of control requests be used as a DoS attack
> vector ?
> A userspace application could kmalloc large amounts of memory without any
> restriction. Memory would be reclaimed eventually, but after performing a
> large number of USB requests, which could take quite a long time.
Perhaps we should limit the number of controls to a maximum of 1024. That
should really be enough :-)
I'm OK with such a limitation.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly
On Monday 25 May 2009 21:22:06 Trent Piepho wrote:
> On Mon, 25 May 2009, Laurent Pinchart wrote:
> > diff -r e0d881b21bc9 linux/drivers/media/video/v4l2-ioctl.c
> > --- a/linux/drivers/media/video/v4l2-ioctl.cTue May 19 15:12:17 2009
> > +0200 +++ b/linux/drivers/media/video/v4l2-ioctl.c Sun May 24 18:26:29
> > 2009 +0200 @@ -402,6 +402,10 @@
> >a specific control that caused it. */
> > p->error_idx = p->count;
> > user_ptr = (void __user *)p->controls;
> > + if (p->count > KMALLOC_MAX_SIZE / sizeof(p->controls[0])) {
> > + err = -ENOMEM;
> > + goto out_ext_ctrl;
> > + }
> > if (p->count) {
> > ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
> > /* Note: v4l2_ext_controls fits in sbuf[] so mbuf is
> > still NULL. */
> > @@ -1859,6 +1863,10 @@
> >a specific control that caused it. */
> > p->error_idx = p->count;
> > user_ptr = (void __user *)p->controls;
> > + if (p->count > KMALLOC_MAX_SIZE / sizeof(p->controls[0])) {
> > + err = -ENOMEM;
> > + goto out_ext_ctrl;
> > + }
> > if (p->count) {
> > ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
> > /* Note: v4l2_ext_controls fits in sbuf[] so mbuf is
> > still NULL. */
> >
> > Restricting v4l2_ext_controls::count to values smaller than
> > KMALLOC_MAX_SIZE / sizeof(struct v4l2_ext_control) should be enough, but
> > we might want to restrict the value even further. I'd like opinions on
> > this.
>
> One thing that could be done is to call access_ok() on the range before
> kmalloc'ing a buffer. If p->count is too high, then it's possible that the
> copy_from_user will fail because the process does not have the address
> space to copy.
arch/x86/include/asm/uaccess.h, about access_ok():
* Note that, depending on architecture, this function probably just
* checks that the pointer is in the user space range - after calling
* this function, memory access functions may still return -EFAULT.
I don't think it's worth it. Let's just kmalloc (or kzalloc) and
copy_from_user. If one of them fails we'll return an error.
Could a very large number of control requests be used as a DoS attack vector ?
A userspace application could kmalloc large amounts of memory without any
restriction. Memory would be reclaimed eventually, but after performing a
large number of USB requests, which could take quite a long time.
Best regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly
On Mon, 25 May 2009, Laurent Pinchart wrote:
> diff -r e0d881b21bc9 linux/drivers/media/video/v4l2-ioctl.c
> --- a/linux/drivers/media/video/v4l2-ioctl.c Tue May 19 15:12:17 2009 +0200
> +++ b/linux/drivers/media/video/v4l2-ioctl.c Sun May 24 18:26:29 2009 +0200
> @@ -402,6 +402,10 @@
> a specific control that caused it. */
> p->error_idx = p->count;
> user_ptr = (void __user *)p->controls;
> + if (p->count > KMALLOC_MAX_SIZE / sizeof(p->controls[0])) {
> + err = -ENOMEM;
> + goto out_ext_ctrl;
> + }
> if (p->count) {
> ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
> /* Note: v4l2_ext_controls fits in sbuf[] so mbuf is
> still NULL. */
> @@ -1859,6 +1863,10 @@
> a specific control that caused it. */
> p->error_idx = p->count;
> user_ptr = (void __user *)p->controls;
> + if (p->count > KMALLOC_MAX_SIZE / sizeof(p->controls[0])) {
> + err = -ENOMEM;
> + goto out_ext_ctrl;
> + }
> if (p->count) {
> ctrls_size = sizeof(struct v4l2_ext_control) * p->count;
> /* Note: v4l2_ext_controls fits in sbuf[] so mbuf is
> still NULL. */
>
> Restricting v4l2_ext_controls::count to values smaller than KMALLOC_MAX_SIZE /
> sizeof(struct v4l2_ext_control) should be enough, but we might want to
> restrict the value even further. I'd like opinions on this.
One thing that could be done is to call access_ok() on the range before
kmalloc'ing a buffer. If p->count is too high, then it's possible that the
copy_from_user will fail because the process does not have the address
space to copy.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly
Em Mon, 25 May 2009 13:17:02 +0200 Laurent Pinchart escreveu: > Hi everybody, > > Márton Németh found an integer overflow bug in the extended control ioctl > handling code. This affects both video_usercopy and video_ioctl2. See > http://bugzilla.kernel.org/show_bug.cgi?id=13357 for a detailed description > of > the problem. > > Restricting v4l2_ext_controls::count to values smaller than KMALLOC_MAX_SIZE / > sizeof(struct v4l2_ext_control) should be enough, but we might want to > restrict the value even further. I'd like opinions on this. Seems fine to my eyes, but being so close to kmalloc size doesn't seem to be a good idea. It seems better to choose an arbitrary size big enough to handle all current needs. Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
