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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 2 Jun 2020 16:49:12 -0700
From:   Prashant Malani <pmalani@...gle.com>
To:     Rajat Jain <rajatja@...gle.com>
Cc:     David Woodhouse <dwmw2@...radead.org>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        Joerg Roedel <joro@...tes.org>,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        Mika Westerberg <mika.westerberg@...el.com>,
        Ashok Raj <ashok.raj@...el.com>,
        lalithambika.krishnakumar@...el.com, rajatxjain@...il.com,
        bleung@...gle.com, levinale@...gle.com, zsm@...gle.com,
        mnissler@...gle.com, tbroch@...gle.com
Subject: Re: [PATCH v3] iommu/vt-d: Don't apply gfx quirks to untrusted
 devices

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 <rajatja@...gle.com>
> Acked-by: Lu Baolu <baolu.lu@...ux.intel.com>
> ---
> 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
>