linux-kernel - Re: [PATCH v2 2/5] iommu/amd: Introduce rw_semaphore for Device Table Entry (DTE)

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: <20240829193402.GK3773488@nvidia.com>
Date: Thu, 29 Aug 2024 16:34:02 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
Cc: linux-kernel@...r.kernel.org, iommu@...ts.linux.dev, joro@...tes.org,
	robin.murphy@....com, vasant.hegde@....com, ubizjak@...il.com,
	jon.grimm@....com, santosh.shukla@....com, pandoh@...gle.com,
	kumaranand@...gle.com
Subject: Re: [PATCH v2 2/5] iommu/amd: Introduce rw_semaphore for Device
 Table Entry (DTE)

On Thu, Aug 29, 2024 at 06:07:23PM +0000, Suravee Suthikulpanit wrote:

> -static void clear_dte_entry(struct amd_iommu *iommu, u16 devid)
> +static void clear_dte_entry(struct amd_iommu *iommu, struct iommu_dev_data *dev_data)
>  {
> +	u16 devid = dev_data->devid;
>  	struct dev_table_entry *dev_table = get_dev_table(iommu);
>  
> +	down_write(&dev_data->dte_sem);
> +
>  	/* remove entry from the device table seen by the hardware */
>  	dev_table[devid].data[0]  = DTE_FLAG_V;

The rest of this function doesn't seem very good either:

	/* remove entry from the device table seen by the hardware */
	dev_table[devid].data[0]  = DTE_FLAG_V;

	if (!amd_iommu_snp_en)
		dev_table[devid].data[0] |= DTE_FLAG_TV;

	dev_table[devid].data[1] &= DTE_FLAG_MASK;

This should also cmpxchg the whole 128 bit quantity, you don't want
the HW to see a tear on HostPageTableRootPtr.

Which is back to my other point. Don't edit the DTE in place.

There is no such thing as 'clear' in the iommu domain API. The DTE is
either PAGING, BLOCKED or IDENTITY, and any write to it should be
writing an entire DTE for that target.

I guess clear is actually trying to set the DTE to BLOCKING?

Also no need to get the lock here because you don't touch 128 bit
quanta [1] which holds the IRQ stuff that is racey. This is already
locked by the iommu core group lock.

Jason