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

[Vasant Hegde <vasant.hegde@amd.com>]

On 2/6/2024 11:28 PM, Jason Gunthorpe wrote:
> 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.

First time we will not know whether device will actually use fault handler or
not. All we will know is whether IOMMU and device is capable of PRI or not.

We can make assumption that it may use and just enable it.


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

Where? in intel driver? (they seems to be using feature_enable() path)

I did look into latest "iommu: Prepare to deliver page faults to user space"
series. I don't see anything specific to PRI enablement flow.


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

ok.

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

If attach fails we throw error message and skip updating DTE. I believe core
layer understands that driver failed to attach device and puts device/group to
its original domain. So things should work fine.

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

We make DTE changes in set_dte() function only (except dirty bit change that
will be consolidated).


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

What is PRI domain?

If I have to enable PRI in attach path then I don't need to track number of
domain stuff. I can simply do something like
	if (pdom_is_sva_capable(pdom))
		// enable PRI in IOMMU
		// enable device PRI

and in detach path,
	if (PRI is enabled)
		// disable IOMMU/device PRI stuff


-Vasant

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