On 4/16/21 2:05 AM, Michael Ellerman wrote:
Daniel Axtens <dja@xxxxxxxxxx> writes:
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...
The out label reads in part:
/* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
return ret ? ERR_PTR(ret) : fdt;
As far as I can tell, any time we get a non-zero ret, we're going to
return an error pointer rather than the uninitialised value...
As Dan pointed out, the new code is in linux-next.
I have copied the new one below - the function doesn't return fdt, but
instead sets it in the arch specific field (please see the link to the
updated elf_64.c below).
https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
(btw, it does look like we might leak fdt if we have an error after we
successfully kmalloc it.)
Am I missing something? Can you link to the report for the kernel test
robot or from Dan?
/*
* Once FDT buffer has been successfully passed to
kexec_add_buffer(),
* the FDT buffer address is saved in image->arch.fdt. In that
case,
* the memory cannot be freed here in case of any other error.
*/
if (ret && !image->arch.fdt)
kvfree(fdt);
return ret ? ERR_PTR(ret) : NULL;
In case of an error, the memory allocated for fdt is freed unless it has
already been passed to kexec_add_buffer().
thanks,
-lakshmi
FWIW, I think it's worth including this patch _anyway_ because initing
local variables is good practice, but I'm just not sure on the
justification.
Why is it good practice?
It defeats -Wuninitialized. So you're guaranteed to be returning
something initialised, but not necessarily initialised to the right
value.
In a case like this NULL seems like a safe choice, but it's still wrong.
The function is meant to return a pointer to the successfully allocated
fdt, or an ERR_PTR() value. NULL is neither of those.
I agree there are security reasons that initialising stack variables is
desirable, but I think that should be handled by the compiler, not at
the source level.
cheers
