From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.1 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 32145C433B4 for ; Tue, 20 Apr 2021 15:56:05 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id F27A3613C2 for ; Tue, 20 Apr 2021 15:56:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F27A3613C2 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.microsoft.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4FPpFF6LRxz30Dk for ; Wed, 21 Apr 2021 01:56:01 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=linux.microsoft.com header.i=@linux.microsoft.com header.a=rsa-sha256 header.s=default header.b=JcAA/vrV; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=linux.microsoft.com (client-ip=13.77.154.182; helo=linux.microsoft.com; envelope-from=nramas@linux.microsoft.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=linux.microsoft.com header.i=@linux.microsoft.com header.a=rsa-sha256 header.s=default header.b=JcAA/vrV; dkim-atps=neutral Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by lists.ozlabs.org (Postfix) with ESMTP id 4FPpDn4LbRz2xdL for ; Wed, 21 Apr 2021 01:55:37 +1000 (AEST) Received: from [192.168.0.104] (c-73-42-176-67.hsd1.wa.comcast.net [73.42.176.67]) by linux.microsoft.com (Postfix) with ESMTPSA id 4CA5320B8002; Tue, 20 Apr 2021 08:55:35 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 4CA5320B8002 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1618934135; bh=YhK0PtMMOEGaHAQ0p2yhKEPU4CuC92LSXKajIv12RtA=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=JcAA/vrVyRKJGeL8RUVuWGoFuFRuHZI3t/Ew8dwn3Kd6FhmNt2nhFYwjZKTVSeP/J sfRZJY6gapnxZLGf+iu22f9U0j7Zpe5mkLO7Tap40xRwdMCXLWrGxhtutZWm6YfU/Q QiiOvxrGOC6LIUe2BuS9jPHETiz6LlJ4OmxPbX8A= Subject: Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load() To: Rob Herring References: <20210415191437.20212-1-nramas@linux.microsoft.com> <4edb1433-4d1e-5719-ec9c-fd232b7cf71f@linux.microsoft.com> <87eefag241.fsf@linkitivity.dja.id.au> <87tuo6eh0j.fsf@mpe.ellerman.id.au> <2817d674-d420-580f-a0c1-b842da915a80@linux.microsoft.com> <87pmypdf93.fsf@mpe.ellerman.id.au> <20210420050015.GA1959@kadam>

<2e8dd39b-0372-4874-340e-6f87185091cc@linux.microsoft.com> <433b4518-0a83-5a97-ac07-da8748dcc90e@linux.microsoft.com> From: Lakshmi Ramasubramanian Message-ID: <5fb4f2d0-5b2d-4358-befe-f7748b6d6430@linux.microsoft.com> Date: Tue, 20 Apr 2021 08:55:34 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, kbuild-all@lists.01.org, kbuild test robot , Thiago Jung Bauermann , linuxppc-dev , Dan Carpenter , Daniel Axtens Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On 4/20/21 8:47 AM, Rob Herring wrote: > On Tue, Apr 20, 2021 at 10:04 AM Lakshmi Ramasubramanian > wrote: >> >> On 4/20/21 7:42 AM, Lakshmi Ramasubramanian wrote: >>> On 4/20/21 6:06 AM, Rob Herring wrote: >>>> On Tue, Apr 20, 2021 at 12:20 AM Lakshmi Ramasubramanian >>>> wrote: >>>>> >>>>> On 4/19/21 10:00 PM, Dan Carpenter wrote: >>>>>> On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote: >>>>>>> Lakshmi Ramasubramanian writes: >>>>>>>> On 4/16/21 2:05 AM, Michael Ellerman wrote: >>>>>>>> >>>>>>>>> Daniel Axtens 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; >>>>>> >>>>>> This will leak. It would need to be something like: >>>>>> >>>>>> if (ret) { >>>>>> pr_err("Error setting up the purgatory.\n"); >>>>>> goto out_free_fdt; >>>>>> } >>>>> Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot >>>>> be freed here - it will be freed when the kexec cleanup function is >>>>> called. >>>> >>>> That may be the case currently, but really if a function returns an >>>> error it should have undone anything it did like memory allocations. I >>>> don't think you should do that to fix this issue, but it would be a >>>> good clean-up. >>>> >>> >>> I agree - in case of an error the function should do a proper clean-up. >>> Just to be clear - for now, I will leave this as is. Correct? > > Yes. okay. > >>> In my patch, I will do the following changes: >>> >>> => Free "fdt" when possible (as Michael had suggested in his patch) >>> => Zero out "elf_info" struct at the start of the function. >>> >> >> Instead of zeroing out "elf_info", I think it would be better to return >> an error immediately, instead of the "goto out;", if >> kexec_build_elf_info() fails. >> >> ret = kexec_build_elf_info(kernel_buf, kernel_len, &ehdr, &elf_info); >> if (ret) >> return ERR_PTR(ret); > > I thought kexec_build_elf_info() can return an error and allocated > memory, so that would leak memory. > I looked at kexec_build_elf_info() more - it does free elf_info, if it allocated it but encountered an error after the allocation. So it does a proper clean-up in case of an error. -lakshmi