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

[Andrii Chepurnyi <Andrii_Chepurnyi@xxxxxxxx>]

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.