| 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
| ||
|
Message-ID: <yq5azfe6ssev.fsf@kernel.org>
Date: Tue, 17 Jun 2025 13:37:04 +0530
From: Aneesh Kumar K.V <aneesh.kumar@...nel.org>
To: Jason Gunthorpe <jgg@...pe.ca>, "Tian, Kevin" <kevin.tian@...el.com>
Cc: "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
Jason Gunthorpe <jgg@...pe.ca> writes:
> On Mon, Jun 16, 2025 at 05:37:58AM +0000, Tian, Kevin wrote:
>
>> the expected destruction flow from userspace is to IOMMU_DESTROY
>> the vdevice object before closing the vfio cdev fd which unbinds the
>> idevice.
>>
>> now we are discussing how to handle a malfunction userspace which
>> violates that flow: let it be or add a tomestone state, after extending
>> unbind to destroy the vdevice...
>
> Right, to be clear the concern is
>
> close(vfio_cdev)
> ioctl(DESTROY, vdevice_id);
> close(iommufd)
>
> Which is a possibile sequence for userspace/syzkaller to trigger.
>
> My position has historically been that DESTROY should not destroy some
> random unrelated object eg because a parallel thread did an allocation
> and re-used the kernel deleted ID. ID's that belong to userspace have
> to be retained right up until DESTROY.
>
> Thus we've historically avoided creating scenarios where IDs owned by
> userspace are destroyed by the kernel.
>
> Given we can say the above is illegal use of the API we could leave
> behind a tombstone in the xarray. The goal would be to prevent lookup
> of the object (since it is destroyed) and prevent reallocation of the
> ID.
>
> For instance a simple thing would be to drop in XA_ZERO_ENTRY, this
> will reserve the ID and fail all future operations. The userspace will
> get a failure on DESTROY so they know they did something wrong. The fd
> close will clean up the reserved ID.
>
How do we reclaim that object id for further reuse?
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?
Something like below
The below sequence will work with the changes as
close(vfio_cdev) -> vdev destroy
ioctl(DESTROY, vdevice_id); -> vdev object id reclaim
close(iommufd)
ioctl(DESTROY, vdevice_id); -> EBUSY
close(vfio_cdev) -> vdev destroy
close(iommufd)
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 7b3f82afd295..1f8e4fd0e240 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -289,7 +289,7 @@ void iommufd_device_unbind(struct iommufd_device *idev)
if (idev->vdev)
/* extra refcount taken during vdevice alloc */
- iommufd_object_destroy_user(idev->ictx, &idev->vdev->obj);
+ __iommufd_object_destroy_user(idev->ictx, &idev->vdev->obj, REMOVE_OBJ_FORCE);
iommufd_object_destroy_user(idev->ictx, &idev->obj);
}
EXPORT_SYMBOL_NS_GPL(iommufd_device_unbind, "IOMMUFD");
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 9c4472df80c6..8c5fc0fe92ce 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -186,9 +186,9 @@ void iommufd_object_abort_and_destroy(struct iommufd_ctx *ictx,
void iommufd_object_finalize(struct iommufd_ctx *ictx,
struct iommufd_object *obj);
-enum {
- REMOVE_WAIT_SHORTTERM = 1,
-};
+#define REMOVE_WAIT_SHORTTERM BIT(0)
+#define REMOVE_OBJ_FORCE BIT(1)
+
int iommufd_object_remove(struct iommufd_ctx *ictx,
struct iommufd_object *to_destroy, u32 id,
unsigned int flags);
@@ -198,12 +198,13 @@ int iommufd_object_remove(struct iommufd_ctx *ictx,
* point the caller has no shortterm_users reference and at least the xarray
* will be holding one.
*/
-static inline void iommufd_object_destroy_user(struct iommufd_ctx *ictx,
- struct iommufd_object *obj)
+static inline void __iommufd_object_destroy_user(struct iommufd_ctx *ictx,
+ struct iommufd_object *obj,
+ unsigned int flags)
{
int ret;
- ret = iommufd_object_remove(ictx, obj, obj->id, REMOVE_WAIT_SHORTTERM);
+ ret = iommufd_object_remove(ictx, obj, obj->id, flags | REMOVE_WAIT_SHORTTERM);
/*
* If there is a bug and we couldn't destroy the object then we did put
@@ -213,6 +214,12 @@ static inline void iommufd_object_destroy_user(struct iommufd_ctx *ictx,
WARN_ON(ret);
}
+static inline void iommufd_object_destroy_user(struct iommufd_ctx *ictx,
+ struct iommufd_object *obj)
+{
+ return __iommufd_object_destroy_user(ictx, obj, 0);
+}
+
/*
* The HWPT allocated by autodomains is used in possibly many devices and
* is automatically destroyed when its refcount reaches zero.
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))
obj = ERR_PTR(-ENOENT);
xa_unlock(&ictx->objects);
@@ -157,17 +158,27 @@ int iommufd_object_remove(struct iommufd_ctx *ictx,
ret = -ENOENT;
goto err_xa;
}
- } else if (xa_is_zero(obj) || !obj) {
+ } else if (xa_is_zero(obj)) {
+ /* We can reclaim the id now. */
+ xas_store(&xas, NULL);
+ ret = 0;
+ goto err_xa;
+ } else if (!obj) {
ret = -ENOENT;
goto err_xa;
}
if (!refcount_dec_if_one(&obj->users)) {
- ret = -EBUSY;
- goto err_xa;
+ if (flags & REMOVE_OBJ_FORCE) {
+ xas_store(&xas, XA_ZERO_ENTRY);
+ } else {
+ ret = -EBUSY;
+ goto err_xa;
+ }
+ } else {
+ xas_store(&xas, NULL);
}
- xas_store(&xas, NULL);
if (ictx->vfio_ioas == container_of(obj, struct iommufd_ioas, obj))
ictx->vfio_ioas = NULL;
xa_unlock(&ictx->objects);
diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
index d9749d9d2ffb..4fc74ada0e62 100644
--- 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;
out_abort:
Powered by blists - more mailing lists