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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel