[PATCH] iommu/vt-d: Don't apply gfx quirks to untrusted devices

[PATCH] iommu/vt-d: Don't apply gfx quirks to untrusted devices

[Rajat Jain]

Currently, an external malicious PCI device can masquerade the VID:PID
of faulty gfx devices, and thus apply iommu quirks to effectively
disable the IOMMU restrictions for itself.

Thus we need to ensure that the device we are applying quirks to, is
indeed an internal trusted device.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 drivers/iommu/intel-iommu.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index ef0a5246700e5..f2a480168a02f 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -6214,6 +6214,11 @@ const struct iommu_ops intel_iommu_ops = {
 
 static void quirk_iommu_igfx(struct pci_dev *dev)
 {
+	if (dev->untrusted) {
+		pci_warn(dev, "skipping iommu quirk for untrusted gfx dev\n");
+		return;
+	}
+
 	pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
 	dmar_map_gfx = 0;
 }
@@ -6255,6 +6260,11 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x163D, quirk_iommu_igfx);
 
 static void quirk_iommu_rwbf(struct pci_dev *dev)
 {
+	if (dev->untrusted) {
+		pci_warn(dev, "skipping iommu quirk for untrusted dev\n");
+		return;
+	}
+
 	/*
 	 * Mobile 4 Series Chipset neglects to set RWBF capability,
 	 * but needs it. Same seems to hold for the desktop versions.
@@ -6285,6 +6295,11 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
 {
 	unsigned short ggc;
 
+	if (dev->untrusted) {
+		pci_warn(dev, "skipping iommu quirk for untrusted gfx dev\n");
+		return;
+	}
+
 	if (pci_read_config_word(dev, GGC, &ggc))
 		return;
 
@@ -6318,6 +6333,13 @@ static void __init check_tylersburg_isoch(void)
 	pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x3a3e, NULL);
 	if (!pdev)
 		return;
+
+	if (pdev->untrusted) {
+		pci_warn(pdev, "skipping iommu quirk due to untrusted dev\n");
+		pci_dev_put(pdev);
+		return;
+	}
+
 	pci_dev_put(pdev);
 
 	/* System Management Registers. Might be hidden, in which case
@@ -6327,6 +6349,12 @@ static void __init check_tylersburg_isoch(void)
 	if (!pdev)
 		return;
 
+	if (pdev->untrusted) {
+		pci_warn(pdev, "skipping iommu quirk due to untrusted dev\n");
+		pci_dev_put(pdev);
+		return;
+	}
+
 	if (pci_read_config_dword(pdev, 0x188, &vtisochctrl)) {
 		pci_dev_put(pdev);
 		return;
-- 
2.27.0.rc2.251.g90737beb825-goog

Re: [PATCH] iommu/vt-d: Don't apply gfx quirks to untrusted devices

[Lu Baolu]

On 2020/6/2 13:45, Rajat Jain wrote:
> Currently, an external malicious PCI device can masquerade the VID:PID
> of faulty gfx devices, and thus apply iommu quirks to effectively
> disable the IOMMU restrictions for itself.
> 
> Thus we need to ensure that the device we are applying quirks to, is
> indeed an internal trusted device.
> 

Acked-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
>   drivers/iommu/intel-iommu.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index ef0a5246700e5..f2a480168a02f 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -6214,6 +6214,11 @@ const struct iommu_ops intel_iommu_ops = {
>   
>   static void quirk_iommu_igfx(struct pci_dev *dev)
>   {
> +	if (dev->untrusted) {
> +		pci_warn(dev, "skipping iommu quirk for untrusted gfx dev\n");
> +		return;
> +	}
> +
>   	pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
>   	dmar_map_gfx = 0;
>   }
> @@ -6255,6 +6260,11 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x163D, quirk_iommu_igfx);
>   
>   static void quirk_iommu_rwbf(struct pci_dev *dev)
>   {
> +	if (dev->untrusted) {
> +		pci_warn(dev, "skipping iommu quirk for untrusted dev\n");
> +		return;
> +	}
> +
>   	/*
>   	 * Mobile 4 Series Chipset neglects to set RWBF capability,
>   	 * but needs it. Same seems to hold for the desktop versions.
> @@ -6285,6 +6295,11 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
>   {
>   	unsigned short ggc;
>   
> +	if (dev->untrusted) {
> +		pci_warn(dev, "skipping iommu quirk for untrusted gfx dev\n");
> +		return;
> +	}
> +
>   	if (pci_read_config_word(dev, GGC, &ggc))
>   		return;
>   
> @@ -6318,6 +6333,13 @@ static void __init check_tylersburg_isoch(void)
>   	pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x3a3e, NULL);
>   	if (!pdev)
>   		return;
> +
> +	if (pdev->untrusted) {
> +		pci_warn(pdev, "skipping iommu quirk due to untrusted dev\n");
> +		pci_dev_put(pdev);
> +		return;
> +	}
> +
>   	pci_dev_put(pdev);
>   
>   	/* System Management Registers. Might be hidden, in which case
> @@ -6327,6 +6349,12 @@ static void __init check_tylersburg_isoch(void)
>   	if (!pdev)
>   		return;
>   
> +	if (pdev->untrusted) {
> +		pci_warn(pdev, "skipping iommu quirk due to untrusted dev\n");
> +		pci_dev_put(pdev);
> +		return;
> +	}
> +
>   	if (pci_read_config_dword(pdev, 0x188, &vtisochctrl)) {
>   		pci_dev_put(pdev);
>   		return;
> 

Re: [PATCH] iommu/vt-d: Don't apply gfx quirks to untrusted devices

[Mika Westerberg]

On Mon, Jun 01, 2020 at 10:45:17PM -0700, Rajat Jain wrote:
> Currently, an external malicious PCI device can masquerade the VID:PID
> of faulty gfx devices, and thus apply iommu quirks to effectively
> disable the IOMMU restrictions for itself.
> 
> Thus we need to ensure that the device we are applying quirks to, is
> indeed an internal trusted device.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
>  drivers/iommu/intel-iommu.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index ef0a5246700e5..f2a480168a02f 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -6214,6 +6214,11 @@ const struct iommu_ops intel_iommu_ops = {
>  
>  static void quirk_iommu_igfx(struct pci_dev *dev)
>  {
> +	if (dev->untrusted) {
> +		pci_warn(dev, "skipping iommu quirk for untrusted gfx dev\n");

I think you should be consistent with other messages. For example iommu
should be spelled IOMMU as done below.

Also this is visible to users so maybe put bit more information there:

  pci_warn(dev, "Will not apply IOMMU quirk for untrusted graphics device\n");

Ditto for all the other places. Also is "untrusted" good word here? If
an ordinary user sees this will it trigger some sort of panic reaction.
Perhaps we should call it "potentially untrusted" or something like
that?

> +		return;
> +	}
> +
>  	pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
>  	dmar_map_gfx = 0;

Re: [PATCH] iommu/vt-d: Don't apply gfx quirks to untrusted devices

[Rajat Jain]

Hi MIka,

Thanks for taking a look.

On Tue, Jun 2, 2020 at 2:50 AM Mika Westerberg
<mika.westerberg@intel.com> wrote:
>
> On Mon, Jun 01, 2020 at 10:45:17PM -0700, Rajat Jain wrote:
> > Currently, an external malicious PCI device can masquerade the VID:PID
> > of faulty gfx devices, and thus apply iommu quirks to effectively
> > disable the IOMMU restrictions for itself.
> >
> > Thus we need to ensure that the device we are applying quirks to, is
> > indeed an internal trusted device.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> >  drivers/iommu/intel-iommu.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index ef0a5246700e5..f2a480168a02f 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -6214,6 +6214,11 @@ const struct iommu_ops intel_iommu_ops = {
> >
> >  static void quirk_iommu_igfx(struct pci_dev *dev)
> >  {
> > +     if (dev->untrusted) {
> > +             pci_warn(dev, "skipping iommu quirk for untrusted gfx dev\n");
>
> I think you should be consistent with other messages. For example iommu
> should be spelled IOMMU as done below.
>
> Also this is visible to users so maybe put bit more information there:
>
>   pci_warn(dev, "Will not apply IOMMU quirk for untrusted graphics device\n");
>
> Ditto for all the other places. Also is "untrusted" good word here? If
> an ordinary user sees this will it trigger some sort of panic reaction.
> Perhaps we should call it "potentially untrusted" or something like
> that?

Fixed it, posted new patch at
https://lkml.org/lkml/2020/6/2/822

Thanks,

Rajat

>
> > +             return;
> > +     }
> > +
> >       pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
> >       dmar_map_gfx = 0;

Re: [PATCH] iommu/vt-d: Don't apply gfx quirks to untrusted devices

[Raj, Ashok]

On Tue, Jun 02, 2020 at 06:43:00PM +0000, Rajat Jain wrote:
> Hi MIka,
> 
> Thanks for taking a look.
> 
> On Tue, Jun 2, 2020 at 2:50 AM Mika Westerberg
> <mika.westerberg@intel.com> wrote:
> >
> > On Mon, Jun 01, 2020 at 10:45:17PM -0700, Rajat Jain wrote:
> > > Currently, an external malicious PCI device can masquerade the VID:PID
> > > of faulty gfx devices, and thus apply iommu quirks to effectively
> > > disable the IOMMU restrictions for itself.
> > >
> > > Thus we need to ensure that the device we are applying quirks to, is
> > > indeed an internal trusted device.
> > >
> > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > ---
> > >  drivers/iommu/intel-iommu.c | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > > index ef0a5246700e5..f2a480168a02f 100644
> > > --- a/drivers/iommu/intel-iommu.c
> > > +++ b/drivers/iommu/intel-iommu.c
> > > @@ -6214,6 +6214,11 @@ const struct iommu_ops intel_iommu_ops = {
> > >
> > >  static void quirk_iommu_igfx(struct pci_dev *dev)
> > >  {
> > > +     if (dev->untrusted) {
> > > +             pci_warn(dev, "skipping iommu quirk for untrusted gfx dev\n");
> >
> > I think you should be consistent with other messages. For example iommu
> > should be spelled IOMMU as done below.
> >
> > Also this is visible to users so maybe put bit more information there:
> >
> >   pci_warn(dev, "Will not apply IOMMU quirk for untrusted graphics device\n");
> >
> > Ditto for all the other places. Also is "untrusted" good word here? If
> > an ordinary user sees this will it trigger some sort of panic reaction.
> > Perhaps we should call it "potentially untrusted" or something like
> > that?

Wish we called it external_facing rather than untrusted attribute, so its
description is consistent with the spec that defines it. Once we have
Platform Component Security Enhancements. 

to be correct, maybe call "Device located on an untrusted link" rather than
assert blame on the device.

Since the information is harvsted from BIOS tables and there are chances
this could be wrongly grouped such, add "Check with your BIOS/Platform
Vendor.



> 
> Fixed it, posted new patch at
> https://lkml.org/lkml/2020/6/2/822
> 
> Thanks,
> 
> Rajat
> 
> >
> > > +             return;
> > > +     }
> > > +
> > >       pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
> > >       dmar_map_gfx = 0;