[PATCH v5 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF

Shameerali Kolothum Thodi shameerali.kolothum.thodi at huawei.com
Wed Apr 17 08:12:57 PDT 2024



> -----Original Message-----
> From: Nicolin Chen <nicolinc at nvidia.com>
> Sent: Saturday, April 13, 2024 4:44 AM
> To: will at kernel.org; robin.murphy at arm.com
> Cc: joro at 8bytes.org; jgg at nvidia.com; thierry.reding at gmail.com;
> vdumpa at nvidia.com; jonathanh at nvidia.com; linux-kernel at vger.kernel.org;
> iommu at lists.linux.dev; linux-arm-kernel at lists.infradead.org; linux-
> tegra at vger.kernel.org
> Subject: [PATCH v5 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned
> VINTF
> 
> When VCMDQs are assigned to a VINTF owned by a guest (HYP_OWN bit unset),
> only TLB and ATC invalidation commands are supported by the VCMDQ HW. So,
> add a new helper to scan the input cmds to make sure every single command
> is supported when selecting a queue.
> 
> Note that the guest VM shouldn't have HYP_OWN bit being set regardless of
> guest kernel driver writing it or not, i.e. the hypervisor running in the
> host OS should wire this bit to zero when trapping a write access to this
> VINTF_CONFIG register from a guest kernel.

Just curious, suppose there is a malicious guest with a compromised kernel 
which bypasses the HYP_OWN bit check and issues other commands, does
the hardware has the capability to detect it and not make the host unstable
in any way?

Thanks,
Shameer

> 
> Signed-off-by: Nicolin Chen <nicolinc at nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  7 +--
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  5 ++-
>  .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 43 ++++++++++++++++++-
>  3 files changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index ba7a933c1efb..9af6659ea488 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -352,10 +352,11 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd,
> struct arm_smmu_cmdq_ent *ent)
>  	return 0;
>  }
> 
> -static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct
> arm_smmu_device *smmu)
> +static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct
> arm_smmu_device *smmu,
> +					       u64 *cmds, int n)
>  {
>  	if (smmu->tegra241_cmdqv)
> -		return tegra241_cmdqv_get_cmdq(smmu);
> +		return tegra241_cmdqv_get_cmdq(smmu, cmds, n);
> 
>  	return &smmu->cmdq;
>  }
> @@ -765,7 +766,7 @@ static int arm_smmu_cmdq_issue_cmdlist(struct
> arm_smmu_device *smmu,
>  	u32 prod;
>  	unsigned long flags;
>  	bool owner;
> -	struct arm_smmu_cmdq *cmdq = arm_smmu_get_cmdq(smmu);
> +	struct arm_smmu_cmdq *cmdq = arm_smmu_get_cmdq(smmu, cmds,
> n);
>  	struct arm_smmu_ll_queue llq, head;
>  	int ret = 0;
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 5b8e463c28eb..fdc3d570cf43 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -836,7 +836,8 @@ static inline void
> arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
>  struct tegra241_cmdqv *
>  tegra241_cmdqv_acpi_probe(struct arm_smmu_device *smmu, int id);
>  int tegra241_cmdqv_device_reset(struct arm_smmu_device *smmu);
> -struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct
> arm_smmu_device *smmu);
> +struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct
> arm_smmu_device *smmu,
> +					      u64 *cmds, int n);
>  #else /* CONFIG_TEGRA241_CMDQV */
>  static inline struct tegra241_cmdqv *
>  tegra241_cmdqv_acpi_probe(struct arm_smmu_device *smmu, int id)
> @@ -850,7 +851,7 @@ static inline int tegra241_cmdqv_device_reset(struct
> arm_smmu_device *smmu)
>  }
> 
>  static inline struct arm_smmu_cmdq *
> -tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
> +tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu, u64 *cmds, int
> n)
>  {
>  	return NULL;
>  }
> diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> index 15683123a4ce..7aeaf810980c 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> @@ -262,6 +262,7 @@ struct tegra241_vcmdq {
>   * struct tegra241_vintf - Virtual Interface
>   * @idx: Global index in the CMDQV HW
>   * @enabled: Enabled or not
> + * @hyp_own: Owned by hypervisor (in-kernel)
>   * @error: Status error or not
>   * @cmdqv: CMDQV HW pointer
>   * @vcmdqs: List of VCMDQ pointers
> @@ -271,6 +272,7 @@ struct tegra241_vintf {
>  	u16 idx;
> 
>  	bool enabled;
> +	bool hyp_own;
>  	atomic_t error;	/* Race between interrupts and get_cmdq() */
> 
>  	struct tegra241_cmdqv *cmdqv;
> @@ -369,7 +371,32 @@ static irqreturn_t tegra241_cmdqv_isr(int irq, void
> *devid)
>  	return IRQ_HANDLED;
>  }
> 
> -struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct
> arm_smmu_device *smmu)
> +static bool tegra241_vintf_support_cmds(struct tegra241_vintf *vintf,
> +					u64 *cmds, int n)
> +{
> +	int i;
> +
> +	/* VINTF owned by hypervisor can execute any command */
> +	if (vintf->hyp_own)
> +		return true;
> +
> +	/* Guest-owned VINTF must Check against the list of supported CMDs
> */
> +	for (i = 0; i < n; i++) {
> +		switch (FIELD_GET(CMDQ_0_OP, cmds[i *
> CMDQ_ENT_DWORDS])) {
> +		case CMDQ_OP_TLBI_NH_ASID:
> +		case CMDQ_OP_TLBI_NH_VA:
> +		case CMDQ_OP_ATC_INV:
> +			continue;
> +		default:
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct
> arm_smmu_device *smmu,
> +					      u64 *cmds, int n)
>  {
>  	struct tegra241_cmdqv *cmdqv = smmu->tegra241_cmdqv;
>  	struct tegra241_vintf *vintf = cmdqv->vintfs[0];
> @@ -386,6 +413,10 @@ struct arm_smmu_cmdq
> *tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
>  	if (atomic_read(&vintf->error))
>  		return &smmu->cmdq;
> 
> +	/* Unsupported CMDs go for smmu->cmdq pathway */
> +	if (!tegra241_vintf_support_cmds(vintf, cmds, n))
> +		return &smmu->cmdq;
> +
>  	/*
>  	 * Select a vcmdq to use. Here we use a temporal solution to
>  	 * balance out traffic on cmdq issuing: each cmdq has its own
> @@ -575,6 +606,11 @@ int tegra241_cmdqv_device_reset(struct
> arm_smmu_device *smmu)
>  	if (ret)
>  		return ret;
> 
> +	/*
> +	 * Note that HYP_OWN bit is wired to zero when running in guest kernel
> +	 * regardless of enabling it here, as !HYP_OWN cmdqs have a restricted
> +	 * set of supported commands, by following the HW design.
> +	 */
>  	regval = FIELD_PREP(VINTF_HYP_OWN, 1);
>  	vintf_writel(regval, CONFIG);
> 
> @@ -582,6 +618,11 @@ int tegra241_cmdqv_device_reset(struct
> arm_smmu_device *smmu)
>  	if (ret)
>  		return ret;
> 
> +	/*
> +	 * As being mentioned above, HYP_OWN bit is wired to zero for a guest
> +	 * kernel, so read it back from HW to ensure that reflects in hyp_own
> +	 */
> +	vintf->hyp_own = !!(VINTF_HYP_OWN & vintf_readl(CONFIG));
>  	vintf->enabled = !!(VINTF_ENABLED & vintf_readl(STATUS));
>  	atomic_set(&vintf->error,
>  		   !!FIELD_GET(VINTF_STATUS, vintf_readl(STATUS)));
> --
> 2.43.0
> 




More information about the linux-arm-kernel mailing list