Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 28 Nov 2022 08:02:25 -0800
From:   Jacob Pan <jacob.jun.pan@linux.intel.com>
To:     Baolu Lu <baolu.lu@linux.intel.com>
Cc:     Robin Murphy <robin.murphy@arm.com>,
        "Tian, Kevin" <kevin.tian@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
        Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        David Woodhouse <dwmw2@infradead.org>,
        "Raj, Ashok" <ashok.raj@intel.com>,
        "Liu, Yi L" <yi.l.liu@intel.com>,
        "Luo, Yuzhang" <yuzhang.luo@intel.com>,
        jacob.jun.pan@linux.intel.com
Subject: Re: [PATCH] iommu/vt-d: Add a fix for devices need extra dtlb flush
Message-ID: <20221128080225.6d51a474@jacob-builder>
In-Reply-To: <992ba86c-73e6-8db0-0216-c2a8b7d1f58f@linux.intel.com>
References: <20221122034529.3311562-1-jacob.jun.pan@linux.intel.com>
        <bc647ceb-b14b-dc5b-ba83-4f8befd188c5@arm.com>
        <BN9PR11MB527620902046A4339EAACD3F8C0C9@BN9PR11MB5276.namprd11.prod.outlook.com>
        <c085f67d-7874-4a83-a12c-703d1638d940@linux.intel.com>
        <BN9PR11MB527642B1DA920C78742036FD8C0C9@BN9PR11MB5276.namprd11.prod.outlook.com>
        <53678e27-1bbc-a7e8-a1b0-0427fc0e5b62@arm.com>
        <992ba86c-73e6-8db0-0216-c2a8b7d1f58f@linux.intel.com>
Organization: OTC
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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: =20
> >>> 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: =20
> >>>>> From: Robin Murphy <robin.murphy@arm.com>
> >>>>> Sent: Wednesday, November 23, 2022 1:49 AM
> >>>>> =20
> >>>>>> +
> >>>>>> +/* 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)
> >>>>>> +{
> >>>>>> +=C2=A0=C2=A0=C2=A0 if (pdev->vendor !=3D PCI_VENDOR_ID_INTEL)
> >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return false;
> >>>>>> +
> >>>>>> +=C2=A0=C2=A0=C2=A0 if ((pdev->device & 0xfffc) !=3D BUGGY_QAT_DEV=
ID_MASK)
> >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return false;
> >>>>>> +
> >>>>>> +=C2=A0=C2=A0=C2=A0 if (risky_device(pdev))
> >>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return false; =20
> >>>>>
> >>>>> Hmm, I'm not sure that that makes much sense to me - what privilege=
=20
> >>>>> can
> >>>>> the device gain from being told to invalidate things twice? Why=20
> >>>>> 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? =20
> >>>
> >>> =C2=A0 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:
> >>>
> >>> =C2=A0=C2=A0=C2=A0=C2=A0pci_info(pdev,
> >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "Skipping IOMMU quir=
k for dev [%04X:%04X] on untrusted
> >>> PCI link\n",
> >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pdev->vendor, pdev->=
device);
> >>> =C2=A0=C2=A0=C2=A0=C2=A0pci_info(pdev, "Please check with your BIOS/P=
latform vendor about
> >>> this\n");
> >>>
> >>> and stop applying any quirk.
> >>> =20
> >>
> >> A quirk usually relaxes something then you want it only on trusted=20
> >> devices.
> >>
> >> but the quirk in this patch is trying to fix a vulnerability. In=20
> >> concept it's
> >> weird to skip it on untrusted devices. This iiuc was the part causing=
=20
> >> confusion
> >> to Robin. =20
> >=20
> > Right, it's that reasoning in general that seems bogus to me. Clearly=20
> > any quirk that effectively grants additional privileges, like an=20
> > identity mapping quirk, should not be applied to untrusted external=20
> > devices which may be spoofing an affected VID/DID to gain that=20
> > privilege, but not all quirks imply privilege. If, say, a WiFI=20
> > controller needs something innocuous like a DMA alias or address width=
=20
> > quirk to function correctly, it will still need that regardless of=20
> > 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. =20
>=20
> Fair enough. I agreed here.
>=20
> 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