Re: [PATCH v5 10/14] iommu/amd: Introduce logic to enable/disable IOPF

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Feb 06, 2024 at 10:59:03PM +0530, Vasant Hegde wrote:
> On 2/6/2024 10:06 PM, Jason Gunthorpe wrote:
> > On Tue, Feb 06, 2024 at 09:49:36PM +0530, Vasant Hegde wrote:
> > 
> >>> This dte change is in the wrong place. When the domain is first
> >>> attached we know if it requires PRI or not. At that moment the DTE
> >>> should be set properly, it should not be set wrong and then changed
> >>> later.
> >>
> >> We want to enable PPR support only after setting up the handler.
> > 
> > That's backwards. It means error handling can't really be sane..
> 
> Why do you think enabling feature only when we are really going to
> use it backwards?

Because you touch the DTE twice, it means the domain is installed in
an inconsistent state where it is not actually working
properly. Domain updates should not "tear" like that.

You already know what is going to happen at the very start of attach,
you don't need to "enable it after" just do it right the first time
through.

There is a clear protocol and ordering requirement for the PRI
enablement. Lu described it in a comment, make sure you follow it.

You also have to think about what happens during detach and what
happens on all the pairs of attach -> attach.

The error unwinds are tricky, the only way I could make it all be
correct for ARM was to fix the attach handling so that there is no
failure scenario after the DTE is updated. ie the attach functions
either do nothing or fully succeed.

The situation where attach fails and leaves the HW in an unknown state
is really hard to deal with - and without the reliable global blocked
domain the core code can't 100% rescue it either.

> >>> (and again the whole dte setting flow needs cleaning, I think you
> >>> should do that before trying to build more complex stuff on top)
> >>
> >> Yeah. I want to fix few things in that path. But that's outside this series.
> > 
> > I was looking at it and there are many security bugs in here now that
> > iommufd can change the DTE at any time. The current design assumes DMA
> > will be stopped and ignores the spec guidance on how to do a safe DTE
> > update :(
> 
> What security issues are you referring? Can you elaborate?

The DTE is not updated correctly. The HW can read inconsistent
versions of it with unpredictable - and possibly security bad -
results.

Like it doesn't even write the two qwords of the DTE in a predicatble
order! Let alone worrying about the 3 qw update or being correct with
races during an ITE touch :(

This doesn't matter so much if there is no DMA active while the DTE is
being changed, which could sort of reasonably be assumed up till
iommufd allowed it to happen under userspace control.

Now a driver cannot make the assumption that DMA is halted. It must
follow all the protocols to ensure that HW observes only exactly the
DTEs/etc it is trying to build and not something random.

The documentation is pretty clear how this is supposed to work. It is
the same as ARM. Use atomic 64/128 bit stores, rely on 'ignored
behavior' or use the valid bit.

This also means, broadly, you can't allow the DTE to evolve during the
operation of attach/detach as the in-between states may become
userspace visible and may be harmful in some way.

> > I'm really not comfortable with adding more stuff here until the
> > security issue is solved. Especially if the more stuff is drifting
> > further from being correct. If you can keep the updates in set_dte
> > then maybe with some reluctance. But not like this with random touches
> > to the DTE all over the place.
>
> Currently all DTE update is happening inside set_dte only (dirty bit enable is
> an exception that may need to moved inside set_dte). This patch just invokes
> that set_dte and invalidates cache.

So then why all this strangeness?? Just set dev_data->ppr earlier in
attach and order the handler setup properly.

It should be really simple:

 // All protected by the core's group mutex

 if (domain->needs_pri)  {
     dev_data->ppr = true;
     if (!dev_data->num_pri_domains)
       // enable fault queues for the device
     dev_data->num_pri_domains++
 }

 if (old_domain->needs_pri) {
     dev_data->num_pri_domains--;
     if (!dev_data->num_pri_domains) {
         dev_data->ppr = false;
         disable pri at PCI()
     }
 }

 set_dte()

 if (domain->needs_pr)
    enable pri at PCI()

The order here is really important too!

Since PRI can only be supported when a GCR3 is present, this should
all be part of some generic 'install GCR3 table DTE' routine that is
called on all the attach paths.

Jason