[PATCH v3] iommu/tegra241-cmdqv: Reject a VCMDQ base above the 48-bit hardware limit

[PATCH v3] iommu/tegra241-cmdqv: Reject a VCMDQ base above the 48-bit hardware limit

[Nicolin Chen]

tegra241_vcmdq_alloc_smmu_cmdq() allocates the VCMDQ buffer via the common
arm_smmu_init_one_queue(), which uses smmu->dev and its coherent DMA mask
of DMA_BIT_MASK(smmu->oas). An SMMUv3 queue base holds a 52-bit address in
Q_BASE_ADDR_MASK, but the VCMDQ_BASE register holds only 48 (VCMDQ_ADDR),
so the masked write "q_base = base_dma & VCMDQ_ADDR" silently drops bits 48
and up. The HW would then fetch its commands from a wrong memory location,
which likely raises VCMDQ errors.

Real hardware always pairs an SMMU OAS with a matching VCMDQ address field,
so this cannot happen; it takes a VMM that gives a guest a mismatched OAS.
Nor can the guest rely on the VMM to catch it: an SMMU CMDQ base keeps bits
48 and up in Q_BASE_ADDR_MASK, so a trapped CMDQ_BASE write still carries
them, while a VCMDQ base goes straight to the hardware and loses them.

Reject a base_dma with bits set outside VCMDQ_ADDR, the way the user-VCMDQ
path already does for its base_addr_pa, failing the queue init rather than
silently truncating the base. Use dev_warn_once() rather than WARN_ON(), as
the OAS is VMM-controlled and a WARN_ON() would let it panic a guest booted
with panic_on_warn.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---

Changelog
v3
 * Rebase on arm/smmu/updates branch
 * Update subject, commit message, and inline notes
v2
 https://lore.kernel.org/all/4e638ec1bbff46a358b4129e264fef66b4f37703.1784059577.git.nicolinc@nvidia.com/
 * No change
v1
 https://lore.kernel.org/all/0dc3a146a3d274d7c47e525385830343cea0f675.1783054570.git.nicolinc@nvidia.com/

 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
index 6644075c1431e..664aff9593de8 100644
--- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
+++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
@@ -672,6 +672,20 @@ static int tegra241_vcmdq_alloc_smmu_cmdq(struct tegra241_vcmdq *vcmdq)
 	if (ret)
 		return ret;
 
+	/*
+	 * The q->base_dma is bounded by DMA_BIT_MASK of SMMU's IDR5.OAS. On a
+	 * real hardware, VCMDQ_ADDR mask and IDR5.OAS are always aligned. But
+	 * a buggy VMM might give a guest a mismatched IDR5.OAS. Spit a warning
+	 * instead of a silent truncation that would make the HW fetch commands
+	 * from a wrong memory location.
+	 */
+	if (q->base_dma & ~VCMDQ_ADDR) {
+		dev_warn_once(
+			vcmdq->cmdqv->dev,
+			"VCMDQ base %pad exceeds the 48-bit VCMDQ_ADDR limit\n",
+			&q->base_dma);
+		return -EINVAL;
+	}
 	/* ...override q_base to write VCMDQ_BASE registers */
 	q->q_base = q->base_dma & VCMDQ_ADDR;
 	q->q_base |= FIELD_PREP(VCMDQ_LOG2SIZE, q->llq.max_n_shift);

base-commit: a86c36163c9722545d27e92bdad8418fdcaa8b1d
-- 
2.43.0

Re: [PATCH v3] iommu/tegra241-cmdqv: Reject a VCMDQ base above the 48-bit hardware limit

[Will Deacon]

On Wed, Jul 29, 2026 at 03:03:29PM -0700, Nicolin Chen wrote:
> tegra241_vcmdq_alloc_smmu_cmdq() allocates the VCMDQ buffer via the common
> arm_smmu_init_one_queue(), which uses smmu->dev and its coherent DMA mask
> of DMA_BIT_MASK(smmu->oas). An SMMUv3 queue base holds a 52-bit address in
> Q_BASE_ADDR_MASK, but the VCMDQ_BASE register holds only 48 (VCMDQ_ADDR),
> so the masked write "q_base = base_dma & VCMDQ_ADDR" silently drops bits 48
> and up. The HW would then fetch its commands from a wrong memory location,
> which likely raises VCMDQ errors.
> 
> Real hardware always pairs an SMMU OAS with a matching VCMDQ address field,
> so this cannot happen; it takes a VMM that gives a guest a mismatched OAS.
> Nor can the guest rely on the VMM to catch it: an SMMU CMDQ base keeps bits
> 48 and up in Q_BASE_ADDR_MASK, so a trapped CMDQ_BASE write still carries
> them, while a VCMDQ base goes straight to the hardware and loses them.
> 
> Reject a base_dma with bits set outside VCMDQ_ADDR, the way the user-VCMDQ
> path already does for its base_addr_pa, failing the queue init rather than
> silently truncating the base. Use dev_warn_once() rather than WARN_ON(), as
> the OAS is VMM-controlled and a WARN_ON() would let it panic a guest booted
> with panic_on_warn.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---

