Re: [RFC][PATCH] media: v4l2-ctrls: add more NULL pointer checks

[Ezequiel Garcia]

Hi Sergey,

Thanks for the patch!

On Fri, 10 Apr 2020 at 07:35, Sergey Senozhatsky
<sergey.senozhatsky@xxxxxxxxx> wrote:
>
> A number of v4l2-ctrls functions gracefully handle NULL ctrl pointers,
> for instance, v4l2_g_ctrl(), v4l2_ctrl_activate(), __v4l2_ctrl_grab()

Please note that v4l2_g_ctrl doesn't really handle
a NULL ctrl parameter, because it doesn't have a ctrl
parameter :-)

Checking the return of a function such as v4l2_ctrl_find,
is not the same as defensive-style parameter checking.

And the thing is, the kernel doesn't really do defensive checking
like this on internal APIs, unless there are good reasons
allowing a NULL parameter, such as kfree.

Now, maybe this is the case, maybe it should be possible
to add controls without checking the result, or to allow
calling the control API with a NULL ctrl.

Quite frankly, I'm not convinced of this being the case,
or just a quirk of the vivid driver.

In any case...

> to name a few. But not all of them. It is relatively easy to crash the
> kernel with the NULL pointer dereference:
>
>         # modprobe vivid node_types=0x60000
>         $ v4l2-compliance
>
> BUG: kernel NULL pointer dereference, address: 0000000000000020
> PF: supervisor read access in kernel mode
> PF: error_code(0x0000) - not-present page
> PGD 0 P4D 0
> Oops: 0000 [#1] SMP PTI
> RIP: 0010:v4l2_ctrl_s_ctrl.isra.0+0x4/0x30 [vivid]
> Call Trace:
>  vidioc_s_input.cold+0x1a8/0x38d [vivid]
>  __video_do_ioctl+0x372/0x3a0 [videodev]
>  ? v4l_enumstd+0x20/0x20 [videodev]
>  ? v4l_enumstd+0x20/0x20 [videodev]
>  video_usercopy+0x1cb/0x450 [videodev]
>  v4l2_ioctl+0x3f/0x50 [videodev]
>  ksys_ioctl+0x3f1/0x7e0
>  ? vfs_write+0x1c4/0x1f0
>  __x64_sys_ioctl+0x11/0x20
>  do_syscall_64+0x49/0x2c0
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> vivid driver crashes the kernel in various places, for instance,
>
>         v4l2_ctrl_modify_range(dev->brightness, ...);
> or
>         v4l2_ctrl_s_ctrl(dev->brightness, ...);
>
> because ->brightness (and quite likely some more controls) is NULL.
> While we may fix the vivid driver, it would be safer to fix core
> API. This patch adds more NULL pointer checks to ctrl API.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 22 ++++++++++++++++-
>  include/media/v4l2-ctrls.h           | 37 ++++++++++++++++++++++++++--
>  2 files changed, 56 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 93d33d1db4e8..02a60f67c2ee 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -2869,6 +2869,9 @@ EXPORT_SYMBOL(v4l2_ctrl_add_handler);
>
>  bool v4l2_ctrl_radio_filter(const struct v4l2_ctrl *ctrl)
>  {
> +       if (WARN_ON(!ctrl))
> +               return false;
> +

.. don't think this is needed, as it's always called via v4l2_ctrl_add_handler
which guarantess a non-NULL pointer.

Thanks!
Ezequiel