Daniel Axtens <dja@xxxxxxxxxx> writes: > 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?) I was reading the version in powerpc/next, clearly I should have looked at linux-next. Having said that, I think I will leave the rest of the bikeshedding to the rest of you, you all seem to have it in hand :) Kind regards, Daniel > > 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... > > (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? > > 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. > > Kind regards, > Daniel > >>> Signed-off-by: Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx> >>> Reported-by: kernel test robot <lkp@xxxxxxxxx> >>> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> >>> --- >>> arch/powerpc/kexec/elf_64.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c >>> index 5a569bb51349..0051440c1f77 100644 >>> --- a/arch/powerpc/kexec/elf_64.c >>> +++ b/arch/powerpc/kexec/elf_64.c >>> @@ -32,7 +32,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf, >>> int ret; >>> unsigned long kernel_load_addr; >>> unsigned long initrd_load_addr = 0, fdt_load_addr; >>> - void *fdt; >>> + void *fdt = NULL; >>> const void *slave_code; >>> struct elfhdr ehdr; >>> char *modified_cmdline = NULL; >>> >> >> thanks, >> -lakshmi
