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:   Sun, 7 Jul 2019 07:41:09 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Norbert Manthey <nmanthey@amazon.de>
Cc:     linux-kernel@vger.kernel.org, David Woodhouse <dwmw@amazon.co.uk>,
        Anton Vorontsov <anton@enomsg.org>,
        Colin Cross <ccross@android.com>,
        Tony Luck <tony.luck@intel.com>
Subject: Re: [PATCH pstore fix v1] pstore: fix use after free
Message-ID: <201907070734.86DE450@keescook>
References: <1562331960-26198-1-git-send-email-nmanthey@amazon.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1562331960-26198-1-git-send-email-nmanthey@amazon.de>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jul 05, 2019 at 03:06:00PM +0200, Norbert Manthey wrote:
> The pstore_mkfile() function is passed a pointer to a struct
> pstore_record. On success it consumes this 'record' pointer and
> references it from the created inode.
> 
> On failure, however, it may or may not free the record. There are even
> two different code paths which return -ENOMEM -- one of which does and
> the other doesn't free the record.
> 
> Make the behaviour deterministic by never consuming and freeing the
> record when returning failure, allowing the caller to do the cleanup
> consistently.

Yup, good catch. Looks like a double-free in the one failure case.

> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>

Fixes: 83f70f0769ddd ("pstore: Do not duplicate record metadata")
Fixes: 1dfff7dd67d1a ("pstore: Pass record contents instead of copying")
Cc: stable@vger.kernel.org

Applied to my pstore tree. :)

-Kees

> ---
>  fs/pstore/inode.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -333,7 +333,6 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
>  	private = kzalloc(sizeof(*private), GFP_KERNEL);
>  	if (!private)
>  		goto fail_alloc;
> -	private->record = record;
>  
>  	switch (record->type) {
>  	case PSTORE_TYPE_DMESG:
> @@ -387,6 +386,8 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record)
>  	if (!dentry)
>  		goto fail_private;
>  
> +	private->record = record;
> +
>  	inode->i_size = private->total_size = size;
>  
>  	inode->i_private = private;
> -- 
> 2.7.4
> 
> 
> 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
> 
> 
> 

-- 
Kees Cook