[PATCH 1/3] vb2: add buf_validate callback

[PATCH 1/3] vb2: add buf_validate callback

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Adding the request API uncovered a pre-existing problem with
validating output buffers.

The problem is that for output buffers the driver has to validate
the 'field' field of struct v4l2_buffer. This is critical when
encoding or deinterlacing interlaced video.

Drivers always did this in the buf_prepare callback, but that is
not called from VIDIOC_QBUF in two situations: when queueing a
buffer to a request and if VIDIOC_PREPARE_BUF has been called
earlier for that buffer.

As a result of this the 'field' value is not validated.

This patch adds a new buf_validate callback to validate the
output buffer at QBUF time.

Note that PREPARE_BUF doesn't need to validate this: it just
locks the buffer memory and doesn't need nor want to know about
how this buffer is actually going to be used. It's the QBUF ioctl
that determines this.

This issue was found by v4l2-compliance since it failed to replace
V4L2_FIELD_ANY by a proper field value when testing the vivid video
output in combination with requests.

There never was a test before for the PREPARE_BUF/QBUF case, so even
though this bug has been present for quite some time, it was never
noticed.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/common/videobuf2/videobuf2-core.c | 12 +++++++++---
 include/media/videobuf2-core.h                  |  5 +++++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 0ca81d495bda..42eb7716f8a9 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -499,9 +499,9 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
 			pr_info("     buf_init: %u buf_cleanup: %u buf_prepare: %u buf_finish: %u\n",
 				vb->cnt_buf_init, vb->cnt_buf_cleanup,
 				vb->cnt_buf_prepare, vb->cnt_buf_finish);
-			pr_info("     buf_queue: %u buf_done: %u buf_request_complete: %u\n",
-				vb->cnt_buf_queue, vb->cnt_buf_done,
-				vb->cnt_buf_request_complete);
+			pr_info("     buf_validate: %u buf_queue: %u buf_done: %u buf_request_complete: %u\n",
+				vb->cnt_buf_validate, vb->cnt_buf_queue,
+				vb->cnt_buf_done, vb->cnt_buf_request_complete);
 			pr_info("     alloc: %u put: %u prepare: %u finish: %u mmap: %u\n",
 				vb->cnt_mem_alloc, vb->cnt_mem_put,
 				vb->cnt_mem_prepare, vb->cnt_mem_finish,
@@ -1506,6 +1506,12 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
 		return -EBUSY;
 	}
 
