linux-kernel - Re: [PATCH v2 3/5] media: mt9p031: Implement [gs]

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: <20200930113831.GG5689@pendragon.ideasonboard.com>
Date:   Wed, 30 Sep 2020 14:38:31 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Stefan Riedmueller <s.riedmueller@...tec.de>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        Enrico Scholz <enrico.scholz@...ma-chemnitz.de>
Subject: Re: [PATCH v2 3/5] media: mt9p031: Implement [gs]_register debug
 calls

Hi Stefan,

Thank you for the patch.

On Wed, Sep 30, 2020 at 12:51:31PM +0200, Stefan Riedmueller wrote:
> From: Enrico Scholz <enrico.scholz@...ma-chemnitz.de>
> 
> Implement g_register and s_register v4l2_subdev_core_ops to access
> camera register directly from userspace for debug purposes.

As the name of the operations imply, this is meant for debug purpose
only. They are however prone to be abused to configure the sensor from
userspace in production, which isn't a direction we want to take.
What's your use case for this ?  I'd rather drop this patch and see the
driver extended with support for more controls if needed

> Signed-off-by: Enrico Scholz <enrico.scholz@...ma-chemnitz.de>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@...tec.de>
> ---
> No changes in v2
> ---
>  drivers/media/i2c/mt9p031.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index b4c042f418c1..de36025260a8 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -703,6 +703,30 @@ static int mt9p031_restore_blc(struct mt9p031 *mt9p031)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +static int mt9p031_g_register(struct v4l2_subdev *sd,
> +			      struct v4l2_dbg_register *reg)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	int ret;
> +
> +	ret = mt9p031_read(client, reg->reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	reg->val = ret;
> +	return 0;
> +}
> +
> +static int mt9p031_s_register(struct v4l2_subdev *sd,
> +			      struct v4l2_dbg_register const *reg)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> +	return mt9p031_write(client, reg->reg, reg->val);
> +}
> +#endif
> +
>  static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct mt9p031 *mt9p031 =
> @@ -1000,6 +1024,10 @@ static int mt9p031_close(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
>  
>  static const struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
>  	.s_power        = mt9p031_set_power,
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +	.s_register	= mt9p031_s_register,
> +	.g_register	= mt9p031_g_register,
> +#endif
>  };
>  
>  static const struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {

-- 
Regards,

Laurent Pinchart