linux-kernel - Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable

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: <1531920370.16896.25.camel@toradex.com>
Date:   Wed, 18 Jul 2018 13:26:12 +0000
From:   Marcel Ziswiler <marcel.ziswiler@...adex.com>
To:     "lee.jones@...aro.org" <lee.jones@...aro.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jingoohan1@...il.com" <jingoohan1@...il.com>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        "linux-fbdev@...r.kernel.org" <linux-fbdev@...r.kernel.org>,
        "b.zolnierkie@...sung.com" <b.zolnierkie@...sung.com>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        "daniel.thompson@...aro.org" <daniel.thompson@...aro.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "patches@...aro.org" <patches@...aro.org>
Subject: Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable

On Wed, 2018-07-18 at 14:08 +0100, Lee Jones wrote:
> On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
> 
> > On Wed, 2018-07-18 at 11:12 +0100, Daniel Thompson wrote:
> > > On Wed, Jul 18, 2018 at 10:53:35AM +0100, Lee Jones wrote:
> > > > On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
> > > > 
> > > > > On Wed, 2018-07-18 at 09:09 +0100, Lee Jones wrote:
> > > > > > On Mon, 16 Jul 2018, Daniel Thompson wrote:
> > > > > > 
> > > > > > > Currently, if the DT does not define num-interpolated-
> > > > > > > steps
> > > > > > > then
> > > > > > > num_steps is undefined and the interpolation code will
> > > > > > > deploy
> > > > > > > randomly.
> > > > > > > Fix this.
> > > > > > > 
> > > > > > > Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear
> > > > > > > interpolation
> > > > > > > between
> > > > > > > brightness-levels")
> > > > > > > Reported-by: Marcel Ziswiler <marcel.ziswiler@...adex.com
> > > > > > > >
> > > > > > > Signed-off-by: Daniel Thompson <daniel.thompson@...aro.or
> > > > > > > g>
> > > > > > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@...adex.c
> > > > > > > om>
> > > > > > 
> > > > > > This line is confusing.  Did you guys author this patch
> > > > > > together?
> > > > > 
> > > > > Yes, I reported it and we came to a conclusion together.
> > > > 
> > > > It sounds like you need to have all of the tags (except this
> > > > one).
> > > > :)
> > > > 
> > > >  Reported-by:  for reporting the issue
> > > >  Suggested-by: for suggesting a resolution
> > > >  Acked-by:     for reviewing it
> > > >  Tested-by:    for testing it
> > > > 
> > > > Signed-off-by usually means you either wrote a significant
> > > > amount
> > > > of
> > > > the diffstat or you were part of the submission path.
> > > 
> > > He did [I don't object to but wouldn't have used the extra
> > > brackets
> > > you
> > > brought up ;-) ].
> > 
> > Yes, I take all the blame for the extra brackets. Regardless of
> > having
> > a masters in CS or not I still prefer too many then too few of them
> > (;-
> > p).
> > 
> > > > > > My guess is that this line should be dropped and the RB and
> > > > > > TB
> > > > > > tags
> > > > > > should remain?  If it was reviewed too, perhaps an AB too?
> > > > > 
> > > > > I'm OK either way and do not need any explicit authorship to
> > > > > be
> > > > > expressed for me.
> > > > 
> > > > In this instance I suggest keeping Reported-by and Tested-by.
> > > > 
> > > > > > > Tested-by: Marcel Ziswiler <marcel.ziswiler@...adex.com>
> > > > > > > ---
> > > > > > >  drivers/video/backlight/pwm_bl.c | 17 ++++++++---------
> > > > > > >  1 file changed, 8 insertions(+), 9 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c
> > > > > > > b/drivers/video/backlight/pwm_bl.c
> > > > > > > index 9ee4c1b735b2..e3c22b79fbcd 100644
> > > > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > > > @@ -299,15 +299,14 @@ static int
> > > > > > > pwm_backlight_parse_dt(struct
> > > > > > > device *dev,
> > > > > > >  		 * interpolation between each of the
> > > > > > > values
> > > > > > > of
> > > > > > > brightness levels
> > > > > > >  		 * and creates a new pre-computed table.
> > > > > > >  		 */
> > > > > > > -		of_property_read_u32(node, "num-
> > > > > > > interpolated-
> > > > > > > steps",
> > > > > > > -				     &num_steps);
> > > > > > > -
> > > > > > > -		/*
> > > > > > > -		 * Make sure that there is at least two
> > > > > > > entries in
> > > > > > > the
> > > > > > > -		 * brightness-levels table, otherwise we
> > > > > > > can't
> > > > > > > interpolate
> > > > > > > -		 * between two points.
> > > > > > > -		 */
> > > > > > > -		if (num_steps) {
> > > > > > > +		if ((of_property_read_u32(node, "num-
> > > > > > > interpolated-
> > > > > > > steps",
> > > > > > > +					  &num_steps) ==
> > > > > > > 0)
> > > > > > > &&
> > > > > > > num_steps) {
> > > > > > 
> > > > > > This is pretty ugly, and isn't it suffering from over-
> > > > > > bracketing?  My
> > > > > > suggestion would be to break out the invocation of
> > > > > > of_property_read_u32() from the if and test only the
> > > > > > result.
> > > > > > 
> > > > > > 		of_property_read_u32(node, "num-interpolated-
> > > > > > steps", 
> > > > > > &num_steps);
> > > > > 
> > > > > you mean:
> > > > > 
> > > > > 		ret = of_property_read_u32(node, "num-
> > > > > interpolated-
> > > > > steps", &num_steps);
> > > > > 
> > > > > > 		if (!ret && num_steps) {
> > > > > > 
> > > > > > I haven't checked the underling code, but is it even
> > > > > > feasible
> > > > > > for
> > > > > > of_property_read_u32() to not succeed AND for num_steps to
> > > > > > be
> > > > > > set?
> > > > > > 
> > > > > > If not, the check for !ret if superfluous and you can drop
> > > > > > it.
> > > > > 
> > > > > No, then we are back to the initial issue of num_steps
> > > > > potentially not
> > > > > being initialised. We really want both of_property_read_u32()
> > > > > to
> > > > > succeed AND num_steps to actually be set.
> > > > 
> > > > I also think num_steps should be pre-initialised.
> > 
> > Yes, I guess it definitely does not hurt.
> > 
> > > > Then it will only be set if of_property_read_u32() succeeds.
> > 
> > Yes, but we still need to check for both, the function not failing
> > and
> > num_steps to actually be non zero.
> 
> Why?  You don't do anything differently if it fails.

Well, maybe we should but given this being an optional property nobody
cared.