Hi Laurent,
Laurent Pinchart wrote:
> Hi Sakari,
>
> On Thursday 23 February 2012 07:57:54 Sakari Ailus wrote:
>> Laurent Pinchart wrote:
>>> 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.ai...@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.
>
> I think I would prefer a BUG() as well. I'm OK with keeping the check. If
> drivers were bug-free this wouldn't be needed at all of course :-)
Changed to BUG_ON() in the next revision.
--
Sakari Ailus
sakari.ai...@iki.fi
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
