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 <andrii_chepurnyi@epam.com>
Release-acked-by: Henry Wang <Henry.Wang@arm.com>
---
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 = &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;
--
2.25.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 <andrii_chepurnyi@epam.com> Reviewed-by: Julien Grall <jgrall@amazon.com> Unless there are any objections, I will commit the patch tomorrow (Friday). Cheers, -- Julien Grall
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 <andrii_chepurnyi@epam.com> > > Reviewed-by: Julien Grall <jgrall@amazon.com> > > 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] 20201005140817.1339-1-paul@xen.org -- Julien Grall
© 2016 - 2024 Red Hat, Inc.