Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, Apr 22, 2021 at 08:05:27AM +0000, David Laight wrote:
> From: Daniel Axtens
> > Sent: 22 April 2021 03:21
> > 
> > > Hi Lakshmi,
> > >
> > >> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> > >>
> > >> Sorry - missed copying device-tree and powerpc mailing lists.
> > >>
> > >>> There are a few "goto out;" statements before the local variable "fdt"
> > >>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> > >>> elf64_load(). This will result in an uninitialized "fdt" being passed
> > >>> to kvfree() in this function if there is an error before the call to
> > >>> of_kexec_alloc_and_setup_fdt().
> > >>>
> > >>> Initialize the local variable "fdt" to NULL.
> > >>>
> > > I'm a huge fan of initialising local variables! But I'm struggling to
> > > find the code path that will lead to an uninit fdt being returned...
> > 
> > OK, so perhaps this was putting it too strongly. I have been bitten
> > by uninitialised things enough in C that I may have taken a slightly
> > overly-agressive view of fixing them in the source rather than the
> > compiler. I do think compiler-level mitigations are better, and I take
> > the point that we don't want to defeat compiler checking.
> > 
> > (Does anyone - and by anyone I mean any large distro - compile with
> > local variables inited by the compiler?)
> 
> There are compilers that initialise locals to zero for 'debug' builds
> and leave the 'random' for optimised 'release' builds.
> Lets not test what we are releasing!

We're eventually going to move to a world where initializing to zero
it the default for the kernel.  I think people will still want to
initialize to a poison value for debug builds.

Initializing to zero is better for debugging because it's more
predictable.  An it avoid information leaks.  And dereferencing random
uninitialized pointers is a privilege escalation but dereferencing a
NULL is just an Oops.

The speed impact is not very significant because (conceptually) it only
needs to be done where there is a compiler warning about uninitialized
variables.  It's slightly more complicated in real life.  In this case,
the compiler doesn't know what happens inside the kexec_build_elf_info()
function so it silences the warning.  And GCC silences warnings if the
variable is initialized inside a loop even when it doesn't know that we
enter the loop.

regards,
dan carpenter