Messages in this thread | | | Date | Mon, 28 Nov 2022 08:02:25 -0800 | From | Jacob Pan <> | Subject | Re: [PATCH] iommu/vt-d: Add a fix for devices need extra dtlb flush |
| |
Hi Baolu,
On Thu, 24 Nov 2022 10:52:32 +0800, Baolu Lu <baolu.lu@linux.intel.com> 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@linux.intel.com> > >>> Sent: Wednesday, November 23, 2022 1:04 PM > >>> > >>> On 2022/11/23 9:02, Tian, Kevin wrote: > >>>>> From: Robin Murphy <robin.murphy@arm.com> > >>>>> 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
| |