linux-kernel - Re: [RFC PATCH] iommufd: Destroy vdevice on device unbind

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250617183452.GG1376515@ziepe.ca>
Date: Tue, 17 Jun 2025 15:34:52 -0300
From: Jason Gunthorpe <jgg@...pe.ca>
To: "Aneesh Kumar K.V" <aneesh.kumar@...nel.org>
Cc: "Tian, Kevin" <kevin.tian@...el.com>,
	"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
	Robin Murphy <robin.murphy@....com>
Subject: Re: [RFC PATCH] iommufd: Destroy vdevice on device unbind

On Tue, Jun 17, 2025 at 01:37:04PM +0530, Aneesh Kumar K.V wrote:

> How do we reclaim that object id for further reuse? 

Maybe just don't? Userspace did something it shouldn't, it now leaked
8 bytes of kernel memory until the FD is closed.

> is it that if there is a request for a iommufd_object_remove() with object
> refcount > 1, we insert a XA_ZERO_ENTRY and convert that to NULL entry
> on IOMMU_DESTROY?

Oh no we can't do that, if the refcount is elevated that is a problem,
it means some thread somewhere is using that memory.

We can sleep and wait for shortterm_users to go to zero and if users
is still elevated then we are toast. WARN_ON and reatin it in the
xarray and hope for the best.

So the thread that will trigger the detruction needs to have a users
refcount of 1. Meaning users needs to be one while idle in the xarray,
and the idevice destruction will obtain a users=2 from its pointer
under some kind of lock.

> -enum {
> -	REMOVE_WAIT_SHORTTERM = 1,
> -};
> +#define	REMOVE_WAIT_SHORTTERM	BIT(0)
> +#define	REMOVE_OBJ_FORCE	BIT(1)

You can keep the enum for flags, but 'force' isn't the right name. I
would think it is 'tombstone'

> diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
> index b7aa725e6b37..d27b61787a53 100644
> --- a/drivers/iommu/iommufd/main.c
> +++ b/drivers/iommu/iommufd/main.c
> @@ -88,7 +88,8 @@ struct iommufd_object *iommufd_get_object(struct iommufd_ctx *ictx, u32 id,
>  
>  	xa_lock(&ictx->objects);
>  	obj = xa_load(&ictx->objects, id);
> -	if (!obj || (type != IOMMUFD_OBJ_ANY && obj->type != type) ||
> +	if (!obj || xa_is_zero(obj) ||
> +	    (type != IOMMUFD_OBJ_ANY && obj->type != type) ||
>  	    !iommufd_lock_obj(obj))

xa_load can't return xa_is_zero(), xas_load() can

We already use XA_ZERO_ENTRY to hold an ID during allocation till
finalize.

I think you want to add a new API

iommufd_object_tombstone_user(idev->ictx, &idev->vdev->obj);

Which I think is the same as the existing
iommufd_object_destroy_user() except it uses tombstone..

The only thing tombstone does is:

	xas_store(&xas, (flags & REMOVE_OBJ_TOMBSTONE) ? XA_ZERO_ENTRY : NULL);

All the rest of the logic including the users and shorterm check would
be the same.

> --- a/drivers/iommu/iommufd/viommu.c
> +++ b/drivers/iommu/iommufd/viommu.c
> @@ -213,6 +213,8 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
>  	/* vdev lifecycle now managed by idev */
>  	idev->vdev = vdev;
>  	refcount_inc(&vdev->obj.users);
> +	/* Increment refcount since userspace can hold the obj id */
> +	refcount_inc(&vdev->obj.users);
>  	goto out_put_idev_unlock;

I don't think this should change.. There should be no extra user refs
or userspace can't destroy it.

The pointer back from the idevice needs locking to protect it while a
refcount is obtained.

Jason