Re: [PATCH 1/2] VT-d: avoid NULL deref on domain_context_mapping_one() error paths

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.04.2022 15:36, Roger Pau Monné wrote:
> On Wed, Apr 06, 2022 at 02:24:32PM +0200, Jan Beulich wrote:
>> First there's a printk() which actually wrongly uses pdev in the first
>> place: We want to log the coordinates of the (perhaps fake) device
>> acted upon, which may not be pdev.
>>
>> Then it was quite pointless for eb19326a328d ("VT-d: prepare for per-
>> device quarantine page tables (part I)") to add a domid_t parameter to
>> domain_context_unmap_one(): It's only used to pass back here via
>> me_wifi_quirk() -> map_me_phantom_function(). Drop the parameter again.
>>
>> Finally there's the invocation of domain_context_mapping_one(), which
>> needs to be passed the correct domain ID. Avoid taking that path when
>> pdev is NULL and the quarantine state is what would need restoring to.
>> This means we can't security-support PCI devices with RMRRs (if such
>> exist in practice) any longer.
>>
>> Fixes: 8f41e481b485 ("VT-d: re-assign devices directly")
>> Fixes: 14dd241aad8a ("IOMMU/x86: use per-device page tables for 
>> quarantining")
>> Coverity ID: 1503784
>> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/SUPPORT.md
>> +++ b/SUPPORT.md
>> @@ -750,6 +750,10 @@ However, this feature can still confer s
>>  when used to remove drivers and backends from domain 0
>>  (i.e., Driver Domains).
>>  
>> +On VT-d (Intel hardware) passing through plain PCI (or PCI-X) devices
>> +when they have associated Reserved Memory Regions (RMRRs)
>> +is not security supported, if such a combination exists in the first place.
> 
> Hm, I think this could be confusing from a user PoV.  It's my
> understanding you want to differentiate between DEV_TYPE_PCIe_ENDPOINT
> and DEV_TYPE_PCI device types, so maybe we could use:
> 
> "On VT-d (Intel hardware) passing through non PCI Express devices with
> associated Reserved Memory Regions (RMRRs) is not supported."
> 
> AFAICT domain_context_mapping will already prevent this from happening
> by returning -EOPNOTSUPP (see the DEV_TYPE_PCI case handling).

Hmm. I did look at that code while writing the patch, but I didn't
draw the same conclusion. I'd like to tie the security support
statement to what could technically be made work. IOW I don't like
to say "not supported"; I'd like to stick to "not security
supported", which won't change even if that -EOPNOTSUPP path would
be replaced by a proper implementation. Even adding a sentence to
say this doesn't even work at present would seem to me to go too
far: Such a sentence would easily be forgotten if the situation
changed. But I'd be willing to add such an auxiliary statement as
a compromise.

As to "plain PCI (or PCI-X)" vs "non PCI Express" - while I prefer
to avoid a negation there, I'd be okay to switch if that's deemed
better for potential readers.

>> @@ -1601,9 +1601,13 @@ int domain_context_mapping_one(
>>  
>>      if ( rc )
>>      {
>> -        if ( !prev_dom )
>> -            ret = domain_context_unmap_one(domain, iommu, bus, devfn,
>> -                                           DEVICE_DOMID(domain, pdev));
>> +        if ( !prev_dom ||
>> +             /*
>> +              * Unmapping here means PCI devices with RMRRs (if such exist)
>> +              * will cause problems if such a region was actually accessed.
>> +              */
>> +             (prev_dom == dom_io && !pdev) )
> 
> Maybe I'm reading this wrong, but plain PCI devices with RMRRs are
> only allowed to be assigned to the hardware domain, and won't be able
> to be reassigned afterwards.  It would be fine to unmap
> unconditionally if !prev_dom or !pdev?  As calls with !pdev only
> happening for phantom functions or bridge devices.

Like with the support statement, I'd prefer this code to be independent
of the (perhaps temporary) decision to not allow certain assignments.

Since you mention phantom functions: Aiui their mapping operations will
be done with a non-NULL pdev, unless of course they're phantom functions
associated with a non-PCIe device (in which case the same secondary
mappings with a NULL pdev would occur - imo pointlessly, as it would
be the same bridge and the same secondary bus as for the actual device;
I'm under the impression that error handling may not work properly when
such redundant mappings occur).

Jan