linux-kernel - Re: [PATCH v1 05/16] iommufd/viommu: Add IOMMU_VIOMMU_SET/UNSET_VDEV

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: <20240819173332.GM2032816@nvidia.com>
Date: Mon, 19 Aug 2024 14:33:32 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Nicolin Chen <nicolinc@...dia.com>
Cc: kevin.tian@...el.com, will@...nel.org, joro@...tes.org,
	suravee.suthikulpanit@....com, robin.murphy@....com,
	dwmw2@...radead.org, baolu.lu@...ux.intel.com, shuah@...nel.org,
	linux-kernel@...r.kernel.org, iommu@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org,
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v1 05/16] iommufd/viommu: Add
 IOMMU_VIOMMU_SET/UNSET_VDEV_ID ioctl

On Thu, Aug 15, 2024 at 05:21:57PM -0700, Nicolin Chen wrote:

> > Why not? The idev becomes linked to the viommu when the dev id is set
> 
> > Unless we are also going to enforce the idev is always attached to a
> > nested then I don't think we need to check it here.
> > 
> > Things will definately not entirely work as expected if the vdev is
> > directly attached to the s2 or a blocking, but it won't harm anything.
> 
> My view is that, the moment there is a VIOMMU object, that must
> be a nested IOMMU case, so there must be a nested hwpt. Blocking
> domain would be a hwpt_nested too (vSTE=Abort) as we previously
> concluded.

I'm not sure other vendors can do that vSTE=Abort/Bypass thing though
yet..

> Then, in a nested case, it feels odd that an idev is attached to
> an S2 hwpt..
>
> That being said, I think we can still do that with validations:
>  If idev->hwpt is nested, compare input viommu v.s idev->hwpt->viommu.
>  If idev->hwpt is paging, compare input viommu->hwpt v.s idev->hwpt.

But again, if you don't contiguously validate those invariants in all
the other attach paths it is sort of pointless to check them since the
userspace can still violate things.

> This complicates things overall especially with the VIRQ that has
> involved interrupt context polling vdev_id, where semaphore/mutex
> won't fit very well. Perhaps it would need a driver-level bottom
> half routine to call those helpers with locks. I am glad that you
> noticed the problem early.

I think you have to show the xarray to the driver and the driver can
use the spinlock to access it safely. Keeping it hidden in the core
code is causing all these locking problems.

Jason