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: <ZsOKe10cOTCcHOIP@Asurada-Nvidia>
Date: Mon, 19 Aug 2024 11:10:03 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...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 Mon, Aug 19, 2024 at 02:33:32PM -0300, Jason Gunthorpe wrote:
> 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.

Hmm, would that be unsafe? I start to wonder if we should allow an
attach to viommu and put validations on that?

> > 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.

Yea, I just figured that out... You have been right. I was able to
get rid of the locking problem with invalidation API. But then irq
became a headache as drivers would only know the dev pointer, so
everything that the dev could convert to would be unsafe as it can
not grab the idev/viommu locks until it converts.

Thanks
Nicolin