linux-kernel - Re: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1943741.XiKEDqKQ7m@z50>
Date:   Wed, 15 May 2019 22:56:36 +0200
From:   Janusz Krzysztofik <jmkrzyszt@...il.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 1/3] media: v4l2-subdev: Verify arguments in v4l2_subdev_call()

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.

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?

Thanks,
Janusz