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.
