diff mbox series

arm/ioreq: clean data field in ioreq struct on read operations

Message ID 20231003131923.2289867-1-andrii_chepurnyi@epam.com (mailing list archive)
State New, archived
Headers show
Series arm/ioreq: clean data field in ioreq struct on read operations | expand

Commit Message

Andrii Chepurnyi Oct. 3, 2023, 1:19 p.m. UTC
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. This behavior could potentially result in the
propagation of incorrect or unintended data to user-space applications.

Signed-off-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com>
---
 xen/arch/arm/ioreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrii Chepurnyi Oct. 4, 2023, 8:42 a.m. UTC | #1
Hello,

On 10/3/23 16:49, Julien Grall wrote:
> 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?

First of all, using the term "user-space" with respect to this problem 
was a mistake from my side.

In general, my use case is to run u-boot with virtio-blk inside the 
guest domain.
I.e. setup configuration(hardware renesas gen3 kingfisher board):  Dom0, 
DomD ( QEMU as backend) and running u-boot in DomA with virtio-blk.
The problem appeared inside the u-boot code :

static int virtio_pci_reset(struct udevice *udev)
{
	struct virtio_pci_priv *priv = dev_get_priv(udev);

	/* 0 status means a reset */
	iowrite8(0, &priv->common->device_status);

	/*
	 * After writing 0 to device_status, the driver MUST wait for a read
	 * of device_status to return 0 before reinitializing the device.
	 * This will flush out the status write, and flush in device writes,
	 * including MSI-X interrupts, if any.
	 */
	while (ioread8(&priv->common->device_status))
		udelay(1000);

	return 0;
}


I found that if u-boot was built with clang, it stuck in while in 
virtio_pci_reset forever. At the same time with gcc is working.

Here is a part disassembly of the virtio_pci_reset for both cases:

gcc:

0000000000000000 <virtio_pci_reset>:
    0:   a9be7bfd        stp     x29, x30, [sp, #-32]!
    4:   910003fd        mov     x29, sp
    8:   f9000bf3        str     x19, [sp, #16]
    c:   94000000        bl      0 <dev_get_priv>
   10:   aa0003f3        mov     x19, x0
   14:   f9400000        ldr     x0, [x0]
   18:   d5033fbf        dmb     sy
   1c:   3900501f        strb    wzr, [x0, #20]
   20:   f9400260        ldr     x0, [x19]
   24:   39405000        ldrb    w0, [x0, #20]
   28:   12001c00        and     w0, w0, #0xff
   2c:   d5033fbf        dmb     sy
   30:   35000080        cbnz    w0, 40 <virtio_pci_reset+0x40>
   34:   f9400bf3        ldr     x19, [sp, #16]
   38:   a8c27bfd        ldp     x29, x30, [sp], #32
   3c:   d65f03c0        ret
   40:   d2807d00        mov     x0, #0x3e8                      // #1000
   44:   94000000        bl      0 <udelay>
   48:   17fffff6        b       20 <virtio_pci_reset+0x20>


clang:

0000000000000000 <virtio_pci_reset>:
    0:   a9be7bfd        stp     x29, x30, [sp, #-32]!
    4:   f9000bf3        str     x19, [sp, #16]
    8:   910003fd        mov     x29, sp
    c:   94000000        bl      0 <dev_get_priv>
   10:   f9400008        ldr     x8, [x0]
   14:   d5033fbf        dmb     sy
   18:   3900511f        strb    wzr, [x8, #20]
   1c:   f9400008        ldr     x8, [x0]
   20:   39405108        ldrb    w8, [x8, #20]
   24:   d5033fbf        dmb     sy
   28:   34000108        cbz     w8, 48 <virtio_pci_reset+0x48>
   2c:   aa0003f3        mov     x19, x0
   30:   52807d00        mov     w0, #0x3e8                      // #1000
   34:   94000000        bl      0 <udelay>
   38:   f9400268        ldr     x8, [x19]
   3c:   39405108        ldrb    w8, [x8, #20]
   40:   d5033fbf        dmb     sy
   44:   35ffff68        cbnz    w8, 30 <virtio_pci_reset+0x30>
   48:   f9400bf3        ldr     x19, [sp, #16]
   4c:   2a1f03e0        mov     w0, wzr
   50:   a8c27bfd        ldp     x29, x30, [sp], #32
   54:   d65f03c0        ret


As you may found, in case of gcc read of 8 bit data :

   24:   39405000        ldrb    w0, [x0, #20]
   28:   12001c00        and     w0, w0, #0xff
   2c:   d5033fbf        dmb     sy

in case of clang:

   20:   39405108        ldrb    w8, [x8, #20]
   24:   d5033fbf        dmb     sy

in second case we got trash inside upper bits w8 and loop forever.


> 
>> 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?

I will rephrase description, since u-boot is not a "user-space 
applications".

> 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.

Ok, I will push v2 with respect to your comments.

Best regards,
Andrii.
Julien Grall Oct. 4, 2023, 3:41 p.m. UTC | #2
On 04/10/2023 09:42, Andrii Chepurnyi wrote:
> Hello,

Hi,

> On 10/3/23 16:49, Julien Grall wrote:
>> 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?
> 
> First of all, using the term "user-space" with respect to this problem
> was a mistake from my side.
> 
> In general, my use case is to run u-boot with virtio-blk inside the
> guest domain.
> I.e. setup configuration(hardware renesas gen3 kingfisher board):  Dom0,
> DomD ( QEMU as backend) and running u-boot in DomA with virtio-blk.
> The problem appeared inside the u-boot code :

I was asking a pointer to the code in the Device Emulator (QEMU in your 
case). I am confident the code is correct in U-boot, because when using 
'w0', the unused bits are meant to be set to zero (per the Arm Arm). But 
I am curious to know why QEMU is not doing it.

Cheers,
Andrii Chepurnyi Oct. 5, 2023, 8:11 a.m. UTC | #3
Hello,

On 10/4/23 18:41, Julien Grall wrote:

> I was asking a pointer to the code in the Device Emulator (QEMU in your 
> case). I am confident the code is correct in U-boot, because when using 
> 'w0', the unused bits are meant to be set to zero (per the Arm Arm). But 
> I am curious to know why QEMU is not doing it.

QEMU flow in the case of the read operation should be the following:

https://github.com/xen-troops/qemu/blob/v7.0.0-xt/hw/xen/xen-hvm-common.c#L389
https://github.com/xen-troops/qemu/blob/v7.0.0-xt/hw/xen/xen-hvm-common.c#L408
https://github.com/xen-troops/qemu/blob/v7.0.0-xt/hw/xen/xen-hvm-common.c#L309
https://github.com/xen-troops/qemu/blob/v7.0.0-xt/hw/xen/xen-hvm-common.c#L228
https://github.com/xen-troops/qemu/blob/v7.0.0-xt/softmmu/physmem.c#L3002
https://github.com/xen-troops/qemu/blob/v7.0.0-xt/softmmu/physmem.c#L2973
https://github.com/xen-troops/qemu/blob/v7.0.0-xt/softmmu/physmem.c#L2942
https://github.com/xen-troops/qemu/blob/v7.0.0-xt/softmmu/physmem.c#L2926
https://github.com/xen-troops/qemu/blob/v7.0.0-xt/softmmu/physmem.c#L2876

 From my understanding, only the quantity of requested bytes is updated.

Best regards,
diff mbox series

Patch

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);
     vio->req = p;
     vio->suspended = false;
     vio->info.dabt_instr = instr;