[PATCH 0/2] videobuf2: Fix kernel memory overwriting

[PATCH 0/2] videobuf2: Fix kernel memory overwriting

[Sakari Ailus]

Hi all,

In multi-planar API, the buffer length and m.planes fields are checked
against the vb2 buffer before passing the buffer on to the core, but
commit b0e0e1f83de31aa0428c38b692c590cc0ecd3f03 removed this check from
VIDIOC_DQBUF path. This leads to kernel memory overwriting in certain
cases.

This affects only v4.4 and newer and a very few drivers which use the
multi-planar API. Due to the very limited scope of the issue this is not
seen to require special handling.

The patches should be applied to the stable series, I'll add cc stable
in the pull request.

-- 
Kind regards,
Sakari

[PATCH 1/2] videobuf2-core: Check user space planes array in dqbuf

[Sakari Ailus]

The number of planes in videobuf2 is specific to a buffer. In order to
verify that the planes array provided by the user is long enough, a new
vb2_buf_op is required.

Call __verify_planes_array() when the dequeued buffer is known. Return an
error to the caller if there was one, otherwise remove the buffer from the
done list.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 10 +++++-----
 include/media/videobuf2-core.h           |  4 ++++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 5d016f4..2169544 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1645,7 +1645,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
  * Will sleep if required for nonblocking == false.
  */
 static int __vb2_get_done_vb(struct vb2_queue *q, struct vb2_buffer **vb,
-				int nonblocking)
+			     void *pb, int nonblocking)
 {
 	unsigned long flags;
 	int ret;
@@ -1666,10 +1666,10 @@ static int __vb2_get_done_vb(struct vb2_queue *q, struct vb2_buffer **vb,
 	/*
 	 * Only remove the buffer from done_list if v4l2_buffer can handle all
 	 * the planes.
-	 * Verifying planes is NOT necessary since it already has been checked
-	 * before the buffer is queued/prepared. So it can never fail.
 	 */
-	list_del(&(*vb)->done_entry);
+	ret = call_bufop(q, verify_planes_array, *vb, pb);
+	if (!ret)
+		list_del(&(*vb)->done_entry);
 	spin_unlock_irqrestore(&q->done_lock, flags);
 
 	return ret;
@@ -1748,7 +1748,7 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb,
 	struct vb2_buffer *vb = NULL;
 	int ret;
 
-	ret = __vb2_get_done_vb(q, &vb, nonblocking);
+	ret = __vb2_get_done_vb(q, &vb, pb, nonblocking);
 	if (ret < 0)
 		return ret;
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 8a0f55b..e2b1773 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -375,6 +375,9 @@ struct vb2_ops {
 /**
  * struct vb2_ops - driver-specific callbacks
  *
+ * @verify_planes_array:Verify that a given user space structure contains
+ *			enough planes for the buffer. This is called
+ *			for each dequeued buffer.
  * @fill_user_buffer:	given a vb2_buffer fill in the userspace structure.
  *			For V4L2 this is a struct v4l2_buffer.
  * @fill_vb2_buffer:	given a userspace structure, fill in the vb2_buffer.
@@ -384,6 +387,7 @@ struct vb2_ops {
  *			the vb2_buffer struct.
  */
 struct vb2_buf_ops {
+	int (*verify_planes_array)(struct vb2_buffer *vb, const void *pb);
 	void (*fill_user_buffer)(struct vb2_buffer *vb, void *pb);
 	int (*fill_vb2_buffer)(struct vb2_buffer *vb, const void *pb,
 				struct vb2_plane *planes);
-- 
2.1.4

[PATCH 2/2] videobuf2-v4l2: Verify planes array in buffer dequeueing

[Sakari Ailus]

When a buffer is being dequeued using VIDIOC_DQBUF IOCTL, the exact buffer
which will be dequeued is not known until the buffer has been removed from
the queue. The number of planes is specific to a buffer, not to the queue.

This does lead to the situation where multi-plane buffers may be requested
and queued with n planes, but VIDIOC_DQBUF IOCTL may be passed an argument
struct with fewer planes.

__fill_v4l2_buffer() however uses the number of planes from the dequeued
videobuf2 buffer, overwriting kernel memory (the m.planes array allocated
in video_usercopy() in v4l2-ioctl.c)  if the user provided fewer
planes than the dequeued buffer had. Oops!

Fixes: b0e0e1f83de3 ("[media] media: videobuf2: Prepare to divide videobuf2")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/videobuf2-v4l2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
index 91f5521..8da7470 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -74,6 +74,11 @@ static int __verify_planes_array(struct vb2_buffer *vb, const struct v4l2_buffer
 	return 0;
 }
 
+static int __verify_planes_array_core(struct vb2_buffer *vb, const void *pb)
+{
+	return __verify_planes_array(vb, pb);
+}
+
 /**
  * __verify_length() - Verify that the bytesused value for each plane fits in
  * the plane length and that the data offset doesn't exceed the bytesused value.
@@ -437,6 +442,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
 }
 
 static const struct vb2_buf_ops v4l2_buf_ops = {
+	.verify_planes_array	= __verify_planes_array_core,
 	.fill_user_buffer	= __fill_v4l2_buffer,
 	.fill_vb2_buffer	= __fill_vb2_buffer,
 	.copy_timestamp		= __copy_timestamp,
-- 
2.1.4

Re: [PATCH 0/2] videobuf2: Fix kernel memory overwriting

[Sakari Ailus]

Fixing Mauro's e-mail address...

On Wed, Apr 06, 2016 at 02:46:06PM +0300, Sakari Ailus wrote:
> Hi all,
> 
> In multi-planar API, the buffer length and m.planes fields are checked
> against the vb2 buffer before passing the buffer on to the core, but
> commit b0e0e1f83de31aa0428c38b692c590cc0ecd3f03 removed this check from
> VIDIOC_DQBUF path. This leads to kernel memory overwriting in certain
> cases.
> 
> This affects only v4.4 and newer and a very few drivers which use the
> multi-planar API. Due to the very limited scope of the issue this is not
> seen to require special handling.
> 
> The patches should be applied to the stable series, I'll add cc stable
> in the pull request.
> 
> -- 
> Kind regards,
> Sakari
> 
> --
> 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

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 2/2] videobuf2-v4l2: Verify planes array in buffer dequeueing

[Hans Verkuil]

On 04/06/2016 04:46 AM, Sakari Ailus wrote:
> When a buffer is being dequeued using VIDIOC_DQBUF IOCTL, the exact buffer
> which will be dequeued is not known until the buffer has been removed from
> the queue. The number of planes is specific to a buffer, not to the queue.
>
> This does lead to the situation where multi-plane buffers may be requested
> and queued with n planes, but VIDIOC_DQBUF IOCTL may be passed an argument
> struct with fewer planes.
>
> __fill_v4l2_buffer() however uses the number of planes from the dequeued
> videobuf2 buffer, overwriting kernel memory (the m.planes array allocated
> in video_usercopy() in v4l2-ioctl.c)  if the user provided fewer
> planes than the dequeued buffer had. Oops!
>
> Fixes: b0e0e1f83de3 ("[media] media: videobuf2: Prepare to divide videobuf2")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks,

	Hans

> ---
>   drivers/media/v4l2-core/videobuf2-v4l2.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index 91f5521..8da7470 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -74,6 +74,11 @@ static int __verify_planes_array(struct vb2_buffer *vb, const struct v4l2_buffer
>   	return 0;
>   }
>
> +static int __verify_planes_array_core(struct vb2_buffer *vb, const void *pb)
> +{
> +	return __verify_planes_array(vb, pb);
> +}
> +
>   /**
>    * __verify_length() - Verify that the bytesused value for each plane fits in
>    * the plane length and that the data offset doesn't exceed the bytesused value.
> @@ -437,6 +442,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
>   }
>
>   static const struct vb2_buf_ops v4l2_buf_ops = {
> +	.verify_planes_array	= __verify_planes_array_core,
>   	.fill_user_buffer	= __fill_v4l2_buffer,
>   	.fill_vb2_buffer	= __fill_vb2_buffer,
>   	.copy_timestamp		= __copy_timestamp,
>