linux-kernel - Re: [PATCH 00/14] iommu: fix device leaks

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: <20251002130116.GB3195829@ziepe.ca>
Date: Thu, 2 Oct 2025 10:01:17 -0300
From: Jason Gunthorpe <jgg@...pe.ca>
To: Robin Murphy <robin.murphy@....com>
Cc: Johan Hovold <johan@...nel.org>, Joerg Roedel <joro@...tes.org>,
	Will Deacon <will@...nel.org>, Sven Peter <sven@...nel.org>,
	Janne Grunau <j@...nau.net>,
	Rob Clark <robin.clark@....qualcomm.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Yong Wu <yong.wu@...iatek.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Chen-Yu Tsai <wens@...e.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Krishna Reddy <vdumpa@...dia.com>, iommu@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 00/14] iommu: fix device leaks

On Thu, Oct 02, 2025 at 12:48:35PM +0100, Robin Murphy wrote:
> > However the SMMU drivers are doing this under the
> > iommu_probe_device_lock and not stashing the pointer into a drvdata
> > where there is no locking protecting it.
> 
> Huh? Every .of_xlate call is under probe_device_lock just as much as
> .probe_device calls are; they *have* to be, since they too are in the
> position of working with dev->iommu before dev->iommu_group is set to
> guarantee its stability.

Yes, of_xlate is under the lock, but IIRC there are still cases where
probe gets deferred, the probe_device_lock is unlocked, and the
drvdata continues to exist.

> indeed the driver cannot be removed, because we hold a module reference
> around the call

That's not how module reference counts work. Drivers can be unbound
through sysfs at any time.

> > Anyhow, I drafted a nice fix for all of this. After all the rework it
> > is trivial for the core code to pass in the struct iommu_device * to
> > probe and then most of the drivers just drop this ugly code
> > completely.
> >
> > https://github.com/jgunthorpe/linux/commits/iommu-fwspec/
> 
> Eww, that is neither nice nor a "fix". Once again it's just piling on a load
> of extra complexity to have multiple confusingly-overlapping-but-different
> ways of doing the same thing, one of which is still the exact same one
> you've decided to object to because you've failed to understand it (as
> demonstrated by the commit message below, the obvious bug in the hideous
> mess below that, and at a glance the other patches actively *breaking* at
> least one driver.)

It is hard to take you seriously with such vauge objections
Robin. Please try to be constructive. If you are specific I'll go fix
the things and maybe other people besides you can understand this
stuff.

I'm shocked you disagree with the core code helping the drivers find
their iommu_driver. This seems like very basic obvious good stuff. We
don't need all sorts of different open coded ugly ways for drivers to
do this. It removes 200 lines of junk for drivers :\

> > +			iommu = fwspec_iommu->ops->probe_device(dev);
> > +			if (IS_ERR(iommu))
> > +				return PTR_ERR(iommu);
> > +			if (WARN_ON(iommu != fwspec_iommu)) {

This is at least one typo.

Jason