[PATCH v3] arm/ioreq: guard interaction data on read/write operations
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 ioreq clients.
During a write access, the Device Model only need to know the content
of the bits associated with the access size (e.g. for 8-bit, the lower
8-bits). During a read access, the Device Model don't need to know any
value. So restrict the value it can access.
Signed-off-by: Andrii Chepurnyi
Release-acked-by: Henry Wang
---
xen/arch/arm/ioreq.c | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
index 3bed0a14c0..5df755b48b 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -17,6 +17,8 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs,
struct vcpu *v)
{
const union hsr hsr = { .bits = regs->hsr };
const struct hsr_dabt dabt = hsr.dabt;
+const uint8_t access_size = (1U << dabt.size) * 8;
+const uint64_t access_mask = GENMASK_ULL(access_size - 1, 0);
/* Code is similar to handle_read */
register_t r = v->io.req.data;
@@ -26,6 +28,12 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs,
struct vcpu *v)
if ( dabt.write )
return IO_HANDLED;
+/*
+ * The Arm Arm requires the value to be zero-extended to the size
+ * of the register. The Device Model is not meant to touch the bits
+ * outside of the access size, but let's not trust that.
+ */
+r &= access_mask;
r = sign_extend(dabt, r);
set_user_reg(regs, dabt.reg, r);
@@ -39,6 +47,8 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
struct vcpu_io *vio = >io;
const struct instr_details instr = info->dabt_instr;
struct hsr_dabt dabt = info->dabt;
+const uint8_t access_size = (1U << dabt.size) * 8;
+const uint64_t access_mask = GENMASK_ULL(access_size - 1, 0);
ioreq_t p = {
.type = IOREQ_TYPE_COPY,
.addr = info->gpa,
@@ -80,7 +90,13 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
ASSERT(dabt.valid);
-p.data = get_user_reg(regs, info->dabt.reg);
+/*
+ * During a write access, the Device Model only need to know the content
+ * of the bits associated with the access size (e.g. for 8-bit, the lower
8-bits).
+ * During a read access, the Device Model don't need to know any value.
+ * So restrict the value it can access.
+ */
+p.data = p.dir ? 0 : get_user_reg(regs, info->dabt.reg) & access_mask;
vio->req = p;
vio->suspended = false;
vio->info.dabt_instr = instr;
--
2.25.1
[PATCH v2] arm/ioreq: guard interaction data on read/write operations
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 dat field with only 8 bits of
updated data. This behavior could potentially result in the
propagation of incorrect or unintended data to ioreq clients.
There is also a good point to guard interaction data with actual size
of the interaction.
Signed-off-by: Andrii Chepurnyi
---
xen/arch/arm/ioreq.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
index 3bed0a14c0..26dae8ca28 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -17,6 +17,8 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs,
struct vcpu *v)
{
const union hsr hsr = { .bits = regs->hsr };
const struct hsr_dabt dabt = hsr.dabt;
+const uint8_t access_size = (1 << dabt.size) * 8;
+const uint64_t access_mask = GENMASK_ULL(access_size - 1, 0);
/* Code is similar to handle_read */
register_t r = v->io.req.data;
@@ -26,6 +28,7 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs,
struct vcpu *v)
if ( dabt.write )
return IO_HANDLED;
+r &= access_mask;
r = sign_extend(dabt, r);
set_user_reg(regs, dabt.reg, r);
@@ -39,6 +42,8 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
struct vcpu_io *vio = >io;
const struct instr_details instr = info->dabt_instr;
struct hsr_dabt dabt = info->dabt;
+const uint8_t access_size = (1 << dabt.size) * 8;
+const uint64_t access_mask = GENMASK_ULL(access_size - 1, 0);
ioreq_t p = {
.type = IOREQ_TYPE_COPY,
.addr = info->gpa,
@@ -79,8 +84,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
return IO_HANDLED;
ASSERT(dabt.valid);
-
-p.data = get_user_reg(regs, info->dabt.reg);
+p.data = p.dir ? 0 : get_user_reg(regs, info->dabt.reg) & access_mask;
vio->req = p;
vio->suspended = false;
vio->info.dabt_instr = instr;
--
2.25.1
Re: [PATCH] arm/ioreq: clean data field in ioreq struct on read operations
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,
Re: [PATCH] arm/ioreq: clean data field in ioreq struct on read operations
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, >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(>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:
:
0: a9be7bfdstp x29, x30, [sp, #-32]!
4: 910003fdmov x29, sp
8: f9000bf3str x19, [sp, #16]
c: 9400bl 0
10: aa0003f3mov x19, x0
14: f940ldr x0, [x0]
18: d5033fbfdmb sy
1c: 3900501fstrbwzr, [x0, #20]
20: f9400260ldr x0, [x19]
24: 39405000ldrbw0, [x0, #20]
28: 12001c00and w0, w0, #0xff
2c: d5033fbfdmb sy
30: 3580cbnzw0, 40
34: f9400bf3ldr x19, [sp, #16]
38: a8c27bfdldp x29, x30, [sp], #32
3c: d65f03c0ret
40: d2807d00mov x0, #0x3e8 // #1000
44: 9400bl 0
48: 17f6b 20
clang:
:
0: a9be7bfdstp x29, x30, [sp, #-32]!
4: f9000bf3str x19, [sp, #16]
8: 910003fdmov x29, sp
c: 9400bl 0
10: f948ldr x8, [x0]
14: d5033fbfdmb sy
18: 3900511fstrbwzr, [x8, #20]
1c: f948ldr x8, [x0]
20: 39405108ldrbw8, [x8, #20]
24: d5033fbfdmb sy
28: 34000108cbz w8, 48
2c: aa0003f3mov x19, x0
30: 52807d00mov w0, #0x3e8 // #1000
34: 9400bl 0
38: f9400268ldr x8, [x19]
3c: 39405108ldrbw8, [x8, #20]
40: d5033fbfdmb sy
44: 3568cbnzw8, 30
48: f9400bf3ldr x19, [sp, #16]
4c: 2a1f03e0mov w0, wzr
50: a8c27bfdldp x29, x30, [sp], #32
54: d65f03c0ret
As you may found, in case of gcc read of 8 bit data :
24: 39405000ldrbw0, [x0, #20]
28: 12001c00and w0, w0, #0xff
2c: d5033fbfdmb sy
in case of clang:
20: 39405108ldrbw8, [x8, #20]
24: d5033fbfdmb 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.
[PATCH] arm/ioreq: clean data field in ioreq struct on read operations
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 --- 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.1
Re: [PATCH v4 0/3] xen-blk{back,front}: Fix two bugs in 'feature_persistent'
Hello SeongJae, Thanks for the efforts. I've tested backend patches(1,2) on my custom 5.10 kernel (since I can't test on vanilla) and it works for me. Best regards, Andrii On Sat, Jul 16, 2022 at 1:51 AM SeongJae Park wrote: > > Introduction of 'feature_persistent' made two bugs. First one is wrong > overwrite of 'vbd->feature_gnt_persistent' in 'blkback' due to wrong > parameter value caching position, and the second one is unintended > behavioral change that could break previous dynamic frontend/backend > persistent feature support changes. This patchset fixes the issues. > > Changes from v3 > (https://lore.kernel.org/xen-devel/20220715175521.126649-1...@kernel.org/) > - Split 'blkback' patch for each of the two issues > - Add 'Reported-by: Andrii Chepurnyi ' > > Changes from v2 > (https://lore.kernel.org/xen-devel/20220714224410.51147-1...@kernel.org/) > - Keep the behavioral change of v1 > - Update blkfront's counterpart to follow the changed behavior > - Update documents for the changed behavior > > Changes from v1 > (https://lore.kernel.org/xen-devel/20220106091013.126076-1-mhe...@amazon.de/) > - Avoid the behavioral change > (https://lore.kernel.org/xen-devel/20220121102309.27802-1...@kernel.org/) > - Rebase on latest xen/tip/linux-next > - Re-work by SeongJae Park > - Cc stable@ > > Maximilian Heyne (1): > xen-blkback: Apply 'feature_persistent' parameter when connect > > SeongJae Park (2): > xen-blkback: fix persistent grants negotiation > xen-blkfront: Apply 'feature_persistent' parameter when connect > > .../ABI/testing/sysfs-driver-xen-blkback | 2 +- > .../ABI/testing/sysfs-driver-xen-blkfront | 2 +- > drivers/block/xen-blkback/xenbus.c| 20 --- > drivers/block/xen-blkfront.c | 4 +--- > 4 files changed, 11 insertions(+), 17 deletions(-) > > -- > 2.25.1 >
Re: [PATCH v2] xen-blkback: fix persistent grants negotiation
Hello All,
I faced the mentioned issue recently and just to bring more context here is
our setup:
We use pvblock backend for Android guest. It starts using u-boot with
pvblock support(which frontend doesn't support the persistent grants
feature), later it loads and starts the Linux kernel(which frontend
supports the persistent grants feature). So in total, we have sequent two
different frontends reconnection, the first of which doesn't support
persistent grants.
So the original patch [1] perfectly solves the original issue and provides
the ability to use persistent grants after the reconnection when Linux
frontend which supports persistent grants comes into play.
At the same time [2] will disable the persistent grants feature for the
first and second frontend.
Is it possible to keep [1] as is?
[1]
https://lore.kernel.org/xen-devel/20220106091013.126076-1-mhe...@amazon.de/
[2] https://lore.kernel.org/xen-devel/20220714224410.51147-1...@kernel.org/
Best regards,
Andrii
On Fri, Jul 15, 2022 at 1:15 PM Oleksandr wrote:
>
> On 15.07.22 01:44, SeongJae Park wrote:
>
>
> Hello all.
>
> Adding Andrii Chepurnyi to CC who have played with the use-case which
> required reconnect recently and faced some issues with
> feature_persistent handling.
>
>
>
>
> > Persistent grants feature can be used only when both backend and the
> > frontend supports the feature. The feature was always supported by
> > 'blkback', but commit aac8a70db24b ("xen-blkback: add a parameter for
> > disabling of persistent grants") has introduced a parameter for
> > disabling it runtime.
> >
> > To avoid the parameter be updated while being used by 'blkback', the
> > commit caches the parameter into 'vbd->feature_gnt_persistent' in
> > 'xen_vbd_create()', and then check if the guest also supports the
> > feature and finally updates the field in 'connect_ring()'.
> >
> > However, 'connect_ring()' could be called before 'xen_vbd_create()', so
> > later execution of 'xen_vbd_create()' can wrongly overwrite 'true' to
> > 'vbd->feature_gnt_persistent'. As a result, 'blkback' could try to use
> > 'persistent grants' feature even if the guest doesn't support the
> > feature.
> >
> > This commit fixes the issue by moving the parameter value caching to
> > 'xen_blkif_alloc()', which allocates the 'blkif'. Because the struct
> > embeds 'vbd' object, which will be used by 'connect_ring()' later, this
> > should be called before 'connect_ring()' and therefore this should be
> > the right and safe place to do the caching.
> >
> > Fixes: aac8a70db24b ("xen-blkback: add a parameter for disabling of
> persistent grants")
> > Cc: # 5.10.x
> > Signed-off-by: Maximilian Heyne
> > Signed-off-by: SeongJae Park
> > ---
> >
> > Changes from v1[1]
> > - Avoid the behavioral change[2]
> > - Rebase on latest xen/tip/linux-next
> > - Re-work by SeongJae Park
> > - Cc stable@
> >
> > [1]
> https://lore.kernel.org/xen-devel/20220106091013.126076-1-mhe...@amazon.de/
> > [2]
> https://lore.kernel.org/xen-devel/20220121102309.27802-1...@kernel.org/
> >
> > drivers/block/xen-blkback/xenbus.c | 15 +++
> > 1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/block/xen-blkback/xenbus.c
> b/drivers/block/xen-blkback/xenbus.c
> > index 97de13b14175..16c6785d260c 100644
> > --- a/drivers/block/xen-blkback/xenbus.c
> > +++ b/drivers/block/xen-blkback/xenbus.c
> > @@ -157,6 +157,11 @@ static int xen_blkif_alloc_rings(struct xen_blkif
> *blkif)
> > return 0;
> > }
> >
> > +/* Enable the persistent grants feature. */
> > +static bool feature_persistent = true;
> > +module_param(feature_persistent, bool, 0644);
> > +MODULE_PARM_DESC(feature_persistent, "Enables the persistent grants
> feature");
> > +
> > static struct xen_blkif *xen_blkif_alloc(domid_t domid)
> > {
> > struct xen_blkif *blkif;
> > @@ -181,6 +186,8 @@ static struct xen_blkif *xen_blkif_alloc(domid_t
> domid)
> > __module_get(THIS_MODULE);
> > INIT_WORK(>free_work, xen_blkif_deferred_free);
> >
> > + blkif->vbd.feature_gnt_persistent = feature_persistent;
> > +
> > return blkif;
> > }
> >
> > @@ -472,12 +479,6 @@ static void xen_vbd_free(struct xen_vbd *vbd)
> > vbd->bdev = NULL;
> > }
> >
> > -/* Enable the persistent grants feature. */
> > -static bool feature_persistent = true;
> > -module_param(feature_persistent, bool, 0644);
> > -MODULE_PARM_DESC(feature_persistent,
>
