linux-kernel - Re: [PATCH v5 4/8] iommu/amd: Add function copy_dev

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]

Message-ID: <20160920115804.GC3541@8bytes.org>
Date:   Tue, 20 Sep 2016 13:58:04 +0200
From:   Joerg Roedel <joro@...tes.org>
To:     Baoquan He <bhe@...hat.com>
Cc:     iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        kexec@...ts.infradead.org, dyoung@...hat.com, xlpang@...hat.com,
        Vincent.Wan@....com
Subject: Re: [PATCH v5 4/8] iommu/amd: Add function copy_dev_tables

Hi Baoquan,

On Thu, Sep 15, 2016 at 11:03:22PM +0800, Baoquan He wrote:
> +static int copy_dev_tables(void)
> +{
> +	u64 entry;
> +	u32 lo, hi, devid;
> +	phys_addr_t old_devtb_phys;
> +	struct dev_table_entry *old_devtb = NULL;
> +	u16 dom_id, dte_v;
> +	struct amd_iommu *iommu;
> +	static int copied;

Please order this by line-length, longer lines first.

> +        for_each_iommu(iommu) {
> +		if (!translation_pre_enabled(iommu)) {
> +			pr_err("IOMMU:%d is not pre-enabled!/n", iommu->index);
> +			return -1;
> +		}
> +
> +		if (copied)
> +			continue;
> +
> +                lo = readl(iommu->mmio_base + MMIO_DEV_TABLE_OFFSET);
> +                hi = readl(iommu->mmio_base + MMIO_DEV_TABLE_OFFSET + 4);
> +                entry = (((u64) hi) << 32) + lo;
> +                old_devtb_phys = entry & PAGE_MASK;
> +                old_devtb = memremap(old_devtb_phys, dev_table_size, MEMREMAP_WB);
> +		if (!old_devtb)
> +			return -1;
> +                for (devid = 0; devid <= amd_iommu_last_bdf; ++devid) {
> +                        amd_iommu_dev_table[devid] = old_devtb[devid];
> +                        dom_id = amd_iommu_dev_table[devid].data[1] & DEV_DOMID_MASK;
> +			dte_v = amd_iommu_dev_table[devid].data[0] & DTE_FLAG_V;
> +			if (!dte_v || !dom_id)
> +				continue;
> +                        __set_bit(dom_id, amd_iommu_pd_alloc_bitmap);
> +                }
> +		memunmap(old_devtb);
> +		copied = 1;
> +        }

This loop need more refinement and sanity checking code. I suggest using
two loops and do the sanity checking in the first one. The sanity checks
should do:

	* Check whether all IOMMUs actually use the same device table
	  with the same size

	* Verify that the size of the old device table is the expected
	  size.

	* Also sanity check the irq-remapping information and remapping
	  table sizes.

If any of these checks fail, just bail out of copying.

What is further needed it some more selection on what is copied from the
old kernel. There is no need to copy all the GCR3 root-pointer
information. If a device is set up with guest translations (DTE.GV=1),
then don't copy that information but move the device over to an empty
guest-cr3 table and handle the faults in the PPR log (which should just
answer them with INVALID). After all these PPR faults are recoverable
for the device and we should not allow the device to change old-kernels
data when we don't have to.


	Joerg