Re: [PATCH 03/14 v4] xen/arm: vpl011: Add pl011 uart emulation in Xen

[Julien Grall <julien.grall@arm.com>]

Hi,

On 19/06/17 11:14, Andre Przywara wrote:
>> +
>> +static bool vpl011_reg32_check_access(struct hsr_dabt dabt)
>> +{
>> +    return (dabt.size != DABT_DOUBLE_WORD);
>> +}
>> +
>> +static void vpl011_update(struct domain *d)
>
> Can you please rename this function to indicate that it updates the
> *interrupt status*? That name as it is rather generic at the moment.

Well, it updates the pl011 state even though today it is only handling 
interrupt...

>> +static int vpl011_mmio_write(struct vcpu *v,
>> +                             mmio_info_t *info,
>> +                             register_t r,
>> +                             void *priv)
>> +{
>> +    struct hsr_dabt dabt = info->dabt;
>> +    uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
>> +    struct vpl011 *vpl011 = &v->domain->arch.vpl011;
>> +    struct domain *d = v->domain;
>> +    unsigned long flags;
>> +
>> +    switch ( vpl011_reg )
>> +    {
>> +    case DR:
>> +    {
>> +        uint32_t data = 0;
>> +
>> +        /*
>> +         * Since pl011 registers are 32-bit registers, all registers
>> +         * are handled similarly allowing 8-bit, 16-bit and 32-bit
>> +         * accesses.
>> +         */
>> +        if ( !vpl011_reg32_check_access(dabt) ) goto bad_width;
>> +
>> +        vreg_reg32_update(&data, r, info);
>> +        data &= 0xFF;
>
> This is not needed as vpl011_write_data()'s second argument is uint8_t,
> so the compiler does this masking anyway.
> And even if it wouldn't, you could move this statement into the call.

Sorry, I have only spotted this comment now. We should really avoid 
implicit cast when calling function. This is a call to make error if we 
ever decide to change the type of the parameter. More than the local 
variable data is 32-bit.

Better to play safe than handling security issue after afterwards.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel