Re: [PATCH pstore fix v1] pstore: fix use after free

[Kees Cook <keescook@chromium.org>]

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