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
