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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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