This still just sounds like something that a VMM shouldn't do?

Will

Re: [PATCH v3] iommu/tegra241-cmdqv: Reject a VCMDQ base above the 48-bit hardware limit

[Robin Murphy]

On 2026-08-06 1:47 pm, Will Deacon wrote:
> On Wed, Jul 29, 2026 at 03:03:29PM -0700, Nicolin Chen wrote:
>> tegra241_vcmdq_alloc_smmu_cmdq() allocates the VCMDQ buffer via the common
>> arm_smmu_init_one_queue(), which uses smmu->dev and its coherent DMA mask
>> of DMA_BIT_MASK(smmu->oas). An SMMUv3 queue base holds a 52-bit address in
>> Q_BASE_ADDR_MASK, but the VCMDQ_BASE register holds only 48 (VCMDQ_ADDR),
>> so the masked write "q_base = base_dma & VCMDQ_ADDR" silently drops bits 48
>> and up. The HW would then fetch its commands from a wrong memory location,
>> which likely raises VCMDQ errors.
>>
>> Real hardware always pairs an SMMU OAS with a matching VCMDQ address field,
>> so this cannot happen; it takes a VMM that gives a guest a mismatched OAS.
>> Nor can the guest rely on the VMM to catch it: an SMMU CMDQ base keeps bits
>> 48 and up in Q_BASE_ADDR_MASK, so a trapped CMDQ_BASE write still carries
>> them, while a VCMDQ base goes straight to the hardware and loses them.
>>
>> Reject a base_dma with bits set outside VCMDQ_ADDR, the way the user-VCMDQ
>> path already does for its base_addr_pa, failing the queue init rather than
>> silently truncating the base. Use dev_warn_once() rather than WARN_ON(), as
>> the OAS is VMM-controlled and a WARN_ON() would let it panic a guest booted
>> with panic_on_warn.
>>
>> Assisted-by: Claude:claude-opus-5
>> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
>> ---
> 
> This still just sounds like something that a VMM shouldn't do?

Yeah, if a VMM is going out of its way to emulate obviously-broken 
hardware, then in my opinion we should just give it what it wants and 
let it deal with the consequences of its brokenness.

But if we really do want to bother sanity-checking emulations, then why 
allow them to still get away with being broken in the case that we do 
happen to allocate queue memory at a sufficiently low PA anyway? Why not 
actually validate smmu->oas itself (plus anything else relevant) in 
__tegra241_cmdqv_probe()?

Thanks,
Robin.

Re: [PATCH v3] iommu/tegra241-cmdqv: Reject a VCMDQ base above the 48-bit hardware limit

[Jason Gunthorpe]

On Thu, Aug 06, 2026 at 02:17:31PM +0100, Robin Murphy wrote:

> But if we really do want to bother sanity-checking emulations, then why
> allow them to still get away with being broken in the case that we do happen
> to allocate queue memory at a sufficiently low PA anyway? Why not actually
> validate smmu->oas itself (plus anything else relevant) in
> __tegra241_cmdqv_probe()?

Yeah, if the VMM has setup the SMMU so it cannot access all of DRAM
then it is going to be really broken across the entire driver. This
just doesn't work at all. No reason to add any special cases to look
for it only in vcmdq.

IMHO the error here is masking the base:

 	q->q_base = q->base_dma & VCMDQ_ADDR;

That's sort of a HW bug to design like that, the base should be a full
64 bit value and programming any base that falls outside the S2 should
always generate the same HW fault forwarded toward the VM. The HW
should have captured the unsupported upper bits and compressed them
into a 'fail all DMA' flag.

Jason

Re: [PATCH v3] iommu/tegra241-cmdqv: Reject a VCMDQ base above the 48-bit hardware limit

[Nicolin Chen]

On Thu, Aug 06, 2026 at 11:09:51AM -0300, Jason Gunthorpe wrote:
> On Thu, Aug 06, 2026 at 02:17:31PM +0100, Robin Murphy wrote:
> 
> > But if we really do want to bother sanity-checking emulations, then why
> > allow them to still get away with being broken in the case that we do happen
> > to allocate queue memory at a sufficiently low PA anyway? Why not actually
> > validate smmu->oas itself (plus anything else relevant) in
> > __tegra241_cmdqv_probe()?
> 
> Yeah, if the VMM has setup the SMMU so it cannot access all of DRAM
> then it is going to be really broken across the entire driver. This
> just doesn't work at all. No reason to add any special cases to look
> for it only in vcmdq.

It sounds that a rejection in __tegra241_cmdqv_probe() wouldn't be
necessary either.

Let's drop the patch then.

Thanks
Nicolin