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 <[email protected]>
Acked-by: Lu Baolu <[email protected]>
---
v3: - Separate out the warning mesage in a function to be called from
other places. Change the warning string as suggested.
v2: - Change the warning print strings.
- Add Lu Baolu's acknowledgement.
drivers/iommu/intel-iommu.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index ef0a5246700e5..dc859f02985a0 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -6185,6 +6185,23 @@ intel_iommu_domain_set_attr(struct iommu_domain *domain,
return ret;
}
+/*
+ * Check that the device does not live on an external facing PCI port that is
+ * marked as untrusted. Such devices should not be able to apply quirks and
+ * thus not be able to bypass the IOMMU restrictions.
+ */
+static bool risky_device(struct pci_dev *pdev)
+{
+ if (pdev->untrusted) {
+ pci_warn(pdev,
+ "Skipping IOMMU quirk for dev (%04X:%04X) on untrusted"
+ " PCI link. Please check with your BIOS/Platform"
+ " vendor about this\n", pdev->vendor, pdev->device);
+ return true;
+ }
+ return false;
+}
+
const struct iommu_ops intel_iommu_ops = {
.capable = intel_iommu_capable,
.domain_alloc = intel_iommu_domain_alloc,
@@ -6214,6 +6231,9 @@ const struct iommu_ops intel_iommu_ops = {
static void quirk_iommu_igfx(struct pci_dev *dev)
{
+ if (risky_device(dev))
+ return;
+
pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
dmar_map_gfx = 0;
}
@@ -6255,6 +6275,9 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x163D, quirk_iommu_igfx);
static void quirk_iommu_rwbf(struct pci_dev *dev)
{
+ if (risky_device(dev))
+ return;
+
/*
* Mobile 4 Series Chipset neglects to set RWBF capability,
* but needs it. Same seems to hold for the desktop versions.
@@ -6285,6 +6308,9 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
{
unsigned short ggc;
+ if (risky_device(dev))
+ return;
+
if (pci_read_config_word(dev, GGC, &ggc))
return;
@@ -6318,6 +6344,12 @@ static void __init check_tylersburg_isoch(void)
pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x3a3e, NULL);
if (!pdev)
return;
+
+ if (risky_device(pdev)) {
+ pci_dev_put(pdev);
+ return;
+ }
+
pci_dev_put(pdev);
/* System Management Registers. Might be hidden, in which case
@@ -6327,6 +6359,11 @@ static void __init check_tylersburg_isoch(void)
if (!pdev)
return;
+ if (risky_device(pdev)) {
+ 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
On Tue, Jun 02, 2020 at 04:26:02PM -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 <[email protected]>
> Acked-by: Lu Baolu <[email protected]>
With these changes
Reviewed-by: Ashok Raj <[email protected]>
> ---
> v3: - Separate out the warning mesage in a function to be called from
> other places. Change the warning string as suggested.
> v2: - Change the warning print strings.
> - Add Lu Baolu's acknowledgement.
>
> drivers/iommu/intel-iommu.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index ef0a5246700e5..dc859f02985a0 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -6185,6 +6185,23 @@ intel_iommu_domain_set_attr(struct iommu_domain *domain,
> return ret;
> }
>
> +/*
> + * Check that the device does not live on an external facing PCI port that is
> + * marked as untrusted. Such devices should not be able to apply quirks and
> + * thus not be able to bypass the IOMMU restrictions.
> + */
> +static bool risky_device(struct pci_dev *pdev)
> +{
> + if (pdev->untrusted) {
> + pci_warn(pdev,
> + "Skipping IOMMU quirk for dev (%04X:%04X) on untrusted"
> + " PCI link. Please check with your BIOS/Platform"
> + " vendor about this\n", pdev->vendor, pdev->device);
> + return true;
> + }
> + return false;
> +}
> +
> const struct iommu_ops intel_iommu_ops = {
> .capable = intel_iommu_capable,
> .domain_alloc = intel_iommu_domain_alloc,
> @@ -6214,6 +6231,9 @@ const struct iommu_ops intel_iommu_ops = {
>
> static void quirk_iommu_igfx(struct pci_dev *dev)
> {
> + if (risky_device(dev))
> + return;
> +
> pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
> dmar_map_gfx = 0;
> }
> @@ -6255,6 +6275,9 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x163D, quirk_iommu_igfx);
>
> static void quirk_iommu_rwbf(struct pci_dev *dev)
> {
> + if (risky_device(dev))
> + return;
> +
> /*
> * Mobile 4 Series Chipset neglects to set RWBF capability,
> * but needs it. Same seems to hold for the desktop versions.
> @@ -6285,6 +6308,9 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
> {
> unsigned short ggc;
>
> + if (risky_device(dev))
> + return;
> +
> if (pci_read_config_word(dev, GGC, &ggc))
> return;
>
> @@ -6318,6 +6344,12 @@ static void __init check_tylersburg_isoch(void)
> pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x3a3e, NULL);
> if (!pdev)
> return;
> +
> + if (risky_device(pdev)) {
> + pci_dev_put(pdev);
> + return;
> + }
> +
> pci_dev_put(pdev);
>
> /* System Management Registers. Might be hidden, in which case
> @@ -6327,6 +6359,11 @@ static void __init check_tylersburg_isoch(void)
> if (!pdev)
> return;
>
> + if (risky_device(pdev)) {
> + 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
>
Hi Rajat,
On Tue, Jun 02, 2020 at 04:26:02PM -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 <[email protected]>
> Acked-by: Lu Baolu <[email protected]>
> ---
> v3: - Separate out the warning mesage in a function to be called from
> other places. Change the warning string as suggested.
> v2: - Change the warning print strings.
> - Add Lu Baolu's acknowledgement.
>
> drivers/iommu/intel-iommu.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index ef0a5246700e5..dc859f02985a0 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -6185,6 +6185,23 @@ intel_iommu_domain_set_attr(struct iommu_domain *domain,
> return ret;
> }
>
> +/*
> + * Check that the device does not live on an external facing PCI port that is
> + * marked as untrusted. Such devices should not be able to apply quirks and
> + * thus not be able to bypass the IOMMU restrictions.
> + */
> +static bool risky_device(struct pci_dev *pdev)
> +{
> + if (pdev->untrusted) {
> + pci_warn(pdev,
> + "Skipping IOMMU quirk for dev (%04X:%04X) on untrusted"
> + " PCI link. Please check with your BIOS/Platform"
> + " vendor about this\n", pdev->vendor, pdev->device);
> + return true;
> + }
> + return false;
minor suggestion: Perhaps you could use a guard clause here? It would save you
a level of indentation, and possibly allow better string splitting
(e.g keeping "untrusted PCI" together). So something like:
if (!pdev->untrusted)
return false;
pci_warn(...);
I also hear the column limit warning is now for 100 chars [1], though
I'm not sure how it's being handled in this file.
Best regards,
-Prashant
[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/process/coding-style.rst?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> +}
> +
> const struct iommu_ops intel_iommu_ops = {
> .capable = intel_iommu_capable,
> .domain_alloc = intel_iommu_domain_alloc,
> @@ -6214,6 +6231,9 @@ const struct iommu_ops intel_iommu_ops = {
>
> static void quirk_iommu_igfx(struct pci_dev *dev)
> {
> + if (risky_device(dev))
> + return;
> +
> pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
> dmar_map_gfx = 0;
> }
> @@ -6255,6 +6275,9 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x163D, quirk_iommu_igfx);
>
> static void quirk_iommu_rwbf(struct pci_dev *dev)
> {
> + if (risky_device(dev))
> + return;
> +
> /*
> * Mobile 4 Series Chipset neglects to set RWBF capability,
> * but needs it. Same seems to hold for the desktop versions.
> @@ -6285,6 +6308,9 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
> {
> unsigned short ggc;
>
> + if (risky_device(dev))
> + return;
> +
> if (pci_read_config_word(dev, GGC, &ggc))
> return;
>
> @@ -6318,6 +6344,12 @@ static void __init check_tylersburg_isoch(void)
> pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x3a3e, NULL);
> if (!pdev)
> return;
> +
> + if (risky_device(pdev)) {
> + pci_dev_put(pdev);
> + return;
> + }
> +
> pci_dev_put(pdev);
>
> /* System Management Registers. Might be hidden, in which case
> @@ -6327,6 +6359,11 @@ static void __init check_tylersburg_isoch(void)
> if (!pdev)
> return;
>
> + if (risky_device(pdev)) {
> + 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
>
On Tue, Jun 2, 2020 at 4:49 PM Prashant Malani <[email protected]> wrote:
>
> Hi Rajat,
Hi Prashant, thanks for taking a look.
>
> On Tue, Jun 02, 2020 at 04:26:02PM -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 <[email protected]>
> > Acked-by: Lu Baolu <[email protected]>
> > ---
> > v3: - Separate out the warning mesage in a function to be called from
> > other places. Change the warning string as suggested.
> > v2: - Change the warning print strings.
> > - Add Lu Baolu's acknowledgement.
> >
> > drivers/iommu/intel-iommu.c | 37 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index ef0a5246700e5..dc859f02985a0 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -6185,6 +6185,23 @@ intel_iommu_domain_set_attr(struct iommu_domain *domain,
> > return ret;
> > }
> >
> > +/*
> > + * Check that the device does not live on an external facing PCI port that is
> > + * marked as untrusted. Such devices should not be able to apply quirks and
> > + * thus not be able to bypass the IOMMU restrictions.
> > + */
> > +static bool risky_device(struct pci_dev *pdev)
> > +{
> > + if (pdev->untrusted) {
> > + pci_warn(pdev,
> > + "Skipping IOMMU quirk for dev (%04X:%04X) on untrusted"
> > + " PCI link. Please check with your BIOS/Platform"
> > + " vendor about this\n", pdev->vendor, pdev->device);
> > + return true;
> > + }
> > + return false;
> minor suggestion: Perhaps you could use a guard clause here? It would save you
> a level of indentation, and possibly allow better string splitting
> (e.g keeping "untrusted PCI" together). So something like:
>
> if (!pdev->untrusted)
> return false;
I personally have found double negation expressions always confusing,
even if negation is part of the variable. (For e.g. I have found I
need to be always stop and convince myself that:
"if (!pdev->untrusted)"
<do something>
conceptually implies
"if (pdev->trusted)".
<do something>
So I tend to keep negations to minimum. In this case, it doesn't buy
us much either, so I'd prefer to keep it the same unless there are
more opinions on this. OTOH I don't mind changing it too if you feel
strongly about this.
Thanks,
Rajat
>
> pci_warn(...);
>
> I also hear the column limit warning is now for 100 chars [1], though
> I'm not sure how it's being handled in this file.
>
> Best regards,
>
> -Prashant
>
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/process/coding-style.rst?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
>
> > +}
> > +
> > const struct iommu_ops intel_iommu_ops = {
> > .capable = intel_iommu_capable,
> > .domain_alloc = intel_iommu_domain_alloc,
> > @@ -6214,6 +6231,9 @@ const struct iommu_ops intel_iommu_ops = {
> >
> > static void quirk_iommu_igfx(struct pci_dev *dev)
> > {
> > + if (risky_device(dev))
> > + return;
> > +
> > pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
> > dmar_map_gfx = 0;
> > }
> > @@ -6255,6 +6275,9 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x163D, quirk_iommu_igfx);
> >
> > static void quirk_iommu_rwbf(struct pci_dev *dev)
> > {
> > + if (risky_device(dev))
> > + return;
> > +
> > /*
> > * Mobile 4 Series Chipset neglects to set RWBF capability,
> > * but needs it. Same seems to hold for the desktop versions.
> > @@ -6285,6 +6308,9 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
> > {
> > unsigned short ggc;
> >
> > + if (risky_device(dev))
> > + return;
> > +
> > if (pci_read_config_word(dev, GGC, &ggc))
> > return;
> >
> > @@ -6318,6 +6344,12 @@ static void __init check_tylersburg_isoch(void)
> > pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x3a3e, NULL);
> > if (!pdev)
> > return;
> > +
> > + if (risky_device(pdev)) {
> > + pci_dev_put(pdev);
> > + return;
> > + }
> > +
> > pci_dev_put(pdev);
> >
> > /* System Management Registers. Might be hidden, in which case
> > @@ -6327,6 +6359,11 @@ static void __init check_tylersburg_isoch(void)
> > if (!pdev)
> > return;
> >
> > + if (risky_device(pdev)) {
> > + 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
> >
(Trimming text)
On Wed, Jun 03, 2020 at 12:23:48AM +0000, Rajat Jain wrote:
> On Tue, Jun 2, 2020 at 4:49 PM Prashant Malani <[email protected]> wrote:
> >
> > Hi Rajat,
>
> Hi Prashant, thanks for taking a look.
>
> >
> > On Tue, Jun 02, 2020 at 04:26:02PM -0700, Rajat Jain wrote:
> > > +static bool risky_device(struct pci_dev *pdev)
> > > +{
> > > + if (pdev->untrusted) {
> > > + pci_warn(pdev,
> > > + "Skipping IOMMU quirk for dev (%04X:%04X) on untrusted"
> > > + " PCI link. Please check with your BIOS/Platform"
> > > + " vendor about this\n", pdev->vendor, pdev->device);
> > > + return true;
> > > + }
> > > + return false;
> > minor suggestion: Perhaps you could use a guard clause here? It would save you
> > a level of indentation, and possibly allow better string splitting
> > (e.g keeping "untrusted PCI" together). So something like:
> >
> > if (!pdev->untrusted)
> > return false;
>
> I personally have found double negation expressions always confusing,
> even if negation is part of the variable. (For e.g. I have found I
> need to be always stop and convince myself that:
>
> "if (!pdev->untrusted)"
> <do something>
>
> conceptually implies
>
> "if (pdev->trusted)".
> <do something>
>
> So I tend to keep negations to minimum. In this case, it doesn't buy
> us much either, so I'd prefer to keep it the same unless there are
> more opinions on this. OTOH I don't mind changing it too if you feel
> strongly about this.
Ordinarily, I'd agree with you regarding double-negatives.
However, in this case the condition phrasing is so brief ("not untrusted") that I'd
argue the indentation savings outweigh possible interpretation issues.
That said, I don't have a strong opinion here, so will defer to the maintainer's preference.
Best,
>
> Thanks,
>
> Rajat
>
>
> >
> > pci_warn(...);
> >
> > I also hear the column limit warning is now for 100 chars [1], though
> > I'm not sure how it's being handled in this file.
> >
> > Best regards,
> >
> > -Prashant
> >
> > [1]:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/process/coding-style.rst?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> >
> > > +}
> > > +
> > > const struct iommu_ops intel_iommu_ops = {
> > > .capable = intel_iommu_capable,
> > > .domain_alloc = intel_iommu_domain_alloc,
> > > @@ -6214,6 +6231,9 @@ const struct iommu_ops intel_iommu_ops = {
> > >
> > > static void quirk_iommu_igfx(struct pci_dev *dev)
> > > {
> > > + if (risky_device(dev))
> > > + return;
> > > +
> > > pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
> > > dmar_map_gfx = 0;
> > > }
> > > @@ -6255,6 +6275,9 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x163D, quirk_iommu_igfx);
> > >
> > > static void quirk_iommu_rwbf(struct pci_dev *dev)
> > > {
> > > + if (risky_device(dev))
> > > + return;
> > > +
> > > /*
> > > * Mobile 4 Series Chipset neglects to set RWBF capability,
> > > * but needs it. Same seems to hold for the desktop versions.
> > > @@ -6285,6 +6308,9 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
> > > {
> > > unsigned short ggc;
> > >
> > > + if (risky_device(dev))
> > > + return;
> > > +
> > > if (pci_read_config_word(dev, GGC, &ggc))
> > > return;
> > >
> > > @@ -6318,6 +6344,12 @@ static void __init check_tylersburg_isoch(void)
> > > pdev = pci_get_device(PCI_VENDOR_ID_INTEL, 0x3a3e, NULL);
> > > if (!pdev)
> > > return;
> > > +
> > > + if (risky_device(pdev)) {
> > > + pci_dev_put(pdev);
> > > + return;
> > > + }
> > > +
> > > pci_dev_put(pdev);
> > >
> > > /* System Management Registers. Might be hidden, in which case
> > > @@ -6327,6 +6359,11 @@ static void __init check_tylersburg_isoch(void)
> > > if (!pdev)
> > > return;
> > >
> > > + if (risky_device(pdev)) {
> > > + 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
> > >
On Tue, Jun 02, 2020 at 04:26:02PM -0700, Rajat Jain wrote:
> +static bool risky_device(struct pci_dev *pdev)
> +{
> + if (pdev->untrusted) {
> + pci_warn(pdev,
> + "Skipping IOMMU quirk for dev (%04X:%04X) on untrusted"
> + " PCI link. Please check with your BIOS/Platform"
> + " vendor about this\n", pdev->vendor, pdev->device);
You should not break user visible strings like this. It makes grepping
for them harder (see also CodingStyle). You can write it like this instead:
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");
Also I guess pci_info() might be better here after all. Your call :)
Rest of the patch looks good to me.
On Tue, Jun 2, 2020 at 10:30 PM Mika Westerberg
<[email protected]> wrote:
>
> On Tue, Jun 02, 2020 at 04:26:02PM -0700, Rajat Jain wrote:
> > +static bool risky_device(struct pci_dev *pdev)
> > +{
> > + if (pdev->untrusted) {
> > + pci_warn(pdev,
> > + "Skipping IOMMU quirk for dev (%04X:%04X) on untrusted"
> > + " PCI link. Please check with your BIOS/Platform"
> > + " vendor about this\n", pdev->vendor, pdev->device);
>
> You should not break user visible strings like this. It makes grepping
> for them harder (see also CodingStyle). You can write it like this instead:
>
> 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");
>
> Also I guess pci_info() might be better here after all. Your call :)
Done, sent the updated patch.
Thanks,
Rajat
>
> Rest of the patch looks good to me.