linux-kernel - Re: [RFC][PATCH] media: v4l2-ctrls: add more NULL pointer checks

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: <20200416113249.GG30641@jagdpanzerIV.localdomain>
Date:   Thu, 16 Apr 2020 20:32:49 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky@...il.com>
To:     Hans Verkuil <hverkuil-cisco@...all.nl>
Cc:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Tomasz Figa <tfiga@...omium.org>, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH] media: v4l2-ctrls: add more NULL pointer checks

On (20/04/16 10:53), Hans Verkuil wrote:
[..]
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -2869,6 +2869,9 @@ EXPORT_SYMBOL(v4l2_ctrl_add_handler);
> >  
> >  bool v4l2_ctrl_radio_filter(const struct v4l2_ctrl *ctrl)
> >  {
> > +	if (WARN_ON(!ctrl))
> > +		return false;
> > +
> >  	if (V4L2_CTRL_ID2WHICH(ctrl->id) == V4L2_CTRL_CLASS_FM_TX)
> >  		return true;
> >  	if (V4L2_CTRL_ID2WHICH(ctrl->id) == V4L2_CTRL_CLASS_FM_RX)
> > @@ -3794,7 +3797,9 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl)
> >  	struct v4l2_ext_control c;
> >  
> >  	/* It's a driver bug if this happens. */
> > -	WARN_ON(!ctrl->is_int);
> > +	if (WARN_ON(!ctrl || !ctrl->is_int))
> > +		return -EINVAL;
> 
> Just return 0 here. The return value is the control's value, not an error code.
> So all you can do here is return 0 in the absence of anything better.

OK.

> > +
> >  	c.value = 0;
> >  	get_ctrl(ctrl, &c);
> >  	return c.value;
> > @@ -4212,6 +4217,9 @@ EXPORT_SYMBOL(v4l2_s_ctrl);
> >  
> >  int __v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
> >  {
> > +	if (!ctrl)
> 
> Change this to 'if (WARN_ON(!ctrl))'
> 
> I don't think NULL pointers should be silently ignored: it really
> indicates a driver bug. It it certainly a good idea to WARN instead.

Should WARN_ON() be only in unlocked versions of ctrl API? It probably
would make sense to add WARNs to both - e.g. to v4l2_ctrl_s_ctrl() and
to __v4l2_ctrl_s_ctrl(). By the way, why don't locked and unlocked
versions live together in v4l2-ctrls.c file? Any reason for, e.g.,
v4l2_ctrl_s_ctrl() to be in header and __v4l2_ctrl_s_ctrl() to be C-file?

> The same is true for the functions below.

OK.

	-ss