Re: [PATCH v3 03/17] iommu: Make __iommu_group_set_domain() handle error unwind

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Apr 06, 2023 at 07:08:59PM +0100, Robin Murphy wrote:

> > +		if (ret) {
> > +			result = ret;
> > +			/*
> > +			 * Keep trying the other devices in the group. If a
> > +			 * driver fails attach to an otherwise good domain, and
> > +			 * does not support blocking domains, it should at least
> > +			 * drop its reference on the current domain so we don't
> > +			 * UAF.
> > +			 */
> > +			if (flags & IOMMU_SET_DOMAIN_MUST_SUCCEED)
> > +				continue;
> 
> I know it's called out above, but my brain truly can't cope with a flag
> named "MUST_SUCCEED" which actually means "can fail, but you can't reason
> about the state after failure". As before, I think the actual flag itself
> can go away, but the behaviour itself will still be no nicer for being
> implicit :/

What I saw is that a bunch of drivers tend to implement attach_dev
with first some kind of detach, which often cannot fail, and then can
fail the new attach.

So, this is intended to go along with that design pattern. We call
every device's attach and hope that the outcome is either to release
the current iommu_domain or to attach to the new one.

If a driver doesn't follow this pattern then we'll UAF.

The primary purpose of the MUST_SUCCEED flag is to prevent the code
from trying to go back to the old domain. This really should only
depend on the caller's context and ability to handle errors, so I
don't see what would make it implicit?

We definitely do not want to attach back the old domain if we are
about to unconditionally free it.

> TBH I still think the responsibility should be on the drivers to handle
> rollback of their internal per-device state if returning failure from an
> .attach_dev call.

It helps the driver stay consistent, but I don't think it helps the
core code too much.

With the two patches to add the rodata IDENTITY domain to drivers I
think we should go in that direction.

Lets have an "ops->emergency_domain" that is something the driver can
always attach. Ideally it will be the BLOCKING domain, but IDENTITY
could work too.

When this function gets into trouble it will put the entire group on
'emergency_domain' and things will be safe, no UAF.

This seems pretty simple for drivers since the attach_dev can just
leave behind whatever state is convenient, so long as their
emergency_domain cannot fail.

> But what would be really really really good, now that .detach_dev has
> truly gone, is to pass *both* "to" and "from" domains to .attach_dev (and a
> "from" domain to .release_device), so that drivers don't have to squirrel
> away all these internal references to their own notions of "current" domains
> which are the root of this whole class of horrid UAF and consistency
> bugs.

Usually the IOMMU HW will be touching the IOPTEs via DMA enclosed by
the iommu_domain, so even if we could remove the iommu_domain pointers
from the drivers it still retains the HW programming to the
memory. Freeing the domain will still UAF.

Refcounts would be another way to address the UAF, then we can leak
the iommu_domain if things go bad. I think this is more complex on drivers.

Jason