Hi,
On 01/12/2022 16:02, Rahul Singh wrote:
Add support for virtual cmdqueue handling for guests
This commit message is a bit light given the security implication of
this approach. See some of the questions below.
[...]
+static int arm_vsmmu_handle_cmds(struct virt_smmu *smmu)
+{
+ struct arm_vsmmu_queue *q = &smmu->cmdq;
+ struct domain *d = smmu->d;
+ uint64_t command[CMDQ_ENT_DWORDS];
+ paddr_t addr;
+
+ if ( !smmu_get_cmdq_enabled(smmu->cr[0]) )
+ return 0;
+
+ while ( !queue_empty(q) )
What is the size of the queue and what would happen if the guest fill up
the queue before triggering a call to arm_vsmmu_handle_cmds()?
+ {
+ int ret;
+
+ addr = Q_CONS_ENT(q);
+ ret = access_guest_memory_by_ipa(d, addr, command,
+ sizeof(command), false);
+ if ( ret )
+ return ret;
+
+ switch ( smmu_cmd_get_command(command[0]) )
+ {
+ case CMDQ_OP_CFGI_STE:
+ break;
It is not clear to me why there is a break here when...
+ case CMDQ_OP_PREFETCH_CFG:
+ case CMDQ_OP_CFGI_CD:
+ case CMDQ_OP_CFGI_CD_ALL:
+ case CMDQ_OP_CFGI_ALL:
+ case CMDQ_OP_CMD_SYNC:
+ break;
... the implementation for those is also empty.
+ case CMDQ_OP_TLBI_NH_ASID:
+ case CMDQ_OP_TLBI_NSNH_ALL:
+ case CMDQ_OP_TLBI_NH_VA:
+ if ( !iommu_iotlb_flush_all(smmu->d, 1) )
+ break;
It is not clear to me why you are implementing TLB flush right now but
not the other.
This call is also a massive hammer (the more if there are multiple
IOMMUs involved) and I can see this been a way for a guest to abuse the
vSMMUv3.
What is your end goal with the vSMMUv3, are you going to expose it to
untrusted environment?
+ default:
+ gdprintk(XENLOG_ERR, "vSMMUv3: unhandled command\n");
+ dump_smmu_command(command);
This would only be printed in a debug build. However, it is not clear to
me how a guest would notice that Xen didn't handle the command.
So I think this should be a gprintk() to enable the user to figure out
in production that something went wrong.
That said, isn't there a way to report error to the guest?
+ break;
+ }
+
+ if ( ret )
+ {
+ gdprintk(XENLOG_ERR,
+ "vSMMUv3: command error %d while handling command\n",
+ ret);
Same here. However, ret is so far always 0 until here. It would be
preferable if this is introduced on the first user.
+ dump_smmu_command(command);
+ }
+ queue_inc_cons(q);
+ }
+ return 0;
+}
+
static int vsmmuv3_mmio_write(struct vcpu *v, mmio_info_t *info,
register_t r, void *priv)
{
@@ -103,9 +196,15 @@ static int vsmmuv3_mmio_write(struct vcpu *v, mmio_info_t
*info,
break;
case VREG32(ARM_SMMU_CMDQ_PROD):
+ spin_lock(&smmu->cmd_queue_lock);
The amount of work done with the spin_lock() taken looks quite large.
This means a guest with N vCPUs could easily block N pCPUs for several
seconds.
How do you plan to address this?
Cheers,
--
Julien Grall
