v4l2-ioctl: don't set PRIV_MAGIC unconditionally in g_fmt()

PATCH Superseded Hans Verkuil 2 participants 2 messages Jul 21, 2014 Media (V4L/DVB)

lore

Share & tools

lore

Hans Verkuil

author

2014-07-21 09:31 UTC #0 lore raw

PATCH v2 1/1 v4l2-ioctl: don't set PRIV_MAGIC unconditionally in g_fmt()

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 <[email protected]>

drivers/media/v4l2-core/v4l2-ioctl.c +11 -9

index e620387..e876bb9 100644
@@ -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;

Laurent Pinchart

2014-07-21 09:34 UTC #1 lore raw

Hi Hans,

Thank you for the patch.

Quoted 45 lines · 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 <[email protected]>
(quoted diff) +0 -0
b/drivers/media/v4l2-core/v4l2-ioctl.c index e620387..e876bb9 100644
drivers/media/v4l2-core/v4l2-ioctl.c +9 -3
@@ -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:

drivers/media/v4l2-core/v4l2-ioctl.c static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops, · line 1182

Quoted 6 lines

drivers/media/v4l2-core/v4l2-ioctl.c +1 -3

 		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.

Quoted 14 lines

drivers/media/v4l2-core/v4l2-ioctl.c +0 -0

 	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:

drivers/media/v4l2-core/v4l2-ioctl.c +1 -3

 		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;

Signature · 3 lines

Regards,

Laurent Pinchart