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: <20180718134103.bgwpgk7l6joxtjoa@holly.lan>
Date:   Wed, 18 Jul 2018 14:41:03 +0100
From:   Daniel Thompson <daniel.thompson@...aro.org>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Marcel Ziswiler <marcel.ziswiler@...adex.com>,
        "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>,
        "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, Jul 18, 2018 at 02:08:53PM +0100, Lee Jones wrote:
> > > > > 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.

Only if you initialize num_steps...

We should either initialize to zero and not worry about the return
code[1] or we check the return code and not worry about
initialization[2]. I don't think both are worthwhile.

Whilst initialization can fix this specific instance we generally avoid
overusing it since it messes up static analysis and, in this instance,
distance from declaration to use is >25 lines, hence current patchset.

Daniel.

[1] https://lkml.org/lkml/2018/7/16/399
[2] https://lkml.org/lkml/2018/7/16/1042

Or...

We check the return code and leave number

num_steps is uninitialized and stack allocated so it only has a valid
value if of_property_read_u32() succeeds.

We can (and I originally did) fix the bug by initializing num_steps to 0
but its quite some distance between declaration and use so I accepted
Marcel's counter proposal to check the return code instead.

Daniel.