Re: [PATCH] iommu/vt-d: Add a fix for devices need extra dtlb flush

[Jacob Pan]

Hi Baolu,

On Thu, 24 Nov 2022 10:52:32 +0800, Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
wrote:

> On 11/23/22 7:32 PM, Robin Murphy wrote:
> > On 2022-11-23 05:18, Tian, Kevin wrote:  
> >>> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> >>> Sent: Wednesday, November 23, 2022 1:04 PM
> >>>
> >>> On 2022/11/23 9:02, Tian, Kevin wrote:  
> >>>>> From: Robin Murphy <robin.murphy@xxxxxxx>
> >>>>> Sent: Wednesday, November 23, 2022 1:49 AM
> >>>>>  
> >>>>>> +
> >>>>>> +/* Impacted QAT device IDs ranging from 0x4940 to 0x4943 */
> >>>>>> +#define BUGGY_QAT_DEVID_MASK 0x494c
> >>>>>> +static bool dev_needs_extra_dtlb_flush(struct pci_dev *pdev)
> >>>>>> +{
> >>>>>> +    if (pdev->vendor != PCI_VENDOR_ID_INTEL)
> >>>>>> +        return false;
> >>>>>> +
> >>>>>> +    if ((pdev->device & 0xfffc) != BUGGY_QAT_DEVID_MASK)
> >>>>>> +        return false;
> >>>>>> +
> >>>>>> +    if (risky_device(pdev))
> >>>>>> +        return false;  
> >>>>>
> >>>>> Hmm, I'm not sure that that makes much sense to me - what privilege 
> >>>>> can
> >>>>> the device gain from being told to invalidate things twice? Why 
> >>>>> would we
> >>>>> want to implicitly *allow* a device to potentially keep using a
> >>>>> stale translation if for some bizarre reason firmware has marked it
> >>>>> as external, surely that's worse?  
> >>>
> >>>   From the perspective of IOMMU, any quirk is only applicable to
> >>> trusted devices. If the IOMMU driver detects that a quirk is being
> >>> applied to an untrusted device, it is already buggy or malicious. The
> >>> IOMMU driver should let the users know by:
> >>>
> >>>     pci_info(pdev,
> >>>          "Skipping IOMMU quirk for dev [%04X:%04X] on untrusted
> >>> PCI link\n",
> >>>          pdev->vendor, pdev->device);
> >>>     pci_info(pdev, "Please check with your BIOS/Platform vendor about
> >>> this\n");
> >>>
> >>> and stop applying any quirk.
> >>>  
> >>
> >> A quirk usually relaxes something then you want it only on trusted 
> >> devices.
> >>
> >> but the quirk in this patch is trying to fix a vulnerability. In 
> >> concept it's
> >> weird to skip it on untrusted devices. This iiuc was the part causing 
> >> confusion
> >> to Robin.  
> > 
> > Right, it's that reasoning in general that seems bogus to me. Clearly 
> > any quirk that effectively grants additional privileges, like an 
> > identity mapping quirk, should not be applied to untrusted external 
> > devices which may be spoofing an affected VID/DID to gain that 
> > privilege, but not all quirks imply privilege. If, say, a WiFI 
> > controller needs something innocuous like a DMA alias or address width 
> > quirk to function correctly, it will still need that regardless of 
> > whether it's soldered to a motherboard or to a removable expansion
> > card, and it would do nobody any good to deny correct functionality
> > based on that unnecessary distinction. Yes, I appreciate that in
> > practice many of those kind of quirks will be applied in other layers
> > anyway, but I still think it's wrong to make a sweeping assumption that
> > all IOMMU-level quirks are precious treasure not to be shared with
> > outsiders, rather than assess their impact individually. The detriment
> > in this case is small (just needless code churn), but even that's still
> > not nothing.  
> 
> Fair enough. I agreed here.
> 
> Can we put some comments here so that people can still easily read the
> discussion here after a long time?

Sure, I will remove risky_device(pdev) check and add a comment explaining
the exemption.


Thanks,

Jacob