linux-kernel - Re: [RFC PATCH 17/30] iommufd/device: Add TSM Bind/Unbind for TIO support

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: <aD6UQy4KwKcdSvVE@yilunxu-OptiPlex-7050>
Date: Tue, 3 Jun 2025 14:20:51 +0800
From: Xu Yilun <yilun.xu@...ux.intel.com>
To: "Aneesh Kumar K.V" <aneesh.kumar@...nel.org>
Cc: kvm@...r.kernel.org, sumit.semwal@...aro.org, christian.koenig@....com,
	pbonzini@...hat.com, seanjc@...gle.com, alex.williamson@...hat.com,
	jgg@...dia.com, dan.j.williams@...el.com, aik@....com,
	linux-coco@...ts.linux.dev, dri-devel@...ts.freedesktop.org,
	linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org,
	vivek.kasireddy@...el.com, yilun.xu@...el.com,
	linux-kernel@...r.kernel.org, lukas@...ner.de, yan.y.zhao@...el.com,
	daniel.vetter@...ll.ch, leon@...nel.org, baolu.lu@...ux.intel.com,
	zhenzhong.duan@...el.com, tao1.su@...el.com,
	linux-pci@...r.kernel.org, zhiw@...dia.com, simona.vetter@...ll.ch,
	shameerali.kolothum.thodi@...wei.com, iommu@...ts.linux.dev,
	kevin.tian@...el.com
Subject: Re: [RFC PATCH 17/30] iommufd/device: Add TSM Bind/Unbind for TIO
 support

On Mon, Jun 02, 2025 at 06:13:16PM +0530, Aneesh Kumar K.V wrote:
> Xu Yilun <yilun.xu@...ux.intel.com> writes:
> 
> ....
> 
> > +/**
> > + * iommufd_device_tsm_bind - Move a device to TSM Bind state
> > + * @idev: device to attach
> > + * @vdev_id: Input a IOMMUFD_OBJ_VDEVICE
> > + *
> > + * This configures for device Confidential Computing(CC), and moves the device
> > + * to the TSM Bind state. Once this completes the device is locked down (TDISP
> > + * CONFIG_LOCKED or RUN), waiting for guest's attestation.
> > + *
> > + * This function is undone by calling iommufd_device_tsm_unbind().
> > + */
> > +int iommufd_device_tsm_bind(struct iommufd_device *idev, u32 vdevice_id)
> > +{
> > +	struct iommufd_vdevice *vdev;
> > +	int rc;
> > +
> > +	if (!dev_is_pci(idev->dev))
> > +		return -ENODEV;
> > +
> > +	vdev = container_of(iommufd_get_object(idev->ictx, vdevice_id, IOMMUFD_OBJ_VDEVICE),
> > +			    struct iommufd_vdevice, obj);
> > +	if (IS_ERR(vdev))
> > +		return PTR_ERR(vdev);
> > +
> > +	if (vdev->dev != idev->dev) {
> > +		rc = -EINVAL;
> > +		goto out_put_vdev;
> > +	}
> > +
> > +	mutex_lock(&idev->igroup->lock);
> > +	if (idev->vdev) {
> > +		rc = -EEXIST;
> > +		goto out_unlock;
> > +	}
> > +
> > +	rc = iommufd_vdevice_tsm_bind(vdev);
> > +	if (rc)
> > +		goto out_unlock;
> > +
> > +	idev->vdev = vdev;
> > +	refcount_inc(&vdev->obj.users);
> > +	mutex_unlock(&idev->igroup->lock);
> > +
> > +	/*
> > +	 * Pairs with iommufd_device_tsm_unbind() - catches caller bugs attempting
> > +	 * to destroy a bound device.
> > +	 */
> > +	refcount_inc(&idev->obj.users);
> >
> 
> Do we really need this refcount_inc? As I understand it, the objects

The idev refcount is not necessary, it is just to "catch caller bug".

> aren't being pinned directly. Instead, the reference count seems to be
> used more as a way to establish an object hierarchy, ensuring that
> objects are freed in the correct order.
> 
> In vfio_pci_core_close_device(), you’re decrementing the reference, and
> on the iommufd side, we’re covered because the VFIO bind operation takes
> a file reference (fget)—so iommufd_fops_release() won’t be called
> prematurely.

Correct.

> 
> Wouldn’t it be simpler to skip the reference count increment altogether
> and just call tsm_unbind in the virtual device’s destroy callback?
> (iommufd_vdevice_destroy())

The vdevice refcount is the main concern, there is also an IOMMU_DESTROY
ioctl. User could just free the vdevice instance if no refcount, while VFIO
is still in bound state. That seems not the correct free order.

Thanks,
Yilun

> 
> > +	goto out_put_vdev;
> > +
> > +out_unlock:
> > +	mutex_unlock(&idev->igroup->lock);
> > +out_put_vdev:
> > +	iommufd_put_object(idev->ictx, &vdev->obj);
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iommufd_device_tsm_bind, "IOMMUFD");
> 
> -aneesh