[v2,1/4] powerpc/kvm/book3s: Fixes possible 'use after release' of kvm
diff mbox series

Message ID 20191107170258.36379-2-leonardo@linux.ibm.com
State Rejected
Headers show
Series
  • Replace current->mm by kvm->mm on powerpc/kvm
Related show

Commit Message

Leonardo Bras Nov. 7, 2019, 5:02 p.m. UTC
Fixes a possible 'use after free' of kvm variable in
kvm_vm_ioctl_create_spapr_tce, where it does a mutex_unlock(&kvm->lock)
after a kvm_put_kvm(kvm).

Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
---
 arch/powerpc/kvm/book3s_64_vio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Michael Ellerman Nov. 12, 2019, 4:57 a.m. UTC | #1
Hi Leonardo,

Leonardo Bras <leonardo@linux.ibm.com> writes:
> Fixes a possible 'use after free' of kvm variable in
> kvm_vm_ioctl_create_spapr_tce, where it does a mutex_unlock(&kvm->lock)
> after a kvm_put_kvm(kvm).

There is no potential for an actual use after free here AFAICS.

> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
> index 5834db0a54c6..a402ead833b6 100644
> --- a/arch/powerpc/kvm/book3s_64_vio.c
> +++ b/arch/powerpc/kvm/book3s_64_vio.c

The preceeding context is:

	mutex_lock(&kvm->lock);

	/* Check this LIOBN hasn't been previously allocated */
	ret = 0;
	list_for_each_entry(siter, &kvm->arch.spapr_tce_tables, list) {
		if (siter->liobn == args->liobn) {
			ret = -EBUSY;
			break;
		}
	}

	kvm_get_kvm(kvm);
	if (!ret)
		ret = anon_inode_getfd("kvm-spapr-tce", &kvm_spapr_tce_fops,
				       stt, O_RDWR | O_CLOEXEC);

> @@ -316,14 +316,13 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
>  
>  	if (ret >= 0)
>  		list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);
> -	else
> -		kvm_put_kvm(kvm);
>  
>  	mutex_unlock(&kvm->lock);
>  
>  	if (ret >= 0)
>  		return ret;
>  
> +	kvm_put_kvm(kvm);
>  	kfree(stt);
>   fail_acct:
>  	account_locked_vm(current->mm, kvmppc_stt_pages(npages), false);


If the kvm_put_kvm() you've moved actually caused the last reference to
be dropped that would mean that our caller had passed us a kvm struct
without holding a reference to it, and that would be a bug in our
caller.

Or put another way, it would mean the mutex_lock() above could already
be operating on a freed kvm struct.

The kvm_get_kvm() prior to the anon_inode_getfd() is to account for the
reference that's held by the `stt` struct, and dropped in
kvm_spapr_tce_release().

So although this patch isn't wrong, the explanation is not accurate.

cheers
Leonardo Bras Nov. 14, 2019, 6:43 p.m. UTC | #2
On Tue, 2019-11-12 at 15:57 +1100, Michael Ellerman wrote:
> Hi Leonardo,

Hello Micheal, thanks for the feedback!

> 
> Leonardo Bras <leonardo@linux.ibm.com> writes:
> > Fixes a possible 'use after free' of kvm variable in
> > kvm_vm_ioctl_create_spapr_tce, where it does a mutex_unlock(&kvm-
> > >lock)
> > after a kvm_put_kvm(kvm).
> 
> There is no potential for an actual use after free here AFAICS.
> 
> > diff --git a/arch/powerpc/kvm/book3s_64_vio.c
> > b/arch/powerpc/kvm/book3s_64_vio.c
> > index 5834db0a54c6..a402ead833b6 100644
> > --- a/arch/powerpc/kvm/book3s_64_vio.c
> > +++ b/arch/powerpc/kvm/book3s_64_vio.c
> 
> The preceeding context is:
> 
> 	mutex_lock(&kvm->lock);
> 
> 	/* Check this LIOBN hasn't been previously allocated */
> 	ret = 0;
> 	list_for_each_entry(siter, &kvm->arch.spapr_tce_tables, list) {
> 		if (siter->liobn == args->liobn) {
> 			ret = -EBUSY;
> 			break;
> 		}
> 	}
> 
> 	kvm_get_kvm(kvm);
> 	if (!ret)
> 		ret = anon_inode_getfd("kvm-spapr-tce",
> &kvm_spapr_tce_fops,
> 				       stt, O_RDWR | O_CLOEXEC);
> 
> > @@ -316,14 +316,13 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm
> > *kvm,
> >  
> >  	if (ret >= 0)
> >  		list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);
> > -	else
> > -		kvm_put_kvm(kvm);
> >  
> >  	mutex_unlock(&kvm->lock);
> >  
> >  	if (ret >= 0)
> >  		return ret;
> >  
> > +	kvm_put_kvm(kvm);
> >  	kfree(stt);
> >   fail_acct:
> >  	account_locked_vm(current->mm, kvmppc_stt_pages(npages),
> > false);
> 
> If the kvm_put_kvm() you've moved actually caused the last reference
> to
> be dropped that would mean that our caller had passed us a kvm struct
> without holding a reference to it, and that would be a bug in our
> caller.
> 

So, there is no chance that between this function's kvm_get_kvm() and 
kvm_put_kvm(), another thread can decrease this reference counter?

> Or put another way, it would mean the mutex_lock() above could
> already
> be operating on a freed kvm struct.
> 
> The kvm_get_kvm() prior to the anon_inode_getfd() is to account for
> the
> reference that's held by the `stt` struct, and dropped in
> kvm_spapr_tce_release().
> 
> So although this patch isn't wrong, the explanation is not accurate.
> 
> cheers

Kind regards
Leonardo Bras Nov. 21, 2019, 1:24 p.m. UTC | #3
On Thu, 2019-11-14 at 15:43 -0300, Leonardo Bras wrote:
> > If the kvm_put_kvm() you've moved actually caused the last
> > reference
> > to
> > be dropped that would mean that our caller had passed us a kvm
> > struct
> > without holding a reference to it, and that would be a bug in our
> > caller.
> > 
> 
> So, there is no chance that between this function's kvm_get_kvm()
> and 
> kvm_put_kvm(), another thread can decrease this reference counter?

I am probably missing something here, could you please help me
understand that?

> > Or put another way, it would mean the mutex_lock() above could
> > already
> > be operating on a freed kvm struct.
> > 
> > The kvm_get_kvm() prior to the anon_inode_getfd() is to account for
> > the
> > reference that's held by the `stt` struct, and dropped in
> > kvm_spapr_tce_release().
> > 
> > So although this patch isn't wrong, the explanation is not
> > accurate.
> > 
> > cheers
> 
> Kind regards

Best regards,

Patch
diff mbox series

diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
index 5834db0a54c6..a402ead833b6 100644
--- a/arch/powerpc/kvm/book3s_64_vio.c
+++ b/arch/powerpc/kvm/book3s_64_vio.c
@@ -316,14 +316,13 @@  long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
 
 	if (ret >= 0)
 		list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);
-	else
-		kvm_put_kvm(kvm);
 
 	mutex_unlock(&kvm->lock);
 
 	if (ret >= 0)
 		return ret;
 
+	kvm_put_kvm(kvm);
 	kfree(stt);
  fail_acct:
 	account_locked_vm(current->mm, kvmppc_stt_pages(npages), false);