Re: [PATCH v3 06/33] v4l: Check pad number in get try pointer functions

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Laurent,

Laurent Pinchart wrote:
> Thanks for the patch.

Thanks for the review! :-)

> On Monday 20 February 2012 03:56:45 Sakari Ailus wrote:
>> Unify functions to get try pointers and validate the pad number accessed by
>> the user.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>> ---
>>  include/media/v4l2-subdev.h |   31 ++++++++++++++-----------------
>>  1 files changed, 14 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index bcaf6b8..d48dae5 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -565,23 +565,20 @@ struct v4l2_subdev_fh {
>>  	container_of(fh, struct v4l2_subdev_fh, vfh)
>>
>>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>> -static inline struct v4l2_mbus_framefmt *
>> -v4l2_subdev_get_try_format(struct v4l2_subdev_fh *fh, unsigned int pad)
>> -{
>> -	return &fh->pad[pad].try_fmt;
>> -}
>> -
>> -static inline struct v4l2_rect *
>> -v4l2_subdev_get_try_crop(struct v4l2_subdev_fh *fh, unsigned int pad)
>> -{
>> -	return &fh->pad[pad].try_crop;
>> -}
>> -
>> -static inline struct v4l2_rect *
>> -v4l2_subdev_get_try_compose(struct v4l2_subdev_fh *fh, unsigned int pad)
>> -{
>> -	return &fh->pad[pad].try_compose;
>> -}
>> +#define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name)		\
>> +	static inline struct rtype *					\
>> +	v4l2_subdev_get_try_##fun_name(struct v4l2_subdev_fh *fh,	\
>> +				       unsigned int pad)		\
>> +	{								\
>> +		if (unlikely(pad > vdev_to_v4l2_subdev(			\
>> +				     fh->vfh.vdev->entity.num_pads)	\
>> +			return NULL;					\
>> +		return &fh->pad[pad].field_name;			\
>> +	}
>> +
>> +__V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, format, try_fmt)
>> +__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, crop, try_compose)
>> +__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, compose, try_compose)
>>  #endif
>>
>>  extern const struct v4l2_file_operations v4l2_subdev_fops;
> 
> I'm not sure if this is a good idea. Drivers usually access the active and try 
> formats/rectangles through a single function that checks the which argument 
> and returns the active format/rectangle from the driver-specific device 
> structure, or calls v4l2_subdev_get_try_*. The pad number should be checked 
> for both active and try formats/rectangles, as both can result in accessing a 
> wrong memory location.
> 
> Furthermore, only in-kernel access to the active/try formats/rectangles need 
> to be checked, as the pad argument to subdev ioctls are already checked in 
> v4l2-subdev.c. If your goal is to catch buggy kernel code here, a BUG_ON might 
> be more suitable (although accessing the NULL pointer would result in an oops 
> anyway).

This was basically the reason for the memory corryption issue I had some
time ago with the driver. The drivers (typically, I guess) need to
access this data also to validate the following selection rectangles
inside the subdev.

The active rectangles are also driver's own property so it's the matter
of driver to access them properly. In principle the same goes for the
try rectangles, but the fact still is that this patch would have caught
the bad accesses right at the time they were made. I feel it's just too
easy to give the function a faulty pad number --- see the SMIA++ driver
for an example.

I'd prefer to keep this change, and also I'm fine with doing BUG()
instead of returning NULL.

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi