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

From: Jason Gunthorpe
Date: Fri Sep 06 2024 - 14:11:05 EST


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