Re: [Xen-devel] [PATCH for-4.12] passthrough/vtd: Drop the "workaround_bios_bug" logic entirely

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.19 at 12:02, <george.dunlap@xxxxxxxxxx> wrote:
> On 3/25/19 3:24 PM, Jan Beulich wrote:
>>>>> On 21.03.19 at 21:26, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> It turns out that this code was previously dead.
>> 
>> If it was entirely dead, why the rush to get the change into 4.12?
>> (I suppose the later parts of description are then justifying why
>> the code wasn't actually dead, and why it was getting in the way,
>> but I think this way of putting it is at least confusing.)
>> 
>>> c/s dcf41790 " x86/mmcfg/drhd: Move acpi_mmcfg_init() call before calling
>>> acpi_parse_dmar()" resulted in PCI segment 0 now having been initialised
>>> enough for acpi_parse_one_drhd() to not take the
>>>
>>>   /* Skip checking if segment is not accessible yet. */
>>>
>>> path unconditionally.
>> 
>> Or am I misreading the initial sentence, and you're really suggesting
>> that prior to said commit the code in question had been dead? How's
>> that commit related then? Segment 0 is valid irrespective of any
>> MMCFG information gained from ACPI tables (see pci_segments_init()).
>> 
>>>  However, some systems have DMAR tables which list
>>> devices which are disabled by user choice (in particular, Dell PowerEdge 
> R740
>>> with I/O AT DMA disabled), and turning off all IOMMU functionality in this
>>> case is entirely unhelpful behaviour.
>> 
>> As in many other cases, what is or is not unhelpful is often a matter
>> of perception and hence possibly subjective. I can see your point,
>> but I also can see why the authors of the code considered it a rather
>> bad sign if non-existing PCI devices get named by an ACPI table.
>> What if e.g. later a device gets hot-plugged into that very SBDF?
>> 
>>> Leave the warning which identifies the problematic devices, but drop the
>>> remaining logic.  This leaves the system in better overall state, and 
> working
>>> in the same way that it did in previous releases.
>> 
>> I wonder whether you've taken the time to look at the description
>> of the commit first introducing this logic (a8059ffced "VT-d: improve
>> RMRR validity checking"). I find it worrying in particular to
>> effectively revert a change which claims 'to avoid any security
>> vulnerability with malicious s/s re-enabling "supposed disabled"
>> devices' without any discussion of why that may have been a
>> wrong perspective to take.
> 
> Having read the patch description, I think you have the polarity of that
> comment wrong.
> 
> If I understand correctly, that patch disables part of the IOMMU if it
> finds something strange, *unless* iommu=force is on.  The text gives the
> idea that it is *less* safe to disable the IOMMU; and that enabling the
> IOMMU functionality, even with invalid entries, is safer than leaving it
> off.

Hmm, indeed, I did associate the security vulnerability statement
with the wrong context. Yet still, "iommu=force" is what is precisely
meant for such a situation: Enable the IOMMU despite there having
been some issues.

> The patch we checked in (if I understand correctly), enables the IOMMU
> in more situations, even when iommu=force is not set; and thus (by the
> logic of the original patch) is more "safe" by default than the previous
> patch.

I'm not sure about this conclusion of yours: If the goal had been to
make things "more secure" by default, why would they have disabled
the IOMMU in the first place when finding non-discoverable devices?

How do we know (in the abstract general case) that enabling the
IOMMU is not going to cause well hidden problems when the firmware
provided data is inconsistent? Leaving the IOMMU off in such a case
puts the system in a well known (albeit likely less secure) state.
Turning the IOMMU on, otoh, puts the system in an unknown (albeit
likely more secure) state. This calls for an admin decision, and I
continue to think that the well known state is preferable as the
default, because of the risk that the firmware flaw opens an unknown
security hole.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel