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

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

On Fri, Apr 16, 2021 at 04:44:30PM +1000, Daniel Axtens wrote:
> 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!

Don't be!  It just disables static checker warnings and hides bugs.
The kbuild emails are archived but the email is mangled and unreadable.
https://www.mail-archive.com/kbuild@lists.01.org/msg06371.html

I think maybe you're not on the most recent code.  In linux-next this
code looks like:

arch/powerpc/kexec/elf_64.c
    27  static void *elf64_load(struct kimage *image, char *kernel_buf,
    28                          unsigned long kernel_len, char *initrd,
    29                          unsigned long initrd_len, char *cmdline,
    30                          unsigned long cmdline_len)
    31  {
    32          int ret;
    33          unsigned long kernel_load_addr;
    34          unsigned long initrd_load_addr = 0, fdt_load_addr;
    35          void *fdt;
    36          const void *slave_code;
    37          struct elfhdr ehdr;
    38          char *modified_cmdline = NULL;
    39          struct kexec_elf_info elf_info;
    40          struct kexec_buf kbuf = { .image = image, .buf_min = 0,
    41                                    .buf_max = ppc64_rma_size };
    42          struct kexec_buf pbuf = { .image = image, .buf_min = 0,
    43                                    .buf_max = ppc64_rma_size, .top_down = true,
    44                                    .mem = KEXEC_BUF_MEM_UNKNOWN };
    45  
    46          ret = kexec_build_elf_info(kernel_buf, kernel_len, &ehdr, &elf_info);
    47          if (ret)
    48                  goto out;
                        ^^^^^^^^
I really despise "goto out;" because freeing things which haven't been
allocated is always dangerous.

[ snip ].


   143  out:
   144          kfree(modified_cmdline);
   145          kexec_free_elf_info(&elf_info);
                                     ^^^^^^^^
There is a possibility that "elf_info" has holds uninitialized stack
data if elf_read_ehdr() fails so that's probably fixing as well.  kexec()
is root only so this can't be exploited.

   146  
   147          /*
   148           * Once FDT buffer has been successfully passed to kexec_add_buffer(),
   149           * the FDT buffer address is saved in image->arch.fdt. In that case,
   150           * the memory cannot be freed here in case of any other error.
   151           */
   152          if (ret && !image->arch.fdt)
   153                  kvfree(fdt);
                               ^^^
Uninitialized.

   154  
   155          return ret ? ERR_PTR(ret) : NULL;
   156  }

regards,
dan carpenter