Re: [PATCH] arm/ioreq: clean data field in ioreq struct on read operations

[Julien Grall <julien@xxxxxxx>]

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 <andrii_chepurnyi@xxxxxxxx>
> ---
>   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