| Message ID | 4A9A3EB0.8060304@freemail.hu (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
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>
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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/
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;