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

[Janusz Krzysztofik]

On Friday, May 17, 2019 5:58:40 PM CEST Sakari Ailus wrote:
> 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.

OK, now I see your point.  You don't want the check to succeed if a media 
entity has num_pads == 0.

> 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?

Yes, that's acceptable.  Let's require subdevice drivers to register as media 
entities if they want to use pads > 0.

I'll update the patches and submit as v7 soon.

Thanks,
Janusz

> > 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?
> 
>