Re: [RFC PATCH] media: v4l2-subdev: Verify arguments of v4l2_subdev_call()

[Hans Verkuil]

On 5/10/19 12:09 AM, Janusz Krzysztofik wrote:
> 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
> reimplementing those checks inside drivers.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@xxxxxxxxx>

I'm very much opposed to this as it creates one horrible macro.

The checks must be done for ioctls, since that's called by userspace and
it is therefor untrusted. But v4l2_subdev_call is just an internal
function call, and if you pass wrong arguments, then that's a driver bug.

If you want to avoid code duplication, then a much better approach is to
create helpers functions such as check_format() in e.g. v4l2-common.c that
can be called from wherever it is needed.

Regards,

	Hans

> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 121 +++-----------------------
>  include/media/v4l2-subdev.h           |  79 +++++++++++++++++
>  2 files changed, 89 insertions(+), 111 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index d75815ab0d7b..186749d31abf 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -121,53 +121,17 @@ static int subdev_close(struct file *file)
>  }
>  
>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> -static int check_format(struct v4l2_subdev *sd,
> -			struct v4l2_subdev_format *format)
> +void *v4l2_subdev_call_va_arg(int n, ...)
>  {
> -	if (format->which != V4L2_SUBDEV_FORMAT_TRY &&
> -	    format->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> -		return -EINVAL;
> -
> -	if (format->pad >= sd->entity.num_pads)
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
> -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop *crop)
> -{
> -	if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
> -	    crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> -		return -EINVAL;
> -
> -	if (crop->pad >= sd->entity.num_pads)
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
> -static int check_selection(struct v4l2_subdev *sd,
> -			   struct v4l2_subdev_selection *sel)
> -{
> -	if (sel->which != V4L2_SUBDEV_FORMAT_TRY &&
> -	    sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> -		return -EINVAL;
> -
> -	if (sel->pad >= sd->entity.num_pads)
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
> -static int check_edid(struct v4l2_subdev *sd, struct v4l2_subdev_edid *edid)
> -{
> -	if (edid->pad >= sd->entity.num_pads)
> -		return -EINVAL;
> -
> -	if (edid->blocks && edid->edid == NULL)
> -		return -EINVAL;
> -
> -	return 0;
> +	va_list ap;
> +	int i;
> +	void *p;
> +
> +	va_start(ap, n);
> +	for (i = 9; i < n; i++)
> +		p = va_arg(ap, void *);
> +	va_end(ap);
> +	return p;
>  }
>  #endif
>  
> @@ -292,10 +256,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 +264,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 +273,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 +292,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 +309,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 +317,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 +325,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 +332,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 +339,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 +347,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 +355,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 +363,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..a0ad8c6f588b 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1091,6 +1091,82 @@ 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);
>  
> +#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> +void *v4l2_subdev_call_va_arg(int n, ...);
> +
> +#define v4l2_subdev_call_chk_args(sd, o, f, args...)			\
> +({									\
> +	__u32 __which = V4L2_SUBDEV_FORMAT_ACTIVE;			\
> +	__u32 __pad = 0;						\
> +	if ((void *)&sd->ops->o == &sd->ops->pad) {			\
> +		if ((void *)&sd->ops->o->f == &sd->ops->pad->get_fmt ||	\
> +		    (void *)&sd->ops->o->f == &sd->ops->pad->set_fmt) {	\
> +			struct v4l2_subdev_format *__fmt;		\
> +			__fmt = v4l2_subdev_call_va_arg(2, ##args);	\
> +			__which = __fmt->which;				\
> +			__pad = __fmt->pad;				\
> +		} else if ((void *)&sd->ops->o->f ==			\
> +				   &sd->ops->pad->enum_mbus_code) {	\
> +			struct v4l2_subdev_mbus_code_enum *__code;	\
> +			__code = v4l2_subdev_call_va_arg(2, ##args);	\
> +			__which = __code->which;			\
> +			__pad = __code->pad;				\
> +		} else if ((void *)&sd->ops->o->f ==			\
> +				   &sd->ops->pad->enum_frame_size) {	\
> +			struct v4l2_subdev_frame_size_enum *__fse;	\
> +			__fse = v4l2_subdev_call_va_arg(2, ##args);	\
> +			__which = __fse->which;				\
> +			__pad = __fse->pad;				\
> +		} else if ((void *)&sd->ops->o->f ==			\
> +				   &sd->ops->pad->enum_frame_interval) {\
> +			struct v4l2_subdev_frame_interval_enum *__fie;	\
> +			__fie = v4l2_subdev_call_va_arg(2, ##args);	\
> +			__which = __fie->which;				\
> +			__pad = __fie->pad;				\
> +		} else if ((void *)&sd->ops->o->f ==			\
> +				   &sd->ops->pad->get_selection ||	\
> +			   (void *)&sd->ops->o->f ==			\
> +				   &sd->ops->pad->set_selection) {	\
> +			struct v4l2_subdev_selection *__sel;		\
> +			__sel = v4l2_subdev_call_va_arg(2, ##args);	\
> +			__which = __sel->which;				\
> +			__pad = __sel->pad;				\
> +		} else if ((void *)&sd->ops->o->f ==			\
> +				   &sd->ops->pad->get_edid ||		\
> +			   (void *)&sd->ops->o->f ==			\
> +				   &sd->ops->pad->set_edid) {		\
> +			struct v4l2_subdev_edid *__edid;		\
> +			__edid = v4l2_subdev_call_va_arg(1, ##args);	\
> +			__pad = __edid->pad;				\
> +		} else if ((void *)&sd->ops->o->f ==			\
> +				   &sd->ops->pad->dv_timings_cap) {	\
> +			struct v4l2_dv_timings_cap *__cap;		\
> +			__cap = v4l2_subdev_call_va_arg(1, ##args);	\
> +			__pad = __cap->pad;				\
> +		} else if ((void *)&sd->ops->o->f ==			\
> +				   &sd->ops->pad->enum_dv_timings) {	\
> +			struct v4l2_enum_dv_timings *__dvt;		\
> +			__dvt = v4l2_subdev_call_va_arg(1, ##args);	\
> +			__pad = __dvt->pad;				\
> +		}							\
> +	} else if ((void *)&sd->ops->o == &sd->ops->video) {		\
> +		if ((void *)&sd->ops->o->f ==				\
> +			    &sd->ops->video->g_frame_interval ||	\
> +		    (void *)&sd->ops->o->f ==				\
> +			    &sd->ops->video->s_frame_interval) {	\
> +			struct v4l2_subdev_frame_interval *__fi;	\
> +			__fi = v4l2_subdev_call_va_arg(1, ##args);	\
> +			__pad = __fi->pad;				\
> +		}							\
> +	}								\
> +	(__which == V4L2_SUBDEV_FORMAT_ACTIVE ||			\
> +	 __which == V4L2_SUBDEV_FORMAT_TRY) &&				\
> +	__pad < (sd->entity.num_pads ? : 1) ? 0 : -EINVAL;		\
> +})
> +#else
> +#define v4l2_subdev_call_chk_args(sd, o, f, args...) 0
> +#endif
> +
>  /**
>   * v4l2_subdev_call - call an operation of a v4l2_subdev.
>   *
> @@ -1112,6 +1188,9 @@ 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_chk_args(sd, o, f, ##args))	\
> +			__result = v4l2_subdev_call_chk_args(sd, o, f,	\
> +							     ##args);	\
>  		else							\
>  			__result = __sd->ops->o->f(__sd, ##args);	\
>  		__result;						\
>