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@xxxxxxxxxxxx/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
