Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 8 Jan 2020 10:01:04 -0800
From:   Kees Cook <keescook@chromium.org>
To:     Cengiz Can <cengiz@kernel.wtf>
Cc:     linux-kernel@vger.kernel.org, Anton Vorontsov <anton@enomsg.org>,
        Colin Cross <ccross@android.com>,
        Tony Luck <tony.luck@intel.com>
Subject: Re: [PATCH] fs: pstore: fix double-free on ramoops_init_przs
Message-ID: <202001081000.88F20E5@keescook>
References: <20200107110445.162404-1-cengiz@kernel.wtf>
 <202001071002.A236EBCA0@keescook>
 <d4ec59002ede4aaf9928c7f7526da87c@kernel.wtf>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d4ec59002ede4aaf9928c7f7526da87c@kernel.wtf>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jan 07, 2020 at 10:40:58PM +0300, Cengiz Can wrote:
> Hello Kees!
> 
> It's a pleasure to hear from you!
> 
> On 2020-01-07 21:05, Kees Cook wrote:
> > 
> > I think this is a false positive (have you actually hit the
> > double-free?). The logic in this area is:
> 
> No I did not actually hit the double-free. I'm just following
> the indicators from static analyzer.
> 
> > nothing was freeing the label on the failed prz, but all the other prz
> > labels were free (i.e. there is a "i--" that skips the failed prz
> > alloc).
> 
> I have noticed that. Thanks for clearing it up though.
> 
> The `kfree` I was referring to is in `err:` label of function
> `persistent_ram_new` in `ram_core.c#595` of `for-next/pstore` tree:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/tree/fs/pstore/ram_core.c?h=for-next/pstore#n595
> 
> Here are the relevant bits:
> 
> ```
> struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t
> size,
> 			u32 sig, struct persistent_ram_ecc_info *ecc_info,
> 			unsigned int memtype, u32 flags, char *label)
> {
> 	/* ... */
> 	/* ... */
> 	/* ... */
> 	return prz;
> err:
> 	persistent_ram_free(prz); /* <----- */
> 	return ERR_PTR(ret);
> }
> ```
> 
> So, to my understanding, if our `persistent_ram_new` call in `ram.c#583`
> fails, it already does clean up the `label` pointer in itself and returns
> an ERR_PTR back to us and our skipping logic does its job.
> 
> I might be missing something but it seems so.
> 
> Thank you for looking into this.

Ah-ha! Yes, I see it now. We have multiple paths to the err: label, and
I was focused on the kzalloc() failure path. I will get this fixed
better. Thanks!

-- 
Kees Cook