[PATCH v6 0/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()

[PATCH v6 0/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()

[Janusz Krzysztofik]

Correctness of format type (try or active) and pad ID parameters passed
to subdevice operation callbacks is now verified only for IOCTL calls.
However, those callbacks are also used by drivers, e.g., V4L2 host
interfaces.
    
Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call()
macro while calling subdevice operations, move those parameter checks
from subdev_do_ioctl() to v4l2_subdev_call().  Also, add check for
non-NULL pointers, including pad config if V4L2_SUBDEV_FORMAT_tRY is
requested.

Having that done, we can avoid taking care of those checks inside
drivers.

Janusz Krzysztofik (3):
  media: v4l2-subdev: Verify arguments in v4l2_subdev_call()
  media: v4l2-subdev: Verify v4l2_subdev_call() pointer arguments
  media: v4l2-subdev: Verify v4l2_subdev_call() pad config argument

 drivers/media/v4l2-core/v4l2-subdev.c | 262 +++++++++++++++++---------
 include/media/v4l2-subdev.h           |   6 +
 2 files changed, 182 insertions(+), 86 deletions(-)

Changelog:
v5->v6:
- rename wrappers to call_something() as suggested by Sakari - thanks!
- make check_ functions inline - also on Sakari's suggestion, thanks!
- drop patch 2/4 and remove WARN_ONs from remaining patches to avoid
  kernel WARNs on non-kernel bugs - thanks Hans for pointing this out!

v4->v5:
- a few coding style and code formatting changes,
- require CONFIG_MEDIA_CONTROLLER, not CONFIG_VIDEO_V4L2_SUBDEV_API,
  for a valid pad ID check,
- perform pad ID check only if at least one pad is configured so
  drivers which don't configure pads are not affected if built with
  CONFIG_MEDIA_CONTROLLER defined,
- issue kernel warnings on invalid parameters (new patch - 2/4),
- validate pointers before using them (new patch - 3/4).

v3->v4:
- fix 'struct' keyword missing from patch 2/2,
- fix checkpatch reported style issue in patch 2/2
Sorry for that.

v2->v3:
- add patch 2/2 with pad config check,
- adjust continuation line alignments in patch 1/2 to match those
  used in 2/2.

v1->v2:
- replace the horrible macro with a structure of wrapper functions;
  inspired by Hans' and Sakari's comments - thanks!

-- 
2.21.0

[PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()

[Janusz Krzysztofik]

Correctness of format type (try or active) and pad number parameters
passed to subdevice operation callbacks is now verified only for IOCTL
calls.  However, those callbacks are also used by drivers, e.g., V4L2
host interfaces.

Since both subdev_do_ioctl() and drivers are using v4l2_subdev_call()
macro while calling subdevice operations, move those parameter checks
from subdev_do_ioctl() to v4l2_subdev_call() so we can avoid taking care
of those checks inside drivers.

Define a wrapper function for each operation callback in scope, then
gather those wrappers in a static v4l2_subdev_ops structure so the
v4l2_subdev_call() macro can find them easy if provided.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 232 ++++++++++++++++----------
 include/media/v4l2-subdev.h           |   6 +
 2 files changed, 146 insertions(+), 92 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index d75815ab0d7b..c61c95007d89 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -120,56 +120,169 @@ static int subdev_close(struct file *file)
 	return 0;
 }
 
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-static int check_format(struct v4l2_subdev *sd,
-			struct v4l2_subdev_format *format)
+static inline int check_which(__u32 which)
 {
-	if (format->which != V4L2_SUBDEV_FORMAT_TRY &&
-	    format->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-		return -EINVAL;
-
-	if (format->pad >= sd->entity.num_pads)
+	if (which != V4L2_SUBDEV_FORMAT_TRY &&
+	    which != V4L2_SUBDEV_FORMAT_ACTIVE)
 		return -EINVAL;
 
 	return 0;
 }
 
-static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop *crop)
+static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
 {
-	if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
-	    crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	if (sd->entity.num_pads && pad >= sd->entity.num_pads)
 		return -EINVAL;
+#endif
+	return 0;
+}
 
-	if (crop->pad >= sd->entity.num_pads)
-		return -EINVAL;
+static inline int check_format(struct v4l2_subdev *sd,
+			       struct v4l2_subdev_format *format)
+{
+	return check_which(format->which) ? : check_pad(sd, format->pad);
+}
 
-	return 0;
+static int call_get_fmt(struct v4l2_subdev *sd,
+			struct v4l2_subdev_pad_config *cfg,
+			struct v4l2_subdev_format *format)
+{
+	return check_format(sd, format) ? :
+	       sd->ops->pad->get_fmt(sd, cfg, format);
 }
 
-static int check_selection(struct v4l2_subdev *sd,
-			   struct v4l2_subdev_selection *sel)
+static int call_set_fmt(struct v4l2_subdev *sd,
+			struct v4l2_subdev_pad_config *cfg,
+			struct v4l2_subdev_format *format)
 {
-	if (sel->which != V4L2_SUBDEV_FORMAT_TRY &&
-	    sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-		return -EINVAL;
+	return check_format(sd, format) ? :
+	       sd->ops->pad->set_fmt(sd, cfg, format);
+}
 
-	if (sel->pad >= sd->entity.num_pads)
-		return -EINVAL;
+static int call_enum_mbus_code(struct v4l2_subdev *sd,
+			       struct v4l2_subdev_pad_config *cfg,
+			       struct v4l2_subdev_mbus_code_enum *code)
+{
+	return check_which(code->which) ? : check_pad(sd, code->pad) ? :
+	       sd->ops->pad->enum_mbus_code(sd, cfg, code);
+}
 
-	return 0;
+static int call_enum_frame_size(struct v4l2_subdev *sd,
+				struct v4l2_subdev_pad_config *cfg,
+				struct v4l2_subdev_frame_size_enum *fse)
+{
+	return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
+	       sd->ops->pad->enum_frame_size(sd, cfg, fse);
 }
 
-static int check_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
+static inline int check_frame_interval(struct v4l2_subdev *sd,
+				       struct v4l2_subdev_frame_interval *fi)
 {
-	if (edid->pad >= sd->entity.num_pads)
-		return -EINVAL;
+	return check_pad(sd, fi->pad);
+}
+
+static int call_g_frame_interval(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_frame_interval *fi)
+{
+	return check_frame_interval(sd, fi) ? :
+	       sd->ops->video->g_frame_interval(sd, fi);
+}
+
+static int call_s_frame_interval(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_frame_interval *fi)
+{
+	return check_frame_interval(sd, fi) ? :
+	       sd->ops->video->s_frame_interval(sd, fi);
+}
+
+static int call_enum_frame_interval(struct v4l2_subdev *sd,
+				    struct v4l2_subdev_pad_config *cfg,
+				    struct v4l2_subdev_frame_interval_enum *fie)
+{
+	return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
+	       sd->ops->pad->enum_frame_interval(sd, cfg, fie);
+}
 
+static inline int check_selection(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_selection *sel)
+{
+	return check_which(sel->which) ? : check_pad(sd, sel->pad);
+}
+
+static int call_get_selection(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_pad_config *cfg,
+			      struct v4l2_subdev_selection *sel)
+{
+	return check_selection(sd, sel) ? :
+	       sd->ops->pad->get_selection(sd, cfg, sel);
+}
+
+static int call_set_selection(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_pad_config *cfg,
+			      struct v4l2_subdev_selection *sel)
+{
+	return check_selection(sd, sel) ? :
+	       sd->ops->pad->set_selection(sd, cfg, sel);
+}
+
+static inline int check_edid(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_edid *edid)
+{
 	if (edid->blocks && edid->edid == NULL)
 		return -EINVAL;
 
-	return 0;
+	return check_pad(sd, edid->pad);
 }
-#endif
+
+static int call_get_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
+{
+	return check_edid(sd, edid) ? : sd->ops->pad->get_edid(sd, edid);
+}
+
+static int call_set_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
+{
+	return check_edid(sd, edid) ? : sd->ops->pad->set_edid(sd, edid);
+}
+
+static int call_dv_timings_cap(struct v4l2_subdev *sd,
+			       struct v4l2_dv_timings_cap *cap)
+{
+	return check_pad(sd, cap->pad) ? :
+	       sd->ops->pad->dv_timings_cap(sd, cap);
+}
+
+static int call_enum_dv_timings(struct v4l2_subdev *sd,
+				struct v4l2_enum_dv_timings *dvt)
+{
+	return check_pad(sd, dvt->pad) ? :
+	       sd->ops->pad->enum_dv_timings(sd, dvt);
+}
+
+static const struct v4l2_subdev_pad_ops v4l2_subdev_call_pad_wrappers = {
+	.get_fmt		= call_get_fmt,
+	.set_fmt		= call_set_fmt,
+	.enum_mbus_code		= call_enum_mbus_code,
+	.enum_frame_size	= call_enum_frame_size,
+	.enum_frame_interval	= call_enum_frame_interval,
+	.get_selection		= call_get_selection,
+	.set_selection		= call_set_selection,
+	.get_edid		= call_get_edid,
+	.set_edid		= call_set_edid,
+	.dv_timings_cap		= call_dv_timings_cap,
+	.enum_dv_timings	= call_enum_dv_timings,
+};
+
+static const struct v4l2_subdev_video_ops v4l2_subdev_call_video_wrappers = {
+	.g_frame_interval	= call_g_frame_interval,
+	.s_frame_interval	= call_s_frame_interval,
+};
+
+const struct v4l2_subdev_ops v4l2_subdev_call_wrappers = {
+	.pad	= &v4l2_subdev_call_pad_wrappers,
+	.video	= &v4l2_subdev_call_video_wrappers,
+};
+EXPORT_SYMBOL(v4l2_subdev_call_wrappers);
 
 static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 {
@@ -292,10 +405,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_SUBDEV_G_FMT: {
 		struct v4l2_subdev_format *format = arg;
 
-		rval = check_format(sd, format);
-		if (rval)
-			return rval;
-
 		memset(format->reserved, 0, sizeof(format->reserved));
 		memset(format->format.reserved, 0, sizeof(format->format.reserved));
 		return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
@@ -304,10 +413,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_SUBDEV_S_FMT: {
 		struct v4l2_subdev_format *format = arg;
 
-		rval = check_format(sd, format);
-		if (rval)
-			return rval;
-
 		memset(format->reserved, 0, sizeof(format->reserved));
 		memset(format->format.reserved, 0, sizeof(format->format.reserved));
 		return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
@@ -317,10 +422,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		struct v4l2_subdev_crop *crop = arg;
 		struct v4l2_subdev_selection sel;
 
-		rval = check_crop(sd, crop);
-		if (rval)
-			return rval;
-
 		memset(crop->reserved, 0, sizeof(crop->reserved));
 		memset(&sel, 0, sizeof(sel));
 		sel.which = crop->which;
@@ -340,10 +441,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		struct v4l2_subdev_selection sel;
 
 		memset(crop->reserved, 0, sizeof(crop->reserved));
-		rval = check_crop(sd, crop);
-		if (rval)
-			return rval;
-
 		memset(&sel, 0, sizeof(sel));
 		sel.which = crop->which;
 		sel.pad = crop->pad;
@@ -361,13 +458,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_SUBDEV_ENUM_MBUS_CODE: {
 		struct v4l2_subdev_mbus_code_enum *code = arg;
 
-		if (code->which != V4L2_SUBDEV_FORMAT_TRY &&
-		    code->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-			return -EINVAL;
-
-		if (code->pad >= sd->entity.num_pads)
-			return -EINVAL;
-
 		memset(code->reserved, 0, sizeof(code->reserved));
 		return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
 					code);
@@ -376,13 +466,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_SUBDEV_ENUM_FRAME_SIZE: {
 		struct v4l2_subdev_frame_size_enum *fse = arg;
 
-		if (fse->which != V4L2_SUBDEV_FORMAT_TRY &&
-		    fse->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-			return -EINVAL;
-
-		if (fse->pad >= sd->entity.num_pads)
-			return -EINVAL;
-
 		memset(fse->reserved, 0, sizeof(fse->reserved));
 		return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
 					fse);
@@ -391,9 +474,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_SUBDEV_G_FRAME_INTERVAL: {
 		struct v4l2_subdev_frame_interval *fi = arg;
 
-		if (fi->pad >= sd->entity.num_pads)
-			return -EINVAL;
-
 		memset(fi->reserved, 0, sizeof(fi->reserved));
 		return v4l2_subdev_call(sd, video, g_frame_interval, arg);
 	}
@@ -401,9 +481,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_SUBDEV_S_FRAME_INTERVAL: {
 		struct v4l2_subdev_frame_interval *fi = arg;
 
-		if (fi->pad >= sd->entity.num_pads)
-			return -EINVAL;
-
 		memset(fi->reserved, 0, sizeof(fi->reserved));
 		return v4l2_subdev_call(sd, video, s_frame_interval, arg);
 	}
@@ -411,13 +488,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL: {
 		struct v4l2_subdev_frame_interval_enum *fie = arg;
 
-		if (fie->which != V4L2_SUBDEV_FORMAT_TRY &&
-		    fie->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-			return -EINVAL;
-
-		if (fie->pad >= sd->entity.num_pads)
-			return -EINVAL;
-
 		memset(fie->reserved, 0, sizeof(fie->reserved));
 		return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
 					fie);
@@ -426,10 +496,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_SUBDEV_G_SELECTION: {
 		struct v4l2_subdev_selection *sel = arg;
 
-		rval = check_selection(sd, sel);
-		if (rval)
-			return rval;
-
 		memset(sel->reserved, 0, sizeof(sel->reserved));
 		return v4l2_subdev_call(
 			sd, pad, get_selection, subdev_fh->pad, sel);
@@ -438,10 +504,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_SUBDEV_S_SELECTION: {
 		struct v4l2_subdev_selection *sel = arg;
 
-		rval = check_selection(sd, sel);
-		if (rval)
-			return rval;
-
 		memset(sel->reserved, 0, sizeof(sel->reserved));
 		return v4l2_subdev_call(
 			sd, pad, set_selection, subdev_fh->pad, sel);
@@ -450,38 +512,24 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_G_EDID: {
 		struct v4l2_subdev_edid *edid = arg;
 
-		rval = check_edid(sd, edid);
-		if (rval)
-			return rval;
-
 		return v4l2_subdev_call(sd, pad, get_edid, edid);
 	}
 
 	case VIDIOC_S_EDID: {
 		struct v4l2_subdev_edid *edid = arg;
 
-		rval = check_edid(sd, edid);
-		if (rval)
-			return rval;
-
 		return v4l2_subdev_call(sd, pad, set_edid, edid);
 	}
 
 	case VIDIOC_SUBDEV_DV_TIMINGS_CAP: {
 		struct v4l2_dv_timings_cap *cap = arg;
 
-		if (cap->pad >= sd->entity.num_pads)
-			return -EINVAL;
-
 		return v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
 	}
 
 	case VIDIOC_SUBDEV_ENUM_DV_TIMINGS: {
 		struct v4l2_enum_dv_timings *dvt = arg;
 
-		if (dvt->pad >= sd->entity.num_pads)
-			return -EINVAL;
-
 		return v4l2_subdev_call(sd, pad, enum_dv_timings, dvt);
 	}
 
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index a7fa5b80915a..e1e3c18c3fd6 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1091,6 +1091,8 @@ void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg);
 void v4l2_subdev_init(struct v4l2_subdev *sd,
 		      const struct v4l2_subdev_ops *ops);
 
+extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
+
 /**
  * v4l2_subdev_call - call an operation of a v4l2_subdev.
  *
@@ -1112,6 +1114,10 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
 			__result = -ENODEV;				\
 		else if (!(__sd->ops->o && __sd->ops->o->f))		\
 			__result = -ENOIOCTLCMD;			\
+		else if (v4l2_subdev_call_wrappers.o &&			\
+			 v4l2_subdev_call_wrappers.o->f)		\
+			__result = v4l2_subdev_call_wrappers.o->f(	\
+							__sd, ##args);	\
 		else							\
 			__result = __sd->ops->o->f(__sd, ##args);	\
 		__result;						\
-- 
2.21.0

Re: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()

[Sakari Ailus]

Hi Janusz,

On Wed, May 15, 2019 at 12:48:21AM +0200, Janusz Krzysztofik wrote:
> -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop *crop)
> +static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
>  {
> -	if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
> -	    crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	if (sd->entity.num_pads && pad >= sd->entity.num_pads)

One more comment.

The num_pads doesn't really tell whether a given op is valid for a device.
Well, in this case it would have to be a bug in the driver, but those do
happen. How about checking for sd->entity.graph_obj.mdev instead? It's
non-NULL if the entity is registered with a media device, i.e. when these
callback functions are supposed to be called.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()

[Janusz Krzysztofik]

Hi Sakari,

On Wednesday, May 15, 2019 9:16:02 AM CEST Sakari Ailus wrote:
> Hi Janusz,
> 
> On Wed, May 15, 2019 at 12:48:21AM +0200, Janusz Krzysztofik wrote:
> > -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop 
*crop)
> > +static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
> >  {
> > -	if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
> > -	    crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +	if (sd->entity.num_pads && pad >= sd->entity.num_pads)
> 
> One more comment.
> 
> The num_pads doesn't really tell whether a given op is valid for a device.
> Well, in this case it would have to be a bug in the driver, but those do
> happen. How about checking for sd->entity.graph_obj.mdev instead? It's
> non-NULL if the entity is registered with a media device, i.e. when these
> callback functions are supposed to be called.

Before I do that, let me undestand your point better.

My intentions were:
1) to provide a check for validity of a pad ID passed to an operation, not ann 
eligibility of a driver to support the operation,
2) to not break drivers which don't set pad_num, especially when building them 
with CONFIG_MEDIA_CONTROLLER turned on for whatever reason.

Since pad IDs are verified against pad_num which may be not set, we should 
obviously check validity of pad_num before comparing against it.  Since media 
controller compatible subdevices need at least one pad, I think the check for 
non-zero pad_num is quite reasonable.

Moreover, old drivers are actually using those pad operations you describe as 
not supposed to be called.  They are using them because they were converted to 
use them in place of former video ops.  Already dealing with pad IDs, they may 
decide to turn on CONFIG_MEDIA_CONTROLLER and use selected functionality, for 
example register pads, without implementing fulll media controller support.  
Why should we refuse to perform pad ID verification for them?

Thanks,
Janusz

Re: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()

[Sakari Ailus]

Hi Janusz,

On Wed, May 15, 2019 at 10:56:36PM +0200, Janusz Krzysztofik wrote:
> Hi Sakari,
> 
> On Wednesday, May 15, 2019 9:16:02 AM CEST Sakari Ailus wrote:
> > Hi Janusz,
> > 
> > On Wed, May 15, 2019 at 12:48:21AM +0200, Janusz Krzysztofik wrote:
> > > -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop 
> *crop)
> > > +static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
> > >  {
> > > -	if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
> > > -	    crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > > +	if (sd->entity.num_pads && pad >= sd->entity.num_pads)
> > 
> > One more comment.
> > 
> > The num_pads doesn't really tell whether a given op is valid for a device.
> > Well, in this case it would have to be a bug in the driver, but those do
> > happen. How about checking for sd->entity.graph_obj.mdev instead? It's
> > non-NULL if the entity is registered with a media device, i.e. when these
> > callback functions are supposed to be called.
> 
> Before I do that, let me undestand your point better.
> 
> My intentions were:
> 1) to provide a check for validity of a pad ID passed to an operation, not ann 
> eligibility of a driver to support the operation,
> 2) to not break drivers which don't set pad_num, especially when building them 
> with CONFIG_MEDIA_CONTROLLER turned on for whatever reason.

Indeed.

But these checks still allow calling the pad operations on sub-devices that
have no pads. That should not be allowed. Pads are a Media controller
concept, they do not exist outside it; therefore checking for pads only if
the subdev is a part of the media device would be entirely correct.

It should probably accompany a check that requires the pad number is zero
if the subdev doesn't have a graph object, even if the pad field isn't
supposedly used for any purpose. Would that address your concern?

> 
> Since pad IDs are verified against pad_num which may be not set, we should 
> obviously check validity of pad_num before comparing against it.  Since media 
> controller compatible subdevices need at least one pad, I think the check for 
> non-zero pad_num is quite reasonable.
> 
> Moreover, old drivers are actually using those pad operations you describe as 
> not supposed to be called.  They are using them because they were converted to 
> use them in place of former video ops.  Already dealing with pad IDs, they may 
> decide to turn on CONFIG_MEDIA_CONTROLLER and use selected functionality, for 
> example register pads, without implementing fulll media controller support.  
> Why should we refuse to perform pad ID verification for them?

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()

[Janusz Krzysztofik]

On Friday, May 17, 2019 5:58:40 PM CEST Sakari Ailus wrote:
> Hi Janusz,
> 
> On Wed, May 15, 2019 at 10:56:36PM +0200, Janusz Krzysztofik wrote:
> > Hi Sakari,
> > 
> > On Wednesday, May 15, 2019 9:16:02 AM CEST Sakari Ailus wrote:
> > > Hi Janusz,
> > > 
> > > On Wed, May 15, 2019 at 12:48:21AM +0200, Janusz Krzysztofik wrote:
> > > > -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop 
> > *crop)
> > > > +static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
> > > >  {
> > > > -	if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
> > > > -	    crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > > > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > > > +	if (sd->entity.num_pads && pad >= sd->entity.num_pads)
> > > 
> > > One more comment.
> > > 
> > > The num_pads doesn't really tell whether a given op is valid for a 
device.
> > > Well, in this case it would have to be a bug in the driver, but those do
> > > happen. How about checking for sd->entity.graph_obj.mdev instead? It's
> > > non-NULL if the entity is registered with a media device, i.e. when 
these
> > > callback functions are supposed to be called.
> > 
> > Before I do that, let me undestand your point better.
> > 
> > My intentions were:
> > 1) to provide a check for validity of a pad ID passed to an operation, not 
ann 
> > eligibility of a driver to support the operation,
> > 2) to not break drivers which don't set pad_num, especially when building 
them 
> > with CONFIG_MEDIA_CONTROLLER turned on for whatever reason.
> 
> Indeed.
> 
> But these checks still allow calling the pad operations on sub-devices that
> have no pads. That should not be allowed. Pads are a Media controller
> concept, they do not exist outside it; therefore checking for pads only if
> the subdev is a part of the media device would be entirely correct.

OK, now I see your point.  You don't want the check to succeed if a media 
entity has num_pads == 0.

> It should probably accompany a check that requires the pad number is zero
> if the subdev doesn't have a graph object, even if the pad field isn't
> supposedly used for any purpose. Would that address your concern?

Yes, that's acceptable.  Let's require subdevice drivers to register as media 
entities if they want to use pads > 0.

I'll update the patches and submit as v7 soon.

Thanks,
Janusz

> > Since pad IDs are verified against pad_num which may be not set, we should 
> > obviously check validity of pad_num before comparing against it.  Since 
media 
> > controller compatible subdevices need at least one pad, I think the check 
for 
> > non-zero pad_num is quite reasonable.
> > 
> > Moreover, old drivers are actually using those pad operations you describe 
as 
> > not supposed to be called.  They are using them because they were 
converted to 
> > use them in place of former video ops.  Already dealing with pad IDs, they 
may 
> > decide to turn on CONFIG_MEDIA_CONTROLLER and use selected functionality, 
for 
> > example register pads, without implementing fulll media controller 
support.  
> > Why should we refuse to perform pad ID verification for them?
> 
> 

[PATCH v6 2/3] media: v4l2-subdev: Verify v4l2_subdev_call() pointer arguments

[Janusz Krzysztofik]

Parameters passed to check helpers are now obtained by dereferencing
unverified pointer arguments.  Check validity of those pointers first.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index c61c95007d89..6933f30e5041 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -141,6 +141,9 @@ static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
 static inline int check_format(struct v4l2_subdev *sd,
 			       struct v4l2_subdev_format *format)
 {
+	if (!format)
+		return -EINVAL;
+
 	return check_which(format->which) ? : check_pad(sd, format->pad);
 }
 
@@ -164,6 +167,9 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd,
 			       struct v4l2_subdev_pad_config *cfg,
 			       struct v4l2_subdev_mbus_code_enum *code)
 {
+	if (!code)
+		return -EINVAL;
+
 	return check_which(code->which) ? : check_pad(sd, code->pad) ? :
 	       sd->ops->pad->enum_mbus_code(sd, cfg, code);
 }
@@ -172,6 +178,9 @@ static int call_enum_frame_size(struct v4l2_subdev *sd,
 				struct v4l2_subdev_pad_config *cfg,
 				struct v4l2_subdev_frame_size_enum *fse)
 {
+	if (!fse)
+		return -EINVAL;
+
 	return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
 	       sd->ops->pad->enum_frame_size(sd, cfg, fse);
 }
@@ -179,6 +188,9 @@ static int call_enum_frame_size(struct v4l2_subdev *sd,
 static inline int check_frame_interval(struct v4l2_subdev *sd,
 				       struct v4l2_subdev_frame_interval *fi)
 {
+	if (!fi)
+		return -EINVAL;
+
 	return check_pad(sd, fi->pad);
 }
 
@@ -200,6 +212,9 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd,
 				    struct v4l2_subdev_pad_config *cfg,
 				    struct v4l2_subdev_frame_interval_enum *fie)
 {
+	if (!fie)
+		return -EINVAL;
+
 	return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
 	       sd->ops->pad->enum_frame_interval(sd, cfg, fie);
 }
@@ -207,6 +222,9 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd,
 static inline int check_selection(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_selection *sel)
 {
+	if (!sel)
+		return -EINVAL;
+
 	return check_which(sel->which) ? : check_pad(sd, sel->pad);
 }
 
@@ -229,6 +247,9 @@ static int call_set_selection(struct v4l2_subdev *sd,
 static inline int check_edid(struct v4l2_subdev *sd,
 			     struct v4l2_subdev_edid *edid)
 {
+	if (!edid)
+		return -EINVAL;
+
 	if (edid->blocks && edid->edid == NULL)
 		return -EINVAL;
 
@@ -248,6 +269,9 @@ static int call_set_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
 static int call_dv_timings_cap(struct v4l2_subdev *sd,
 			       struct v4l2_dv_timings_cap *cap)
 {
+	if (!cap)
+		return -EINVAL;
+
 	return check_pad(sd, cap->pad) ? :
 	       sd->ops->pad->dv_timings_cap(sd, cap);
 }
@@ -255,6 +279,9 @@ static int call_dv_timings_cap(struct v4l2_subdev *sd,
 static int call_enum_dv_timings(struct v4l2_subdev *sd,
 				struct v4l2_enum_dv_timings *dvt)
 {
+	if (!dvt)
+		return -EINVAL;
+
 	return check_pad(sd, dvt->pad) ? :
 	       sd->ops->pad->enum_dv_timings(sd, dvt);
 }
-- 
2.21.0

[PATCH v6 3/3] media: v4l2-subdev: Verify v4l2_subdev_call() pad config argument

[Janusz Krzysztofik]

Extend parameter checks performed by v4l2_subdev_call() with a check for
a non-NULL pad config pointer if V4L2_SUBDEV_FORMAT_TRY format type is
requested so drivers don't need to care.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 6933f30e5041..6a5c4f046723 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -138,20 +138,30 @@ static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
 	return 0;
 }
 
+static int check_cfg(__u32 which, struct v4l2_subdev_pad_config *cfg)
+{
+	if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg)
+		return -EINVAL;
+
+	return 0;
+}
+
 static inline int check_format(struct v4l2_subdev *sd,
+			       struct v4l2_subdev_pad_config *cfg,
 			       struct v4l2_subdev_format *format)
 {
 	if (!format)
 		return -EINVAL;
 
-	return check_which(format->which) ? : check_pad(sd, format->pad);
+	return check_which(format->which) ? : check_pad(sd, format->pad) ? :
+	       check_cfg(format->which, cfg);
 }
 
 static int call_get_fmt(struct v4l2_subdev *sd,
 			struct v4l2_subdev_pad_config *cfg,
 			struct v4l2_subdev_format *format)
 {
-	return check_format(sd, format) ? :
+	return check_format(sd, cfg, format) ? :
 	       sd->ops->pad->get_fmt(sd, cfg, format);
 }
 
@@ -159,7 +169,7 @@ static int call_set_fmt(struct v4l2_subdev *sd,
 			struct v4l2_subdev_pad_config *cfg,
 			struct v4l2_subdev_format *format)
 {
-	return check_format(sd, format) ? :
+	return check_format(sd, cfg, format) ? :
 	       sd->ops->pad->set_fmt(sd, cfg, format);
 }
 
@@ -171,6 +181,7 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd,
 		return -EINVAL;
 
 	return check_which(code->which) ? : check_pad(sd, code->pad) ? :
+	       check_cfg(code->which, cfg) ? :
 	       sd->ops->pad->enum_mbus_code(sd, cfg, code);
 }
 
@@ -182,6 +193,7 @@ static int call_enum_frame_size(struct v4l2_subdev *sd,
 		return -EINVAL;
 
 	return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
+	       check_cfg(fse->which, cfg) ? :
 	       sd->ops->pad->enum_frame_size(sd, cfg, fse);
 }
 
@@ -216,23 +228,26 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd,
 		return -EINVAL;
 
 	return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
+	       check_cfg(fie->which, cfg) ? :
 	       sd->ops->pad->enum_frame_interval(sd, cfg, fie);
 }
 
 static inline int check_selection(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_pad_config *cfg,
 				  struct v4l2_subdev_selection *sel)
 {
 	if (!sel)
 		return -EINVAL;
 
-	return check_which(sel->which) ? : check_pad(sd, sel->pad);
+	return check_which(sel->which) ? : check_pad(sd, sel->pad) ? :
+	       check_cfg(sel->which, cfg);
 }
 
 static int call_get_selection(struct v4l2_subdev *sd,
 			      struct v4l2_subdev_pad_config *cfg,
 			      struct v4l2_subdev_selection *sel)
 {
-	return check_selection(sd, sel) ? :
+	return check_selection(sd, cfg, sel) ? :
 	       sd->ops->pad->get_selection(sd, cfg, sel);
 }
 
@@ -240,7 +255,7 @@ static int call_set_selection(struct v4l2_subdev *sd,
 			      struct v4l2_subdev_pad_config *cfg,
 			      struct v4l2_subdev_selection *sel)
 {
-	return check_selection(sd, sel) ? :
+	return check_selection(sd, cfg, sel) ? :
 	       sd->ops->pad->set_selection(sd, cfg, sel);
 }
 
-- 
2.21.0