For read operations, there's a potential issue when the data field of the ioreq struct is partially updated in the response. To address this, zero data field during read operations. This modification serves as a safeguard against implementations that may inadvertently partially update the data field in response to read requests. For instance, consider an 8-bit read operation. In such cases, QEMU, returns the same content of the dat field with only 8 bits of updated data. This behavior could potentially result in the propagation of incorrect or unintended data to ioreq clients. There is also a good point to guard interaction data with actual size of the interaction.
Signed-off-by: Andrii Chepurnyi <[email protected]> --- xen/arch/arm/ioreq.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c index 3bed0a14c0..26dae8ca28 100644 --- a/xen/arch/arm/ioreq.c +++ b/xen/arch/arm/ioreq.c @@ -17,6 +17,8 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) { const union hsr hsr = { .bits = regs->hsr }; const struct hsr_dabt dabt = hsr.dabt; + const uint8_t access_size = (1 << dabt.size) * 8; + const uint64_t access_mask = GENMASK_ULL(access_size - 1, 0); /* Code is similar to handle_read */ register_t r = v->io.req.data; @@ -26,6 +28,7 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) if ( dabt.write ) return IO_HANDLED; + r &= access_mask; r = sign_extend(dabt, r); set_user_reg(regs, dabt.reg, r); @@ -39,6 +42,8 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, struct vcpu_io *vio = &v->io; const struct instr_details instr = info->dabt_instr; struct hsr_dabt dabt = info->dabt; + const uint8_t access_size = (1 << dabt.size) * 8; + const uint64_t access_mask = GENMASK_ULL(access_size - 1, 0); ioreq_t p = { .type = IOREQ_TYPE_COPY, .addr = info->gpa, @@ -79,8 +84,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, return IO_HANDLED; ASSERT(dabt.valid); - - p.data = get_user_reg(regs, info->dabt.reg); + p.data = p.dir ? 0 : get_user_reg(regs, info->dabt.reg) & access_mask; vio->req = p; vio->suspended = false; vio->info.dabt_instr = instr; -- 2.25.1
