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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