| Message ID | 20190520212747.368-1-jmkrzyszt@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: v4l2-subdev: Verify arguments in v4l2_subdev_call() | expand |
Hi Janusz, On Mon, May 20, 2019 at 11:27:44PM +0200, Janusz Krzysztofik wrote: > 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 For the set: Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> On the 1st patch __u32 should be u32. I'd suggest to fix that in a separate patch. This was a really nice set. Thank you!
Sakari, Are you OK with this series? Please Ack if that's the case, so that I can merge it. Regards, Hans On 5/20/19 11:27 PM, Janusz Krzysztofik wrote: > 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 | 268 +++++++++++++++++--------- > include/media/v4l2-subdev.h | 6 + > 2 files changed, 188 insertions(+), 86 deletions(-) > > Changelog: > v6->v7: > Changes suggested by Sakari - thanks! > - never succeed pad check on media entities with pad_num == 0, > - allow pad 0 on subdevies not registered as media entities. > > 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! >
Hi,
This patch breaks rcar-vin. I'm sorry I did not find out before it was
merged as a8fa55078a7784a9 ("media: v4l2-subdev: Verify arguments in
v4l2_subdev_call()").
The problem is that rcar-vin calls enum_mbus_code in its bound callback.
At this point call_enum_mbus_code() is invoked which then calls
check_pad(). At this point sd->entity.graph_obj.mdev is not set so the
check if (pad > 0) fails and the binding of the subdevice in rcar-vin
fails.
I'm not sure how to best solve this, suggestions are appreciated. I see
two options, move the call to enum_mbus_code from the bound to the
complete callback or make sure the mdev is associated with the subdev
before the bound callback is invoked. I don't like the former as I think
the complete callback should be removed ;-)
On 2019-05-20 23:27:44 +0200, Janusz Krzysztofik wrote:
> 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 | 268 +++++++++++++++++---------
> include/media/v4l2-subdev.h | 6 +
> 2 files changed, 188 insertions(+), 86 deletions(-)
>
> Changelog:
> v6->v7:
> Changes suggested by Sakari - thanks!
> - never succeed pad check on media entities with pad_num == 0,
> - allow pad 0 on subdevies not registered as media entities.
>
> 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
>
On 6/29/19 5:28 AM, Niklas Söderlund wrote: > Hi, > > This patch breaks rcar-vin. I'm sorry I did not find out before it was > merged as a8fa55078a7784a9 ("media: v4l2-subdev: Verify arguments in > v4l2_subdev_call()"). > > The problem is that rcar-vin calls enum_mbus_code in its bound callback. > At this point call_enum_mbus_code() is invoked which then calls > check_pad(). At this point sd->entity.graph_obj.mdev is not set so the > check if (pad > 0) fails and the binding of the subdevice in rcar-vin > fails. > > I'm not sure how to best solve this, suggestions are appreciated. I see > two options, move the call to enum_mbus_code from the bound to the > complete callback or make sure the mdev is associated with the subdev > before the bound callback is invoked. I don't like the former as I think > the complete callback should be removed ;-) I don't think check_pad() should check mdev. Try this instead: static inline int check_pad(struct v4l2_subdev *sd, __u32 pad) { #if defined(CONFIG_MEDIA_CONTROLLER) if (sd->entity.num_pads) return pad >= sd->entity.num_pads ? -EINVAL : 0; #endif /* allow pad 0 on subdevices not registered as media entities */ return pad ? -EINVAL : 0; } If the subdev defines pads, then sd->entity.num_pads is non-zero and it can be used to check the pad value, otherwise it should just only accept pad 0. And it shouldn't depend on mdev, since that (as you discovered) may not be set yet. Regards, Hans > > On 2019-05-20 23:27:44 +0200, Janusz Krzysztofik wrote: >> 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 | 268 +++++++++++++++++--------- >> include/media/v4l2-subdev.h | 6 + >> 2 files changed, 188 insertions(+), 86 deletions(-) >> >> Changelog: >> v6->v7: >> Changes suggested by Sakari - thanks! >> - never succeed pad check on media entities with pad_num == 0, >> - allow pad 0 on subdevies not registered as media entities. >> >> 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 >>