[PATCH] libv4l: add NULL pointer check

[PATCH] libv4l: add NULL pointer check

[Németh Márton]

From: Márton Németh <nm127@freemail.hu>

Add NULL pointer check before the pointers are dereferenced.

The patch was tested with v4l-test 0.19 [1] together with
"Trust 610 LCD Powerc@m Zoom" in webcam mode.

Reference:
[1] v4l-test: Test environment for Video For Linux Two API
    http://v4l-test.sourceforge.net/

Priority: normal

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
diff -upr libv4l-0.6.1-test.orig/libv4l2/libv4l2.c libv4l-0.6.1-test/libv4l2/libv4l2.c
--- libv4l-0.6.1-test.orig/libv4l2/libv4l2.c	2009-08-20 11:41:15.000000000 +0200
+++ libv4l-0.6.1-test/libv4l2/libv4l2.c	2009-08-30 10:40:12.000000000 +0200
@@ -772,7 +772,8 @@ int v4l2_ioctl (int fd, unsigned long in
       is_capture_request = 1;
       break;
     case VIDIOC_ENUM_FMT:
-      if (((struct v4l2_fmtdesc *)arg)->type == V4L2_BUF_TYPE_VIDEO_CAPTURE &&
+      if (arg &&
+	  ((struct v4l2_fmtdesc *)arg)->type == V4L2_BUF_TYPE_VIDEO_CAPTURE &&
 	  !(devices[index].flags & V4L2_DISABLE_CONVERSION))
 	is_capture_request = 1;
       break;
@@ -782,19 +783,22 @@ int v4l2_ioctl (int fd, unsigned long in
 	is_capture_request = 1;
       break;
     case VIDIOC_TRY_FMT:
-      if (((struct v4l2_format *)arg)->type == V4L2_BUF_TYPE_VIDEO_CAPTURE &&
+      if (arg &&
+	  ((struct v4l2_format *)arg)->type == V4L2_BUF_TYPE_VIDEO_CAPTURE &&
 	  !(devices[index].flags & V4L2_DISABLE_CONVERSION))
 	is_capture_request = 1;
       break;
     case VIDIOC_S_FMT:
     case VIDIOC_G_FMT:
-      if (((struct v4l2_format *)arg)->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+      if (arg &&
+	  ((struct v4l2_format *)arg)->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 	is_capture_request = 1;
 	stream_needs_locking = 1;
       }
       break;
     case VIDIOC_REQBUFS:
-      if (((struct v4l2_requestbuffers *)arg)->type ==
+      if (arg &&
+	  ((struct v4l2_requestbuffers *)arg)->type ==
 	  V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 	is_capture_request = 1;
 	stream_needs_locking = 1;
@@ -803,14 +807,16 @@ int v4l2_ioctl (int fd, unsigned long in
     case VIDIOC_QUERYBUF:
     case VIDIOC_QBUF:
     case VIDIOC_DQBUF:
-      if (((struct v4l2_buffer *)arg)->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+      if (arg &&
+	  ((struct v4l2_buffer *)arg)->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 	is_capture_request = 1;
 	stream_needs_locking = 1;
       }
       break;
     case VIDIOC_STREAMON:
     case VIDIOC_STREAMOFF:
-      if (*((enum v4l2_buf_type *)arg) == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+      if (arg &&
+	  *((enum v4l2_buf_type *)arg) == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 	is_capture_request = 1;
 	stream_needs_locking = 1;
       }
diff -upr libv4l-0.6.1-test.orig/libv4lconvert/control/libv4lcontrol.c libv4l-0.6.1-test/libv4lconvert/control/libv4lcontrol.c
--- libv4l-0.6.1-test.orig/libv4lconvert/control/libv4lcontrol.c	2009-08-20 11:29:51.000000000 +0200
+++ libv4l-0.6.1-test/libv4lconvert/control/libv4lcontrol.c	2009-08-30 10:37:53.000000000 +0200
@@ -543,7 +543,7 @@ int v4lcontrol_vidioc_queryctrl(struct v
   int i;
   struct v4l2_queryctrl *ctrl = arg;
   int retval;
-  __u32 orig_id=ctrl->id;
+  __u32 orig_id;

   /* if we have an exact match return it */
   for (i = 0; i < V4LCONTROL_COUNT; i++)
@@ -556,24 +556,27 @@ int v4lcontrol_vidioc_queryctrl(struct v
   /* find out what the kernel driver would respond. */
   retval = SYS_IOCTL(data->fd, VIDIOC_QUERYCTRL, arg);

-  if ((data->priv_flags & V4LCONTROL_SUPPORTS_NEXT_CTRL) &&
-      (orig_id & V4L2_CTRL_FLAG_NEXT_CTRL)) {
-    /* If the hardware has no more controls check if we still have any
-       fake controls with a higher id then the hardware's highest */
-    if (retval)
-      ctrl->id = V4L2_CTRL_FLAG_NEXT_CTRL;
-
-    /* If any of our controls have an id > orig_id but less than
-       ctrl->id then return that control instead. Note we do not
-       break when we have a match, but keep iterating, so that
-       we end up with the fake ctrl with the lowest CID > orig_id. */
-    for (i = 0; i < V4LCONTROL_COUNT; i++)
-      if ((data->controls & (1 << i)) &&
-	  (fake_controls[i].id > (orig_id & ~V4L2_CTRL_FLAG_NEXT_CTRL)) &&
-	  (fake_controls[i].id <= ctrl->id)) {
-	v4lcontrol_copy_queryctrl(data, ctrl, i);
-	retval = 0;
-      }
+  if (ctrl) {
+    orig_id = ctrl->id;
+    if ((data->priv_flags & V4LCONTROL_SUPPORTS_NEXT_CTRL) &&
+        (orig_id & V4L2_CTRL_FLAG_NEXT_CTRL)) {
+      /* If the hardware has no more controls check if we still have any
+         fake controls with a higher id then the hardware's highest */
+      if (retval)
+        ctrl->id = V4L2_CTRL_FLAG_NEXT_CTRL;
+
+      /* If any of our controls have an id > orig_id but less than
+         ctrl->id then return that control instead. Note we do not
+         break when we have a match, but keep iterating, so that
+         we end up with the fake ctrl with the lowest CID > orig_id. */
+      for (i = 0; i < V4LCONTROL_COUNT; i++)
+        if ((data->controls & (1 << i)) &&
+	    (fake_controls[i].id > (orig_id & ~V4L2_CTRL_FLAG_NEXT_CTRL)) &&
+	    (fake_controls[i].id <= ctrl->id)) {
+	  v4lcontrol_copy_queryctrl(data, ctrl, i);
+	  retval = 0;
+        }
+    }
   }

   return retval;
diff -upr libv4l-0.6.1-test.orig/libv4lconvert/libv4lconvert.c libv4l-0.6.1-test/libv4lconvert/libv4lconvert.c
--- libv4l-0.6.1-test.orig/libv4lconvert/libv4lconvert.c	2009-08-19 15:56:14.000000000 +0200
+++ libv4l-0.6.1-test/libv4lconvert/libv4lconvert.c	2009-08-30 10:45:16.000000000 +0200
@@ -1170,6 +1170,10 @@ static void v4lconvert_get_framesizes(st
 int v4lconvert_enum_framesizes(struct v4lconvert_data *data,
   struct v4l2_frmsizeenum *frmsize)
 {
+  if (!frmsize) {
+      errno = EACCES;
+      return -1;
+  }
   if (!v4lconvert_supported_dst_format(frmsize->pixel_format)) {
     if (v4lconvert_supported_dst_fmt_only(data)) {
       errno = EINVAL;

Re: [PATCH] libv4l: add NULL pointer check

[Laurent Pinchart]

On Sunday 30 August 2009 10:56:16 Németh Márton wrote:
> From: Márton Németh <nm127@freemail.hu>
>
> Add NULL pointer check before the pointers are dereferenced.

Applications are not supposed to pass NULL pointers to those functions. It 
would be an API violation. Instead of silently failing a segfault is better.

> The patch was tested with v4l-test 0.19 [1] together with
> "Trust 610 LCD Powerc@m Zoom" in webcam mode.
>
> Reference:
> [1] v4l-test: Test environment for Video For Linux Two API
>     http://v4l-test.sourceforge.net/
>
> Priority: normal
>
> Signed-off-by: Márton Németh <nm127@freemail.hu>

-- 
Regards,

Laurent Pinchart

Re: [PATCH] libv4l: add NULL pointer check

[Németh Márton]

Laurent Pinchart wrote:
> On Sunday 30 August 2009 10:56:16 Németh Márton wrote:
>> From: Márton Németh <nm127@freemail.hu>
>>
>> Add NULL pointer check before the pointers are dereferenced.
> 
> Applications are not supposed to pass NULL pointers to those functions. It 
> would be an API violation. Instead of silently failing a segfault is better.

Actually we cannot speak about API violation because the behaviour when
passing NULL pointer as ioctl() argument is not defined as of V4L2 API
revision 0.29 available from http://linuxtv.org/hg/v4l-dvb/ . The current
implemention in Linux in kernel space is to return -EACCESS.

I don't really agree with the segfault behaviour, because:

 - currently there is a different behaviour when just using the V4L2
   interface and using the libv4l2 0.6.0. When using the kernel interface
   it is an error in kernelspace if a NULL pointer is dereferenced, thus
   kernel will return -EACCESS. When the libv4l2 0.6.0 is used then the
   behaviour changes: currently there is a segfault instead of getting
   a return value -1 and errno=EACCESS.

 - the segfault normally results that the whole calling process is
   killed. If there is a complex software like a browser, it is not
   very user friendly that the whole software crashes just because an
   implementation error in the V4L2 handling code.

 - currently a lot of V4L2 API ioctls() return -EINVAL or -EFAULT when
   passing NULL pointer as a parameter depending on whether the given
   ioctl is not supported at all or it is supported but there is a problem
   with the passed pointer, respectively. The use case for this would be
   that an application could easily scan for the supported and not supported
   V4L2 ioctls.

 - dereferencing a NULL pointer is not always result segfault, see [1] and
   [2]. So dereferencing a NULL pointer can be treated also as a security
   risk.

 - the patch I sent is only checking the pointer just before it is
   dereferenced. When the libv4l just passes the pointer value to the
   ioctl() then there is no additional check: this situation will be
   handled in kernel space.

These are my arguments. I am open to listen to your arguments.

I think that the final solution could be that the V4L2 API specification
defines what shall happen when NULL pointer is passed as an ioctl() argument.

References:
[1] Jonathan Corbet: Fun with NULL pointers, part 1 (July 20, 2009)
    http://lwn.net/Articles/342330/

[2] Jonathan Corbet: Fun with NULL pointers, part 2
    http://lwn.net/Articles/342420/

Regards,

	Márton Németh

Re: [PATCH] libv4l: add NULL pointer check

[Laurent Pinchart]

On Monday 31 August 2009 07:51:28 Németh Márton wrote:
> Laurent Pinchart wrote:
> > On Sunday 30 August 2009 10:56:16 Németh Márton wrote:
> > > From: Márton Németh <nm127@freemail.hu>
> > >
> > > Add NULL pointer check before the pointers are dereferenced.
> >
> > Applications are not supposed to pass NULL pointers to those functions.
> > It would be an API violation. Instead of silently failing a segfault is
> > better.
>
> Actually we cannot speak about API violation because the behaviour when
> passing NULL pointer as ioctl() argument is not defined as of V4L2 API
> revision 0.29 available from http://linuxtv.org/hg/v4l-dvb/ . The current
> implemention in Linux in kernel space is to return -EACCESS.

Then let's just add a "passing a NULL pointer results in undefined behaviour" 
to the spec.

> I don't really agree with the segfault behaviour, because:
>
>  - currently there is a different behaviour when just using the V4L2
>    interface and using the libv4l2 0.6.0. When using the kernel interface
>    it is an error in kernelspace if a NULL pointer is dereferenced, thus
>    kernel will return -EACCESS. When the libv4l2 0.6.0 is used then the
>    behaviour changes: currently there is a segfault instead of getting
>    a return value -1 and errno=EACCESS.

Right. And I see no problem there. Applications must not pass NULL pointers to 
the V4L2 ioctls. Period.

>  - the segfault normally results that the whole calling process is
>    killed. If there is a complex software like a browser, it is not
>    very user friendly that the whole software crashes just because an
>    implementation error in the V4L2 handling code.

An implementation error in the application. And a pretty serious one, that 
needs to be caught as soon as possible. A segfault will make the error pretty 
obvious.

>  - currently a lot of V4L2 API ioctls() return -EINVAL or -EFAULT when
>    passing NULL pointer as a parameter depending on whether the given
>    ioctl is not supported at all or it is supported but there is a problem
>    with the passed pointer, respectively. The use case for this would be
>    that an application could easily scan for the supported and not
>    supported V4L2 ioctls.

Applications must not do that. The V4L2 spec doesn't prevent side effects to 
calling ioctls with a NULL pointer.

>  - dereferencing a NULL pointer is not always result segfault, see [1] and
>    [2]. So dereferencing a NULL pointer can be treated also as a security
>    risk.

Only if the application explicitly maps a page to virtual address zero on a 
system where the minimum mmap address was set to zero by the administrator. 
Let's be honest, this is a bit akin to make sterile gun bullets to avoid 
infections when someone shots himself in the head. Might be valid in theory, 
but a bit pointless :-)

>  - the patch I sent is only checking the pointer just before it is
>    dereferenced. When the libv4l just passes the pointer value to the
>    ioctl() then there is no additional check: this situation will be
>    handled in kernel space.

Applications must not pass NULL pointer to libv4l, so libv4l simply doesn't 
need to check for that case.

> These are my arguments. I am open to listen to your arguments.
>
> I think that the final solution could be that the V4L2 API specification
> defines what shall happen when NULL pointer is passed as an ioctl()
> argument.
>
> References:
> [1] Jonathan Corbet: Fun with NULL pointers, part 1 (July 20, 2009)
>     http://lwn.net/Articles/342330/
>
> [2] Jonathan Corbet: Fun with NULL pointers, part 2
>     http://lwn.net/Articles/342420/

-- 
Regards,

Laurent Pinchart

Re: [PATCH] libv4l: add NULL pointer check

[Mauro Carvalho Chehab]

Em Mon, 31 Aug 2009 08:52:38 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> >  - dereferencing a NULL pointer is not always result segfault, see [1] and
> >    [2]. So dereferencing a NULL pointer can be treated also as a security
> >    risk.  

>From kernelspace drivers POV, any calls sending a NULL pointer should
result in an error as soon as possible, to avoid any security risks.
Currently, this check is left to the driver, but we should consider
implementing such control globally, at video_ioctl2 and at compat32 layer.

IMHO, libv4l should mimic the driver behavior of returning an error instead of
letting the application to segfault, since, on some critical applications,
like video-surveillance security systems, a segfault could be very bad.



Cheers,
Mauro

Re: [PATCH] libv4l: add NULL pointer check

[Laurent Pinchart]

On Monday 31 August 2009 15:19:32 Mauro Carvalho Chehab wrote:
> Em Mon, 31 Aug 2009 08:52:38 +0200
>
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> > >  - dereferencing a NULL pointer is not always result segfault, see [1]
> > > and [2]. So dereferencing a NULL pointer can be treated also as a
> > > security risk.
>
> From kernelspace drivers POV, any calls sending a NULL pointer should
> result in an error as soon as possible, to avoid any security risks.
> Currently, this check is left to the driver, but we should consider
> implementing such control globally, at video_ioctl2 and at compat32 layer.
>
> IMHO, libv4l should mimic the driver behavior of returning an error instead
> of letting the application to segfault, since, on some critical
> applications, like video-surveillance security systems, a segfault could be
> very bad.

And uncaught errors would be even better. A segfault will be noticed right 
away, while an unhandled error code might slip through to the released 
software. If a security-sensitive application passes a NULL pointer where it 
shouldn't I'd rather see the development machine burst into flames instead of 
silently ignoring the problem.

-- 
Laurent Pinchart

Re: [PATCH] libv4l: add NULL pointer check

[Németh Márton]

Laurent Pinchart wrote:
> On Monday 31 August 2009 15:19:32 Mauro Carvalho Chehab wrote:
>> Em Mon, 31 Aug 2009 08:52:38 +0200
>>
>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
>>>>  - dereferencing a NULL pointer is not always result segfault, see [1]
>>>> and [2]. So dereferencing a NULL pointer can be treated also as a
>>>> security risk.
>> From kernelspace drivers POV, any calls sending a NULL pointer should
>> result in an error as soon as possible, to avoid any security risks.
>> Currently, this check is left to the driver, but we should consider
>> implementing such control globally, at video_ioctl2 and at compat32 layer.
>>
>> IMHO, libv4l should mimic the driver behavior of returning an error instead
>> of letting the application to segfault, since, on some critical
>> applications, like video-surveillance security systems, a segfault could be
>> very bad.
> 
> And uncaught errors would be even better. A segfault will be noticed right 
> away, while an unhandled error code might slip through to the released 
> software. If a security-sensitive application passes a NULL pointer where it 
> shouldn't I'd rather see the development machine burst into flames instead of 
> silently ignoring the problem.

I have an example. Let's imagine the following code:

    struct v4l2_capability* cap;

    cap = malloc(sizeof(*cap));
    ret = ioctl(f, VIDIOC_QUERYCAP, cap);
    if (ret == -1) {
        /* error handling */
    }

Does this code contain implementation problem? Yes, the value of cap should
be checked whether it is NULL or not.

Will this code cause problem? Most of the time not, only in case of low
memory condition, thus this implementation problem will usually not detected
if the ioctl() caused segfault on NULL pointers.

One more thing I would like to mention on this topic. This is coming from
the C language which does not contain structured exception handling as for
example Java has with its exception handling capability. The usual way to
signal errors is through the return value. This is what a C programmer learns
and this is what she or he expects. The signals as segfault is out of the
scope of the C language.

Regards,

	Márton Németh

Re: [PATCH] libv4l: add NULL pointer check

[Hans de Goede]

Hi,

On 08/30/2009 10:56 AM, Németh Márton wrote:
> From: Márton Németh<nm127@freemail.hu>
>
> Add NULL pointer check before the pointers are dereferenced.
>
> The patch was tested with v4l-test 0.19 [1] together with
> "Trust 610 LCD Powerc@m Zoom" in webcam mode.
>

http://linuxtv.org/hg/~hgoede/libv4l/rev/c51a90c0f62f

Regards,

hans