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: Mon, 19 Apr 2021 18:33:17 -0700 [thread overview]
Message-ID: <7f5da2c7-13ff-237c-87ba-a456cbac2bca@linux.microsoft.com> (raw)
In-Reply-To: <87pmypdf93.fsf@mpe.ellerman.id.au>
On 4/19/21 4:30 PM, Michael Ellerman wrote:
> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
>> 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().
>
> It feels like the root of the problem is that the kvfree of fdt is in
> the wrong place. It's only allocated later in the function, so the error
> path should reflect that. Something like the patch below.
>
> cheers
>
>
> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> index 5a569bb51349..02662e72c53d 100644
> --- a/arch/powerpc/kexec/elf_64.c
> +++ b/arch/powerpc/kexec/elf_64.c
> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
> initrd_len, cmdline);
> if (ret)
> - goto out;
> + goto out_free_fdt;
>
> fdt_pack(fdt);
>
> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> ret = kexec_add_buffer(&kbuf);
> if (ret)
> - goto out;
> + goto out_free_fdt;
>
> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
> image->arch.fdt = fdt;
> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> if (ret)
> pr_err("Error setting up the purgatory.\n");
>
> + goto out;
> +
> +out_free_fdt:
> + kvfree(fdt);
> out:
> kfree(modified_cmdline);
> kexec_free_elf_info(&elf_info);
>
> - /*
> - * 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;
> }
>
>
This looks good to me. Thanks Michael.
I'll post the updated patch shortly.
-lakshmi
next prev parent reply other threads:[~2021-04-20 1:33 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20210415191437.20212-1-nramas@linux.microsoft.com>
2021-04-15 19:18 ` [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load() Lakshmi Ramasubramanian
2021-04-16 6:44 ` Daniel Axtens
2021-04-16 7:00 ` Christophe Leroy
2021-04-16 8:09 ` Dan Carpenter
2021-04-16 12:19 ` Michael Ellerman
2021-04-16 7:40 ` Dan Carpenter
2021-04-16 9:05 ` Michael Ellerman
2021-04-16 14:37 ` Lakshmi Ramasubramanian
2021-04-19 23:30 ` Michael Ellerman
2021-04-20 1:33 ` Lakshmi Ramasubramanian [this message]
2021-04-20 5:00 ` Dan Carpenter
2021-04-20 5:20 ` Lakshmi Ramasubramanian
2021-04-20 13:06 ` Rob Herring
2021-04-20 14:42 ` Lakshmi Ramasubramanian
2021-04-20 15:04 ` Lakshmi Ramasubramanian
2021-04-20 15:47 ` Rob Herring
2021-04-20 15:55 ` Lakshmi Ramasubramanian
2021-04-22 2:21 ` Daniel Axtens
2021-04-22 8:05 ` David Laight
2021-04-22 9:34 ` Dan Carpenter
2021-04-22 16:54 ` Segher Boessenkool
2021-04-23 13:50 ` Michael Ellerman
2021-04-23 14:42 ` David Laight
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=7f5da2c7-13ff-237c-87ba-a456cbac2bca@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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).