+	ret = call_vb_qop(vb, buf_validate, vb);
+	if (ret) {
+		dprintk(1, "buffer validation failed\n");
+		return ret;
+	}
+
 	if (req) {
 		int ret;
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index e86981d615ae..c9f0f3f4ef9a 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -294,6 +294,7 @@ struct vb2_buffer {
 	u32		cnt_mem_num_users;
 	u32		cnt_mem_mmap;
 
+	u32		cnt_buf_validate;
 	u32		cnt_buf_init;
 	u32		cnt_buf_prepare;
 	u32		cnt_buf_finish;
@@ -340,6 +341,9 @@ struct vb2_buffer {
  * @wait_finish:	reacquire all locks released in the previous callback;
  *			required to continue operation after sleeping while
  *			waiting for a new buffer to arrive.
+ * @buf_validate:	called every time the buffer is queued from userspace;
+ *			drivers can use this to validate userspace-provided
+ *			information; optional.
  * @buf_init:		called once after allocating a buffer (in MMAP case)
  *			or after acquiring a new USERPTR buffer; drivers may
  *			perform additional buffer-related initialization;
@@ -407,6 +411,7 @@ struct vb2_ops {
 	void (*wait_prepare)(struct vb2_queue *q);
 	void (*wait_finish)(struct vb2_queue *q);
 
+	int (*buf_validate)(struct vb2_buffer *vb);
 	int (*buf_init)(struct vb2_buffer *vb);
 	int (*buf_prepare)(struct vb2_buffer *vb);
 	void (*buf_finish)(struct vb2_buffer *vb);
-- 
2.19.1

[PATCH 2/3] vim2m: add buf_validate callback

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Split off the field validation from buf_prepare into a new
buf_validate function. Field validation for output buffers should
be done there since buf_prepare is not guaranteed to be called at
QBUF time.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/vim2m.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index d01821a6906a..9559be91daca 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -753,15 +753,13 @@ static int vim2m_queue_setup(struct vb2_queue *vq,
 	return 0;
 }
 
-static int vim2m_buf_prepare(struct vb2_buffer *vb)
+static int vim2m_buf_validate(struct vb2_buffer *vb)
 {
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 	struct vim2m_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
-	struct vim2m_q_data *q_data;
 
 	dprintk(ctx->dev, "type: %d\n", vb->vb2_queue->type);
 
-	q_data = get_q_data(ctx, vb->vb2_queue->type);
 	if (V4L2_TYPE_IS_OUTPUT(vb->vb2_queue->type)) {
 		if (vbuf->field == V4L2_FIELD_ANY)
 			vbuf->field = V4L2_FIELD_NONE;
@@ -772,6 +770,17 @@ static int vim2m_buf_prepare(struct vb2_buffer *vb)
 		}
 	}
 
+	return 0;
+}
+
+static int vim2m_buf_prepare(struct vb2_buffer *vb)
+{
+	struct vim2m_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
+	struct vim2m_q_data *q_data;
+
+	dprintk(ctx->dev, "type: %d\n", vb->vb2_queue->type);
+
+	q_data = get_q_data(ctx, vb->vb2_queue->type);
 	if (vb2_plane_size(vb, 0) < q_data->sizeimage) {
 		dprintk(ctx->dev, "%s data will not fit into plane (%lu < %lu)\n",
 				__func__, vb2_plane_size(vb, 0), (long)q_data->sizeimage);
@@ -832,6 +841,7 @@ static void vim2m_buf_request_complete(struct vb2_buffer *vb)
 
 static const struct vb2_ops vim2m_qops = {
 	.queue_setup	 = vim2m_queue_setup,
+	.buf_validate	 = vim2m_buf_validate,
 	.buf_prepare	 = vim2m_buf_prepare,
 	.buf_queue	 = vim2m_buf_queue,
 	.start_streaming = vim2m_start_streaming,
-- 
2.19.1

[PATCH 3/3] vivid: add buf_validate callback

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Split off the field validation from buf_prepare into a new
buf_validate function. Field validation for output buffers should
be done there since buf_prepare is not guaranteed to be called at
QBUF time.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/vivid/vivid-vid-out.c | 23 ++++++++++++++------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-vid-out.c b/drivers/media/platform/vivid/vivid-vid-out.c
index 7642cbdb0e14..3e93dbbb4ffe 100644
--- a/drivers/media/platform/vivid/vivid-vid-out.c
+++ b/drivers/media/platform/vivid/vivid-vid-out.c
@@ -81,10 +81,24 @@ static int vid_out_queue_setup(struct vb2_queue *vq,
 	return 0;
 }
 
-static int vid_out_buf_prepare(struct vb2_buffer *vb)
+static int vid_out_buf_validate(struct vb2_buffer *vb)
 {
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 	struct vivid_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
+
+	dprintk(dev, 1, "%s\n", __func__);
+
+	if (dev->field_out != V4L2_FIELD_ALTERNATE)
+		vbuf->field = dev->field_out;
+	else if (vbuf->field != V4L2_FIELD_TOP &&
+		 vbuf->field != V4L2_FIELD_BOTTOM)
+		return -EINVAL;
+	return 0;
+}
+
+static int vid_out_buf_prepare(struct vb2_buffer *vb)
+{
+	struct vivid_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
 	unsigned long size;
 	unsigned planes;
 	unsigned p;
@@ -105,12 +119,6 @@ static int vid_out_buf_prepare(struct vb2_buffer *vb)
 		return -EINVAL;
 	}
 
-	if (dev->field_out != V4L2_FIELD_ALTERNATE)
-		vbuf->field = dev->field_out;
-	else if (vbuf->field != V4L2_FIELD_TOP &&
-		 vbuf->field != V4L2_FIELD_BOTTOM)
-		return -EINVAL;
-
 	for (p = 0; p < planes; p++) {
 		size = dev->bytesperline_out[p] * dev->fmt_out_rect.height +
 			vb->planes[p].data_offset;
@@ -190,6 +198,7 @@ static void vid_out_buf_request_complete(struct vb2_buffer *vb)
 
 const struct vb2_ops vivid_vid_out_qops = {
 	.queue_setup		= vid_out_queue_setup,
+	.buf_validate		= vid_out_buf_validate,
 	.buf_prepare		= vid_out_buf_prepare,
 	.buf_queue		= vid_out_buf_queue,
 	.start_streaming	= vid_out_start_streaming,
-- 
2.19.1

Re: [PATCH 1/3] vb2: add buf_validate callback

[Mauro Carvalho Chehab]

Em Mon,  3 Dec 2018 13:46:01 +0100
hverkuil-cisco@xs4all.nl escreveu:

> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> Adding the request API uncovered a pre-existing problem with
> validating output buffers.
> 
> The problem is that for output buffers the driver has to validate
> the 'field' field of struct v4l2_buffer. This is critical when
> encoding or deinterlacing interlaced video.
> 
> Drivers always did this in the buf_prepare callback, but that is
> not called from VIDIOC_QBUF in two situations: when queueing a
> buffer to a request and if VIDIOC_PREPARE_BUF has been called
> earlier for that buffer.

Hmm.. if I got it right, it will only affect drivers
using request API, right?

IMO, the description of the callback should be a way more
verbose, containing a summary of the explanation here.

> 
> As a result of this the 'field' value is not validated.
> 
> This patch adds a new buf_validate callback to validate the
> output buffer at QBUF time.
> 
> Note that PREPARE_BUF doesn't need to validate this: it just
> locks the buffer memory and doesn't need nor want to know about
> how this buffer is actually going to be used. It's the QBUF ioctl
> that determines this.
> 
> This issue was found by v4l2-compliance since it failed to replace
> V4L2_FIELD_ANY by a proper field value when testing the vivid video
> output in combination with requests.
> 
> There never was a test before for the PREPARE_BUF/QBUF case, so even
> though this bug has been present for quite some time, it was never
> noticed.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 12 +++++++++---
>  include/media/videobuf2-core.h                  |  5 +++++
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 0ca81d495bda..42eb7716f8a9 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -499,9 +499,9 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>  			pr_info("     buf_init: %u buf_cleanup: %u buf_prepare: %u buf_finish: %u\n",
>  				vb->cnt_buf_init, vb->cnt_buf_cleanup,
>  				vb->cnt_buf_prepare, vb->cnt_buf_finish);
> -			pr_info("     buf_queue: %u buf_done: %u buf_request_complete: %u\n",
> -				vb->cnt_buf_queue, vb->cnt_buf_done,
> -				vb->cnt_buf_request_complete);
> +			pr_info("     buf_validate: %u buf_queue: %u buf_done: %u buf_request_complete: %u\n",
> +				vb->cnt_buf_validate, vb->cnt_buf_queue,
> +				vb->cnt_buf_done, vb->cnt_buf_request_complete);
>  			pr_info("     alloc: %u put: %u prepare: %u finish: %u mmap: %u\n",
>  				vb->cnt_mem_alloc, vb->cnt_mem_put,
>  				vb->cnt_mem_prepare, vb->cnt_mem_finish,
> @@ -1506,6 +1506,12 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
>  		return -EBUSY;
>  	}
>  
> +	ret = call_vb_qop(vb, buf_validate, vb);
> +	if (ret) {
> +		dprintk(1, "buffer validation failed\n");
> +		return ret;
> +	}
> +
>  	if (req) {
>  		int ret;
>  
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index e86981d615ae..c9f0f3f4ef9a 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -294,6 +294,7 @@ struct vb2_buffer {
>  	u32		cnt_mem_num_users;
>  	u32		cnt_mem_mmap;
>  
> +	u32		cnt_buf_validate;
>  	u32		cnt_buf_init;
>  	u32		cnt_buf_prepare;
>  	u32		cnt_buf_finish;
> @@ -340,6 +341,9 @@ struct vb2_buffer {
>   * @wait_finish:	reacquire all locks released in the previous callback;
>   *			required to continue operation after sleeping while
>   *			waiting for a new buffer to arrive.
> + * @buf_validate:	called every time the buffer is queued from userspace;
> + *			drivers can use this to validate userspace-provided
> + *			information; optional.
>   * @buf_init:		called once after allocating a buffer (in MMAP case)
>   *			or after acquiring a new USERPTR buffer; drivers may
>   *			perform additional buffer-related initialization;
> @@ -407,6 +411,7 @@ struct vb2_ops {
>  	void (*wait_prepare)(struct vb2_queue *q);
>  	void (*wait_finish)(struct vb2_queue *q);
>  
> +	int (*buf_validate)(struct vb2_buffer *vb);
>  	int (*buf_init)(struct vb2_buffer *vb);
>  	int (*buf_prepare)(struct vb2_buffer *vb);
>  	void (*buf_finish)(struct vb2_buffer *vb);



Thanks,
Mauro