Re: [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support

[Vijay Kilari <vijay.kilari@gmail.com>]

Hi Julien,

On Wed, Jul 15, 2015 at 6:27 PM, Julien Grall <julien.grall@citrix.com> wrote:
> Hi Vijay,
>
> On 10/07/2015 09:42, vijay.kilari@gmail.com wrote:
>>
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> Add Virtual ITS command processing support to Virtual ITS driver
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> ---
>> v4: - Use helper function to read from command queue
>>      - Add MOVALL
>>      - Removed check for entry in device in domain RB-tree
>> ---
[..]
>> +
>> +static int vgic_its_process_mapvi(struct vcpu *v, struct vgic_its *vits,
>> +                                  its_cmd_block *virt_cmd)
>> +{
>> +    struct vitt entry;
>> +    struct domain *d = v->domain;
>> +    uint8_t vcol_id, cmd;
>> +    uint32_t vid, dev_id, event;
>> +
>> +    vcol_id = virt_cmd->mapvi.col;
>> +    vid = virt_cmd->mapvi.phy_id;
>> +    cmd = virt_cmd->mapvi.cmd;
>> +    dev_id = virt_cmd->mapvi.devid;
>> +
>> +    DPRINTK("%pv: vITS: MAPVI: dev_id 0x%x vcol_id %d vid %d \n",
>> +             v, dev_id, vcol_id, vid);
>> +
>> +    if ( vcol_id > (d->max_vcpus + 1) ||  vid > its_get_nr_events() )
>> +        return -EINVAL;
>
>
> As said on v3, checking the validity is pointless as a malicious guest can
> rewrite the
> ITT. We only need to check it when the LPI is effectively injected.
>
> If you think this is necessary please explain why...

   vcol_id is read from ITS command but not from guest memory.
So command values are validated  here instead of doing at time time
of LPI injection.

If not done here, then we still allow malicious guest to run and during
LPI injection if invalid col_id is found in ITT we just drop LPI.

>
> Furthermore, its_get_nr_events is for the hardware and not the virtual ITS.
> I would prefer to see a field in the vits structure which contains the
> number of event ID bits for a given domain.

 Why do we need to restrict number of LPIs to domain?

[...]

>> +
>> +    if ( vits_get_vitt_entry(d, dev_id, event, &vitt_entry) )
>> +        return -EINVAL;
>> +
>> +    if ( !vitt_entry.valid )
>> +    {
>> +        dprintk(XENLOG_G_ERR,
>> +                "%pv: vITS: INT CMD invalid event %d for dev 0x%x\n",
>> +                v, event, dev_id);
>> +        return -EINVAL;
>> +    }
>> +
>> +    col_id = vitt_entry.vcollection;
>> +    if ( col_id < d->max_vcpus )
>> +    {
>> +        dprintk(XENLOG_G_ERR,
>> +                "%pv: vITS: INT CMD invalid col_id  %d for dev 0x%x\n",
>> +                v, col_id, dev_id);
>> +        return -EINVAL;
>> +    }
>> +
>> +    vgic_vcpu_inject_irq(d->vcpu[col_id], vitt_entry.vlpi);
>
>
> As said on v3, the design document [1] suggested to implement the INT
> command using vgic_vcpu_inject_lpi. Is there any issue to do it?

IIRC,  INT command contains vlpi which does not have its_device because irq_desc
is not reserved for this. Hence it is handled similar to event_ch int.

>
> Also, you have to translate the col_id into to a VCPU ID.

This is virtual collection id which itself is vcpu id. isn't it?

Regards
Vijay