> -----Original Message-----
> From: Nicolin Chen <nicolinc@xxxxxxxxxx>
> Sent: Saturday, April 13, 2024 4:44 AM
> To: will@xxxxxxxxxx; robin.murphy@xxxxxxx
> Cc: joro@xxxxxxxxxx; jgg@xxxxxxxxxx; thierry.reding@xxxxxxxxx;
> vdumpa@xxxxxxxxxx; jonathanh@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> iommu@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> tegra@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> ---
> 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
>
