Re: [PATCH] powerpc/pseries: Fix UAF reference for src_info after list_add

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Haren Myneni <haren@linux.ibm.com> writes:

> Getting the following kernel panic in papr_hvpipe_dev_create_handle()
> when trying to add src_info to the list.
>  Kernel attempted to write user page (0) - exploit attempt? (uid: 0)
>  BUG: Kernel NULL pointer dereference on write at 0x00000000
>  Faulting instruction address: 0xc0000000001b44a0
>  Oops: Kernel access of bad area, sig: 11 [#1]
>  ...
>  Call Trace:
>  papr_hvpipe_dev_ioctl+0x1f4/0x48c (unreliable)
>  sys_ioctl+0x528/0x1064
>  system_call_exception+0x128/0x360
>  system_call_vectored_common+0x15c/0x2ec
>
> The current code adds src_info to the list after UAF for src_info.
> So move the retain_and_null_ptr(src_info) after this list add.
>

Sorry for the delay in getting back on this.

> Fixes: 6d3789d347a7 ("papr-hvpipe: convert papr_hvpipe_dev_create_handle() to FD_PREPARE()")
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/papr-hvpipe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr-hvpipe.c b/arch/powerpc/platforms/pseries/papr-hvpipe.c
> index 14ae480d060a..5121c87d1fad 100644
> --- a/arch/powerpc/platforms/pseries/papr-hvpipe.c
> +++ b/arch/powerpc/platforms/pseries/papr-hvpipe.c
> @@ -509,7 +509,6 @@ static int papr_hvpipe_dev_create_handle(u32 srcID)
>  	if (fdf.err)
>  		return fdf.err;
>  
> -	retain_and_null_ptr(src_info);
>  	spin_lock(&hvpipe_src_list_lock);
>  	/*
>  	 * If two processes are executing ioctl() for the same
> @@ -522,6 +521,7 @@ static int papr_hvpipe_dev_create_handle(u32 srcID)
>  	}
>  	list_add(&src_info->list, &hvpipe_src_list);
>  	spin_unlock(&hvpipe_src_list_lock);
> +	retain_and_null_ptr(src_info);
>  	return fd_publish(fdf);
>  }

Looking at the destructor routine...

static inline void class_fd_prepare_destructor(const struct fd_prepare *fdf)
{
	if (unlikely(fdf->__fd >= 0))
		put_unused_fd(fdf->__fd);
	if (unlikely(!IS_ERR_OR_NULL(fdf->__file)))
		fput(fdf->__file);
}


...I think this approach might still have issues. i.e. if we don't make
src_info as null like how it was done before, then when we return an
error from here, it can cause double free issue:

	if (hvpipe_find_source(srcID)) {
		spin_unlock(&hvpipe_src_list_lock);
		return -EALREADY;
	}

Because, an auto cleanup / kfree(src_info) will get called.  And since
the FD_PREPARE() step was done successfully, it will also call the
file->f_op()->release() function on fput(). Now since we have not even
added the src_info into the global list so this can cause list
corruption as well.

static int papr_hvpipe_handle_release(struct inode *inode,
				struct file *file)
...
	src_info = file->private_data;
	list_del(&src_info->list);
...

So looks like, this approach has list corruption + double free issue.
Maybe you can even try to reproduce this when you call the IOCTL twice
with the same srcID.


@Haren,
While looking at the papr-hvpipe code, I think I may have found couple
of more issues. So, I am preparing a set of fixes for the same. I will
need your help in the review and testing of those please, as I am not
much familiar with this code :)

-ritesh