Re: [PATCH v3 4/5] iommu/amd: Modify clear_dte_entry() to avoid in-place update

[Jason Gunthorpe]

On Fri, Sep 06, 2024 at 12:13:07PM +0000, Suravee Suthikulpanit wrote:
> Lock DTE and copy value to a temporary storage before update using
> cmpxchg128.
> 
> Also, refactor the function to simplify logic for applying erratum 63.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |  2 ++
>  drivers/iommu/amd/iommu.c           | 27 ++++++++++++++++++++-------
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index 1836da2d9e60..81a994471a30 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -425,6 +425,8 @@
>  
>  #define DTE_GPT_LEVEL_SHIFT	54
>  
> +#define DTE_SYSMGT_MASK		GENMASK_ULL(41, 40)
> +
>  #define GCR3_VALID		0x01ULL
>  
>  #define IOMMU_PAGE_MASK (((1ULL << 52) - 1) & ~0xfffULL)
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index f18ae6c077f4..15eb816d4313 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2025,19 +2025,32 @@ static void set_dte_entry(struct amd_iommu *iommu,
>  	}
>  }
>  
> -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)
>  {
> -	struct dev_table_entry *dev_table = get_dev_table(iommu);
> +	struct dev_table_entry new;
> +	struct dev_table_entry *dte = &get_dev_table(iommu)[dev_data->devid];
> +
> +	/*
> +	 * Need to preserve DTE[96:106] because certain fields are
> +	 * programmed using value in IVRS table from early init phase.
> +	 */
> +	spin_lock(&dev_data->dte_lock);
> +	get_dte256(iommu, dev_data, &new);

I think there is no point in the get?

	struct dev_table_entry new = {}

	new.data[0] = DTE_FLAG_V;
	if (FIELD_GET(DTE_SYSMGT_MASK, old.data[1]) == 0x01)
		new.data[0] |= BIT_ULL(DEV_ENTRY_IW);

	new.data[1] = old.data[1] & DTE_FLAG_MASK
	new.data[2..4] = 0

That is pretty clear and simple

> -	dev_table[devid].data[1] &= DTE_FLAG_MASK;
> +	new.data[1] &= DTE_FLAG_MASK;

Would be nice if DTE_FLAG_MASK was broken into fields someday..

> -	amd_iommu_apply_erratum_63(iommu, devid);
> +	/* Apply erratum 63 */
> +	if (FIELD_GET(DTE_SYSMGT_MASK, new.data[1]) == 0x01)
> +		new.data[0] |= BIT_ULL(DEV_ENTRY_IW);
> +
> +	WARN_ON(!try_cmpxchg128(&dte->data128[0], &dte->data128[0], new.data128[0]));

As before this has to move forward, we can't fail to clear the DTE, it
will open a UAF

This should also clear the top 128 bits, so I would call the
update_dte256 directly?

Jason