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(-)
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;
--
2.25.1Hi, 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@epam.com> > --- > 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
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.
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, -- Julien Grall
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,
© 2016 - 2024 Red Hat, Inc.