Re: [PATCH v4 2/6] iommu/arm-smmu-v3-iommufd: Reject unsupported bits in invalidation commands

[Pranjal Shrivastava <praan@google.com>]

On Tue, Jul 14, 2026 at 11:48:48AM -0700, Nicolin Chen wrote:
> The arm_vsmmu_cache_invalidate() op hands a guest's invalidation commands
> to the trusted main command queue after enforcing only the VMID or the SID,
> and passes the rest of the command through to the queue unchanged.
> 

[...]

> +
> +static int arm_vsmmu_validate_user_cmd(struct arm_vsmmu *vsmmu, u64 data[2])
> +{
> +	struct arm_smmu_device *smmu = vsmmu->smmu;
> +	u64 allowed[2] = { CMDQ_0_OP };
> +
> +	/* Collect the fields userspace is allowed to set for each opcode */
> +	switch (data[0] & CMDQ_0_OP) {
> +	case CMDQ_OP_TLBI_NH_VA:
> +		/* An SMMU with 8-bit ASIDs treats the upper 8 bits as RES0 */
> +		allowed[0] |= FIELD_PREP(CMDQ_TLBI_0_ASID,
> +					 GENMASK(smmu->asid_bits - 1, 0));
> +		fallthrough;
> +	case CMDQ_OP_TLBI_NH_VAA:
> +		/* NUM/SCALE/TG/TTL are range fields gated on FEAT_RANGE_INV */
> +		if (smmu->features & ARM_SMMU_FEAT_RANGE_INV) {
> +			if (arm_vsmmu_validate_range(smmu, data))
> +				return -EIO;
> +			allowed[0] |= CMDQ_TLBI_0_NUM | CMDQ_TLBI_0_SCALE;
> +			allowed[1] |= CMDQ_TLBI_1_TG | CMDQ_TLBI_1_TTL;
> +			/* SCALE bit 25 (values above 31) is RES0 without DS */
> +			if (!(smmu->features & ARM_SMMU_FEAT_DS))
> +				allowed[0] &= ~FIELD_PREP(CMDQ_TLBI_0_SCALE,
> +							  BIT(5));
> +		}
> +		allowed[0] |= CMDQ_TLBI_0_VMID;
> +		allowed[1] |= CMDQ_TLBI_1_LEAF | CMDQ_TLBI_1_VA_MASK;
> +		break;
> +	case CMDQ_OP_TLBI_NH_ASID:
> +		/* An SMMU with 8-bit ASIDs treats the upper 8 bits as RES0 */
> +		allowed[0] |= FIELD_PREP(CMDQ_TLBI_0_ASID,
> +					 GENMASK(smmu->asid_bits - 1, 0));
> +		fallthrough;
> +	case CMDQ_OP_TLBI_NH_ALL:
> +		allowed[0] |= CMDQ_TLBI_0_VMID;
> +		break;
> +	case CMDQ_OP_ATC_INV:
> +		/* ATC_INV is illegal unless the SMMU implements ATS */
> +		if (!(smmu->features & ARM_SMMU_FEAT_ATS))
> +			return -EIO;
> +		/* A Size above 52 (invalidate-all) may raise a CERROR_ILL */
> +		if (FIELD_GET(CMDQ_ATC_1_SIZE, data[1]) > ATC_INV_SIZE_ALL)
> +			return -EIO;
> +		/*
> +		 * SSV/SSID/Global need substream support. SSID and Global are
> +		 * IGNORED (not RES0) when SSV == 0, so they need no SSV check.
> +		 */
> +		if (smmu->ssid_bits)
> +			allowed[0] |= CMDQ_0_SSV | CMDQ_ATC_0_SSID |
> +				      CMDQ_ATC_0_GLOBAL;
> +		allowed[0] |= CMDQ_ATC_0_SID;
> +		allowed[1] |= CMDQ_ATC_1_SIZE | CMDQ_ATC_1_ADDR_MASK;
> +		break;
> +	case CMDQ_OP_CFGI_CD:
> +		/* No SSV for CFGI_CD; SSID requires substream support */
> +		if (smmu->ssid_bits)
> +			allowed[0] |= CMDQ_CFGI_0_SSID;
> +		allowed[1] |= CMDQ_CFGI_1_LEAF;
> +		fallthrough;
> +	case CMDQ_OP_CFGI_CD_ALL:
> +		allowed[0] |= CMDQ_CFGI_0_SID;
> +		break;

Nit: Although, the convert_user_cmd would check the CMDQ_OP again, it
seems like validate_user_cmd would return 0 for an unsupported cmd that
doesn't match any of the above cases, should we add a default case and
return -EIO there?

> +	}
> +
> +	/*
> +	 * Reject any other bit, e.g. a RES0 bit or a Secure bit, before the
> +	 * command reaches the trusted main cmdq, so a guest cannot wedge the
> +	 * shared queue for every device with a CERROR_ILL.
> +	 *
> +	 * By contrast, an out-of-range address or ID value does not need a
> +	 * check: the spec defines it as CONSTRAINED UNPREDICTABLE, which is
> +	 * scoped to the guest itself and does not raise a CERROR_ILL.
> +	 */
> +	if ((data[0] & ~allowed[0]) || (data[1] & ~allowed[1]))
> +		return -EIO;
> +	return 0;
> +}
> +

Apart from that,
Reviewed-by: Pranjal Shrivastava <praan@google.com>

Thanks,
Praan