kvm/vfio: ensure kvg instance stays around in kvm_vfio_group_add()

  kvm_vfio_group_add() creates kvg instance, links it to kv->group_list,
and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after
dropping kv->lock. If we race group addition and deletion calls, kvg
instance may get freed by the time we get around to calling
kvm_vfio_file_set_kvm().

Fix this by moving call to kvm_vfio_file_set_kvm() under the protection
of kv->lock. We already call it while holding the same lock when vfio
group is being deleted, so it should be safe here as well.

Fixes: ba70a89f3c2a ("vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 virt/kvm/vfio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On Mon, 10 Jul 2023 15:20:31 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> kvm_vfio_group_add() creates kvg instance, links it to kv->group_list,
> and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after
> dropping kv->lock. If we race group addition and deletion calls, kvg
> instance may get freed by the time we get around to calling
> kvm_vfio_file_set_kvm().
> 
> Fix this by moving call to kvm_vfio_file_set_kvm() under the protection
> of kv->lock. We already call it while holding the same lock when vfio
> group is being deleted, so it should be safe here as well.
> 
> Fixes: ba70a89f3c2a ("vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  virt/kvm/vfio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 9584eb57e0ed..cd46d7ef98d6 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -179,10 +179,10 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
>  	list_add_tail(&kvg->node, &kv->group_list);
>  
>  	kvm_arch_start_assignment(dev->kvm);
> +	kvm_vfio_file_set_kvm(kvg->file, dev->kvm);
>  
>  	mutex_unlock(&kv->lock);
>  
> -	kvm_vfio_file_set_kvm(kvg->file, dev->kvm);
>  	kvm_vfio_update_coherency(dev);
>  
>  	return 0;


I'm not sure this hasn't been an issue since it was originally
introduced in 2fc1bec15883 ("kvm: set/clear kvm to/from vfio_group when
group add/delete").

The change added by the blamed ba70a89f3c2a in this respect is simply
that we get the file pointer from the mutex protected object, but that
mutex protected object is also what maintains that the file pointer is
valid.  The vfio_group implementation suffered the same issue, the
delete path could put the group reference, which could theoretically
cause a use after free of the vfio_group.

We could effectively restore the pre-ba70a89f3c2a behavior by replacing
kvg->file with filp here, but that would still leave us vulnerable to
the original issue.

Note also that kvm_vfio_update_coherency() takes the same mutex
separately, I wonder if it wouldn't make more sense if it were moved
under the caller's mutex to avoid bouncing the lock and unnecessarily
taking it in the release path.  Thanks,

Alex
  

Hi Alex,

On Thu, Jul 13, 2023 at 12:48:11PM -0600, Alex Williamson wrote:
> On Mon, 10 Jul 2023 15:20:31 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > kvm_vfio_group_add() creates kvg instance, links it to kv->group_list,
> > and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after
> > dropping kv->lock. If we race group addition and deletion calls, kvg
> > instance may get freed by the time we get around to calling
> > kvm_vfio_file_set_kvm().
> > 
> > Fix this by moving call to kvm_vfio_file_set_kvm() under the protection
> > of kv->lock. We already call it while holding the same lock when vfio
> > group is being deleted, so it should be safe here as well.
> > 
> > Fixes: ba70a89f3c2a ("vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  virt/kvm/vfio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> > index 9584eb57e0ed..cd46d7ef98d6 100644
> > --- a/virt/kvm/vfio.c
> > +++ b/virt/kvm/vfio.c
> > @@ -179,10 +179,10 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
> >  	list_add_tail(&kvg->node, &kv->group_list);
> >  
> >  	kvm_arch_start_assignment(dev->kvm);
> > +	kvm_vfio_file_set_kvm(kvg->file, dev->kvm);
> >  
> >  	mutex_unlock(&kv->lock);
> >  
> > -	kvm_vfio_file_set_kvm(kvg->file, dev->kvm);
> >  	kvm_vfio_update_coherency(dev);
> >  
> >  	return 0;
> 
> 
> I'm not sure this hasn't been an issue since it was originally
> introduced in 2fc1bec15883 ("kvm: set/clear kvm to/from vfio_group when
> group add/delete").
> 
> The change added by the blamed ba70a89f3c2a in this respect is simply
> that we get the file pointer from the mutex protected object, but that
> mutex protected object is also what maintains that the file pointer is
> valid.  The vfio_group implementation suffered the same issue, the
> delete path could put the group reference, which could theoretically
> cause a use after free of the vfio_group.

Yes, you are right, I'll update the patch with the correct "Fixes".

> 
> We could effectively restore the pre-ba70a89f3c2a behavior by replacing
> kvg->file with filp here, but that would still leave us vulnerable to
> the original issue.
> 
> Note also that kvm_vfio_update_coherency() takes the same mutex
> separately, I wonder if it wouldn't make more sense if it were moved
> under the caller's mutex to avoid bouncing the lock and unnecessarily
> taking it in the release path.  Thanks,

I think I will make it a separate patch.

Thanks.
  

On Mon, Jul 10, 2023 at 03:20:31PM -0700, Dmitry Torokhov wrote:
> kvm_vfio_group_add() creates kvg instance, links it to kv->group_list,
> and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after
> dropping kv->lock. If we race group addition and deletion calls, kvg
> instance may get freed by the time we get around to calling
> kvm_vfio_file_set_kvm().
> 
> Fix this by moving call to kvm_vfio_file_set_kvm() under the protection
> of kv->lock. We already call it while holding the same lock when vfio
> group is being deleted, so it should be safe here as well.
> 
> Fixes: ba70a89f3c2a ("vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  virt/kvm/vfio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This looks correct, I don't know of any lock cylces that could form
with kv->lock at least

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason
  

On Mon, Jul 10, 2023 at 03:20:31PM -0700, Dmitry Torokhov wrote:
> kvm_vfio_group_add() creates kvg instance, links it to kv->group_list,
> and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after
> dropping kv->lock. If we race group addition and deletion calls, kvg
> instance may get freed by the time we get around to calling
> kvm_vfio_file_set_kvm().
> 
> Fix this by moving call to kvm_vfio_file_set_kvm() under the protection
> of kv->lock. We already call it while holding the same lock when vfio
> group is being deleted, so it should be safe here as well.
> 
> Fixes: ba70a89f3c2a ("vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  virt/kvm/vfio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 9584eb57e0ed..cd46d7ef98d6 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -179,10 +179,10 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
>  	list_add_tail(&kvg->node, &kv->group_list);
>  
>  	kvm_arch_start_assignment(dev->kvm);
> +	kvm_vfio_file_set_kvm(kvg->file, dev->kvm);
>  
>  	mutex_unlock(&kv->lock);
>  
> -	kvm_vfio_file_set_kvm(kvg->file, dev->kvm);
>  	kvm_vfio_update_coherency(dev);
>  
>  	return 0;
> -- 
> 2.41.0.255.g8b1d071c50-goog

What ever happened to this change?  Did it end up in a KVM tree
somewhere?

thanks,

greg k-h
  

On Mon, Jul 31, 2023 at 02:02:59PM +0200, Greg KH wrote:
> On Mon, Jul 10, 2023 at 03:20:31PM -0700, Dmitry Torokhov wrote:
> > kvm_vfio_group_add() creates kvg instance, links it to kv->group_list,
> > and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after
> > dropping kv->lock. If we race group addition and deletion calls, kvg
> > instance may get freed by the time we get around to calling
> > kvm_vfio_file_set_kvm().
> > 
> > Fix this by moving call to kvm_vfio_file_set_kvm() under the protection
> > of kv->lock. We already call it while holding the same lock when vfio
> > group is being deleted, so it should be safe here as well.
> > 
> > Fixes: ba70a89f3c2a ("vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  virt/kvm/vfio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> > index 9584eb57e0ed..cd46d7ef98d6 100644
> > --- a/virt/kvm/vfio.c
> > +++ b/virt/kvm/vfio.c
> > @@ -179,10 +179,10 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd)
> >  	list_add_tail(&kvg->node, &kv->group_list);
> >  
> >  	kvm_arch_start_assignment(dev->kvm);
> > +	kvm_vfio_file_set_kvm(kvg->file, dev->kvm);
> >  
> >  	mutex_unlock(&kv->lock);
> >  
> > -	kvm_vfio_file_set_kvm(kvg->file, dev->kvm);
> >  	kvm_vfio_update_coherency(dev);
> >  
> >  	return 0;
> > -- 
> > 2.41.0.255.g8b1d071c50-goog
> 
> What ever happened to this change?  Did it end up in a KVM tree
> somewhere?

It was posted as:

https://lore.kernel.org/all/20230714224538.404793-1-dmitry.torokhov@gmail.com/

and I believe Alex Williamson is planning to take it through VFIO tree.

Thanks.