From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> To: Michael Ellerman <mpe@ellerman.id.au>, Daniel Axtens <dja@axtens.net>, robh@kernel.org, dan.carpenter@oracle.com Cc: devicetree@vger.kernel.org, linuxppc-dev <linuxppc-dev@lists.ozlabs.org>, kbuild-all@lists.01.org, bauerman@linux.ibm.com, lkp@intel.com Subject: Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load() Date: Fri, 16 Apr 2021 07:37:39 -0700 [thread overview] Message-ID: <2817d674-d420-580f-a0c1-b842da915a80@linux.microsoft.com> (raw) In-Reply-To: <87tuo6eh0j.fsf@mpe.ellerman.id.au> On 4/16/21 2:05 AM, Michael Ellerman wrote: > Daniel Axtens <dja@axtens.net> 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 >
WARNING: multiple messages have this Message-ID (diff)
From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> To: kbuild-all@lists.01.org Subject: Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load() Date: Fri, 16 Apr 2021 07:37:39 -0700 [thread overview] Message-ID: <2817d674-d420-580f-a0c1-b842da915a80@linux.microsoft.com> (raw) In-Reply-To: <87tuo6eh0j.fsf@mpe.ellerman.id.au> [-- Attachment #1: Type: text/plain, Size: 2894 bytes --] On 4/16/21 2:05 AM, Michael Ellerman wrote: > Daniel Axtens <dja@axtens.net> 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 >
next prev parent reply other threads:[~2021-04-16 14:37 UTC|newest] Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-15 19:14 [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load() Lakshmi Ramasubramanian 2021-04-15 19:18 ` Lakshmi Ramasubramanian 2021-04-15 19:18 ` Lakshmi Ramasubramanian 2021-04-15 19:18 ` Lakshmi Ramasubramanian 2021-04-16 6:44 ` Daniel Axtens 2021-04-16 6:44 ` Daniel Axtens 2021-04-16 6:44 ` Daniel Axtens 2021-04-16 7:00 ` Christophe Leroy 2021-04-16 7:00 ` Christophe Leroy 2021-04-16 8:09 ` Dan Carpenter 2021-04-16 8:09 ` Dan Carpenter 2021-04-16 8:09 ` Dan Carpenter 2021-04-16 12:19 ` Michael Ellerman 2021-04-16 12:19 ` Michael Ellerman 2021-04-16 7:40 ` Dan Carpenter 2021-04-16 7:40 ` Dan Carpenter 2021-04-16 7:40 ` Dan Carpenter 2021-04-16 9:05 ` Michael Ellerman 2021-04-16 9:05 ` Michael Ellerman 2021-04-16 14:37 ` Lakshmi Ramasubramanian [this message] 2021-04-16 14:37 ` Lakshmi Ramasubramanian 2021-04-19 23:30 ` Michael Ellerman 2021-04-19 23:30 ` Michael Ellerman 2021-04-20 1:33 ` Lakshmi Ramasubramanian 2021-04-20 1:33 ` Lakshmi Ramasubramanian 2021-04-20 5:00 ` Dan Carpenter 2021-04-20 5:00 ` Dan Carpenter 2021-04-20 5:00 ` Dan Carpenter 2021-04-20 5:20 ` Lakshmi Ramasubramanian 2021-04-20 5:20 ` Lakshmi Ramasubramanian 2021-04-20 5:20 ` Lakshmi Ramasubramanian 2021-04-20 13:06 ` Rob Herring 2021-04-20 13:06 ` Rob Herring 2021-04-20 13:06 ` Rob Herring 2021-04-20 14:42 ` Lakshmi Ramasubramanian 2021-04-20 14:42 ` Lakshmi Ramasubramanian 2021-04-20 14:42 ` Lakshmi Ramasubramanian 2021-04-20 15:04 ` Lakshmi Ramasubramanian 2021-04-20 15:04 ` Lakshmi Ramasubramanian 2021-04-20 15:04 ` Lakshmi Ramasubramanian 2021-04-20 15:47 ` Rob Herring 2021-04-20 15:47 ` Rob Herring 2021-04-20 15:47 ` Rob Herring 2021-04-20 15:55 ` Lakshmi Ramasubramanian 2021-04-20 15:55 ` Lakshmi Ramasubramanian 2021-04-20 15:55 ` Lakshmi Ramasubramanian 2021-04-22 2:21 ` Daniel Axtens 2021-04-22 2:21 ` Daniel Axtens 2021-04-22 2:21 ` Daniel Axtens 2021-04-22 8:05 ` David Laight 2021-04-22 8:05 ` David Laight 2021-04-22 9:34 ` Dan Carpenter 2021-04-22 9:34 ` Dan Carpenter 2021-04-22 9:34 ` Dan Carpenter 2021-04-22 16:54 ` Segher Boessenkool 2021-04-22 16:54 ` Segher Boessenkool 2021-04-23 13:50 ` Michael Ellerman 2021-04-23 13:50 ` Michael Ellerman 2021-04-23 14:42 ` David Laight 2021-04-23 14:42 ` David Laight 2021-04-23 15:11 ` Rob Herring 2021-04-23 15:11 ` Rob Herring 2021-04-23 15:11 ` Rob Herring
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=2817d674-d420-580f-a0c1-b842da915a80@linux.microsoft.com \ --to=nramas@linux.microsoft.com \ --cc=bauerman@linux.ibm.com \ --cc=dan.carpenter@oracle.com \ --cc=devicetree@vger.kernel.org \ --cc=dja@axtens.net \ --cc=kbuild-all@lists.01.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=lkp@intel.com \ --cc=mpe@ellerman.id.au \ --cc=robh@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.