Re: [RFC PATCH 09/21] xen/arm: vsmmuv3: Add support for cmdqueue handling

[Julien Grall <julien@xxxxxxx>]

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