[v3] arm/ioreq: guard interaction data on read/write operations

Commit Message

Andrii Chepurnyi Oct. 5, 2023, 1:30 p.m. UTC

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 <[e-mail]>
Release-acked-by: Henry Wang <[e-mail]>
---
 xen/arch/arm/ioreq.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Julien Grall Oct. 5, 2023, 3:17 p.m. UTC | #1

Hi Andrii,

On 05/10/2023 14:30, 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. 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 <[e-mail]>

Reviewed-by: Julien Grall <[e-mail]>

Unless there are any objections, I will commit the patch tomorrow (Friday).

Cheers,

Julien Grall Oct. 6, 2023, 12:39 p.m. UTC | #2

On 05/10/2023 16:17, Julien Grall wrote:
> Hi Andrii,

Hi,

> On 05/10/2023 14:30, 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. 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 <[e-mail]>
> 
> Reviewed-by: Julien Grall <[e-mail]>
> 
> Unless there are any objections, I will commit the patch tomorrow (Friday).

And now committed. Thanks!

I am not sure I would consider it for backport because the IOREQ is 
still a tech preview on Arm. We should consider to switch to SUPPORT, 
that said there is at least one bug that would need to be fixed first [1].

Cheers,

[1] [e-mail]

Patch

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 = &v->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;