[PATCHv2 for v3.17] v4l2-ioctl: don't set PRIV_MAGIC unconditionally in g_fmt()

[PATCHv2 for v3.17] v4l2-ioctl: don't set PRIV_MAGIC unconditionally in g_fmt()

[Hans Verkuil]

Regression fix:

V4L2_PIX_FMT_PRIV_MAGIC should only be set for the VIDEO_CAPTURE and
VIDEO_OUTPUT buffer types, and not for any others. In the case of
the win format this overwrote a pointer value that is passed in from
userspace.

Just set it for V4L2_BUF_TYPE_VIDEO_CAPTURE and OUTPUT and not anywhere
else. Also set it before the callback is called rather than after it,
just as is done for try/s_fmt.

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

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index e620387..e876bb9 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1141,9 +1141,6 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
 	bool is_sdr = vfd->vfl_type == VFL_TYPE_SDR;
 	bool is_rx = vfd->vfl_dir != VFL_DIR_TX;
 	bool is_tx = vfd->vfl_dir != VFL_DIR_RX;
-	int ret;
-
-	p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
 
 	/*
 	 * fmt can't be cleared for these overlay types due to the 'clips'
@@ -1164,6 +1161,15 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
 		p->fmt.win.bitmap = bitmap;
 		break;
 	}
+	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
+	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
+		memset(&p->fmt, 0, sizeof(p->fmt));
+		/*
+		 * Fill in the priv field to signal that the extended
+		 * v4l2_pix_format fields are valid.
+		 */
+		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
+		break;
 	default:
 		memset(&p->fmt, 0, sizeof(p->fmt));
 		break;
@@ -1173,9 +1179,7 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
 		if (unlikely(!is_rx || !is_vid || !ops->vidioc_g_fmt_vid_cap))
 			break;
-		ret = ops->vidioc_g_fmt_vid_cap(file, fh, arg);
-		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
-		return ret;
+		return ops->vidioc_g_fmt_vid_cap(file, fh, arg);
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
 		if (unlikely(!is_rx || !is_vid || !ops->vidioc_g_fmt_vid_cap_mplane))
 			break;
@@ -1195,9 +1199,7 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
 		if (unlikely(!is_tx || !is_vid || !ops->vidioc_g_fmt_vid_out))
 			break;
-		ret = ops->vidioc_g_fmt_vid_out(file, fh, arg);
-		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
-		return ret;
+		return ops->vidioc_g_fmt_vid_out(file, fh, arg);
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
 		if (unlikely(!is_tx || !is_vid || !ops->vidioc_g_fmt_vid_out_mplane))
 			break;

Re: [PATCHv2 for v3.17] v4l2-ioctl: don't set PRIV_MAGIC unconditionally in g_fmt()

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Monday 21 July 2014 11:31:46 Hans Verkuil wrote:
> Regression fix:
> 
> V4L2_PIX_FMT_PRIV_MAGIC should only be set for the VIDEO_CAPTURE and
> VIDEO_OUTPUT buffer types, and not for any others. In the case of
> the win format this overwrote a pointer value that is passed in from
> userspace.
> 
> Just set it for V4L2_BUF_TYPE_VIDEO_CAPTURE and OUTPUT and not anywhere
> else. Also set it before the callback is called rather than after it,
> just as is done for try/s_fmt.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c index e620387..e876bb9 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1141,9 +1141,6 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
> bool is_sdr = vfd->vfl_type == VFL_TYPE_SDR;
>  	bool is_rx = vfd->vfl_dir != VFL_DIR_TX;
>  	bool is_tx = vfd->vfl_dir != VFL_DIR_RX;
> -	int ret;
> -
> -	p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> 
>  	/*
>  	 * fmt can't be cleared for these overlay types due to the 'clips'
> @@ -1164,6 +1161,15 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops
> *ops, p->fmt.win.bitmap = bitmap;
>  		break;
>  	}
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> +		memset(&p->fmt, 0, sizeof(p->fmt));
> +		/*
> +		 * Fill in the priv field to signal that the extended
> +		 * v4l2_pix_format fields are valid.
> +		 */
> +		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> +		break;
>  	default:
>  		memset(&p->fmt, 0, sizeof(p->fmt));
>  		break;
> @@ -1173,9 +1179,7 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
> case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>  		if (unlikely(!is_rx || !is_vid || !ops->vidioc_g_fmt_vid_cap))
>  			break;
> -		ret = ops->vidioc_g_fmt_vid_cap(file, fh, arg);
> -		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> -		return ret;
> +		return ops->vidioc_g_fmt_vid_cap(file, fh, arg);

If a driver zeroes the priv field in vidioc_g_fmt_vid_cap or 
(vidioc_g_fmt_vid_out below) this will break. We should really set the field 
before and after calling the operation.

>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>  		if (unlikely(!is_rx || !is_vid || !ops->vidioc_g_fmt_vid_cap_mplane))
>  			break;
> @@ -1195,9 +1199,7 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
> case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>  		if (unlikely(!is_tx || !is_vid || !ops->vidioc_g_fmt_vid_out))
>  			break;
> -		ret = ops->vidioc_g_fmt_vid_out(file, fh, arg);
> -		p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> -		return ret;
> +		return ops->vidioc_g_fmt_vid_out(file, fh, arg);
>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>  		if (unlikely(!is_tx || !is_vid || !ops->vidioc_g_fmt_vid_out_mplane))
>  			break;

-- 
Regards,

Laurent Pinchart