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=-16.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS autolearn=ham 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 0B9EEC433B4 for ; Tue, 20 Apr 2021 15:47:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BCD09613CA for ; Tue, 20 Apr 2021 15:47:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232303AbhDTPsW (ORCPT ); Tue, 20 Apr 2021 11:48:22 -0400 Received: from mail.kernel.org ([198.145.29.99]:44914 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232764AbhDTPsV (ORCPT ); Tue, 20 Apr 2021 11:48:21 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 4F03B613CA for ; Tue, 20 Apr 2021 15:47:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1618933670; bh=IYqfg7kAXJ4Rm7YLHyAad4JAlWepcjRKNyoYDCNpags=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=ptxt5xT8VW0ErKPoXdAVz4uqQsNYogTTn+TJLYIkfIFZ8sLFBH0UPmskqSpeamgIj +rovtDASFIasau/8YVVn+qZVdHxQFUIilllmVPzZdPSdhDrg5YB15l4ZPwxVcrf4EU HitVE8i4HMwfrySdYcRa4Zf50vbewnYaKK3EL5Rxfpd/aljhVGjwBqtVJBph/P1Fzq dKFR/THcGHEcLFiyAhAkZEOPp3dl5eENzC/xSTthB7cR+J500w7noP9fw/OtIzWczd km+/eniHy+ITuQkZNkmDqDPAAwpxezFZK64rMB5pepart+EWzhrye+qaEFneeqCyUB OAH4CvG0qFcaA== Received: by mail-ed1-f45.google.com with SMTP id s15so45691457edd.4 for ; Tue, 20 Apr 2021 08:47:50 -0700 (PDT) X-Gm-Message-State: AOAM5328wsRmTeX3b5jH9DihzCMQ5y2twuAuyJKd7TGWRClQpXlG6rsb 3CDBvDfH7+3EdNlTfkZyOwuZS1o2Jc3z+DHnVg== X-Google-Smtp-Source: ABdhPJzwUCwDAs9HaD9luzICTw9yTZ3mv8AFbQQ4N9TOgAbafBYxLlIpYgWL71kwt1BKCFzGNMDumpaHgoIXGlEJ/T8= X-Received: by 2002:a05:6402:51c6:: with SMTP id r6mr14587868edd.258.1618933668588; Tue, 20 Apr 2021 08:47:48 -0700 (PDT) MIME-Version: 1.0 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> In-Reply-To: <433b4518-0a83-5a97-ac07-da8748dcc90e@linux.microsoft.com> From: Rob Herring Date: Tue, 20 Apr 2021 10:47:36 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load() To: Lakshmi Ramasubramanian Cc: Dan Carpenter , Michael Ellerman , Daniel Axtens , devicetree@vger.kernel.org, linuxppc-dev , kbuild-all@lists.01.org, Thiago Jung Bauermann , kbuild test robot Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org 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. > > 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. Rob 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=-13.8 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SPF_HELO_NONE,SPF_PASS 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 6BF98C43470 for ; Tue, 20 Apr 2021 15:48:23 +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 7D800613B0 for ; Tue, 20 Apr 2021 15:48:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7D800613B0 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org 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 4FPp4N699yz301N for ; Wed, 21 Apr 2021 01:48:20 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20201202 header.b=ptxt5xT8; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=kernel.org (client-ip=198.145.29.99; helo=mail.kernel.org; envelope-from=robh@kernel.org; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20201202 header.b=ptxt5xT8; dkim-atps=neutral Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4FPp3t1WW6z2xZn for ; Wed, 21 Apr 2021 01:47:54 +1000 (AEST) Received: by mail.kernel.org (Postfix) with ESMTPSA id 35A1A6115C for ; Tue, 20 Apr 2021 15:47:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1618933670; bh=IYqfg7kAXJ4Rm7YLHyAad4JAlWepcjRKNyoYDCNpags=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=ptxt5xT8VW0ErKPoXdAVz4uqQsNYogTTn+TJLYIkfIFZ8sLFBH0UPmskqSpeamgIj +rovtDASFIasau/8YVVn+qZVdHxQFUIilllmVPzZdPSdhDrg5YB15l4ZPwxVcrf4EU HitVE8i4HMwfrySdYcRa4Zf50vbewnYaKK3EL5Rxfpd/aljhVGjwBqtVJBph/P1Fzq dKFR/THcGHEcLFiyAhAkZEOPp3dl5eENzC/xSTthB7cR+J500w7noP9fw/OtIzWczd km+/eniHy+ITuQkZNkmDqDPAAwpxezFZK64rMB5pepart+EWzhrye+qaEFneeqCyUB OAH4CvG0qFcaA== Received: by mail-ed1-f52.google.com with SMTP id y3so9382758eds.5 for ; Tue, 20 Apr 2021 08:47:50 -0700 (PDT) X-Gm-Message-State: AOAM532q76117KJ+4d/jRfXRlvxbw2qR+Lsr9CFeK1LE+yfMEan9Z/YQ zl+e8qv+jXA4FZyKiXHtfh8k0tUAxYyYeKlzjw== X-Google-Smtp-Source: ABdhPJzwUCwDAs9HaD9luzICTw9yTZ3mv8AFbQQ4N9TOgAbafBYxLlIpYgWL71kwt1BKCFzGNMDumpaHgoIXGlEJ/T8= X-Received: by 2002:a05:6402:51c6:: with SMTP id r6mr14587868edd.258.1618933668588; Tue, 20 Apr 2021 08:47:48 -0700 (PDT) MIME-Version: 1.0 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> In-Reply-To: <433b4518-0a83-5a97-ac07-da8748dcc90e@linux.microsoft.com> From: Rob Herring Date: Tue, 20 Apr 2021 10:47:36 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load() To: Lakshmi Ramasubramanian Content-Type: text/plain; charset="UTF-8" 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 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. > > 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. Rob From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============2941137039172295747==" MIME-Version: 1.0 From: Rob Herring To: kbuild-all@lists.01.org Subject: Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load() Date: Tue, 20 Apr 2021 10:47:36 -0500 Message-ID: In-Reply-To: <433b4518-0a83-5a97-ac07-da8748dcc90e@linux.microsoft.com> List-Id: --===============2941137039172295747== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable 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/tre= e/arch/powerpc/kexec/elf_64.c?h=3Dfor-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_6= 4.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 =3D 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 =3D KEXEC_BUF_MEM_UNKNOWN; > >>>>> ret =3D 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 =3D 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 can= not > >>> 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. > > In my patch, I will do the following changes: > > > > =3D> Free "fdt" when possible (as Michael had suggested in his patch) > > =3D> 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 =3D 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. Rob --===============2941137039172295747==--