Hi,
On 03/10/2023 14:19, Andrii Chepurnyi wrote:
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 data field with only 8 bits of
updated data.
Do you have a pointer to the code?
This behavior could potentially result in the
propagation of incorrect or unintended data to user-space applications.
I am a bit confused with the last sentence. Are you referring to the
device emulator or a guest user-space applications? If the latter, then
why are you singling out user-space applications?
Signed-off-by: Andrii Chepurnyi <[email protected]>
---
xen/arch/arm/ioreq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
index 3bed0a14c0..aaa2842acc 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -80,7 +80,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
ASSERT(dabt.valid);
- p.data = get_user_reg(regs, info->dabt.reg);
+ p.data = (p.dir) ? 0 : get_user_reg(regs, info->dabt.reg);
To take the 8-bits example, the assumption is that QEMU will not touch
the top 24-bits. I guess that's a fair assumption. But, at this point, I
feel it would be better to also zero the top 24-bits in handle_ioserv()
when writing back to the register.
Also, if you are worried about unintended data shared, then we should
also make the value of get_user_reg() to only share what matters to the
device model.
Lastly, NIT, the parenthesis around p.dir are not necessary.
vio->req = p;
vio->suspended = false;
vio->info.dabt_instr = instr;
Cheers,
--
Julien Grall
