| 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 |
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,
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 --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;
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(-)