| Message ID | 20231005092141.2540016-1-andrii_chepurnyi@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] arm/ioreq: guard interaction data on read/write operations | expand |
(+ Henry) Hi Andrii, @Henry, this patch is a candidate for Xen 4.18. The fix is self-contained in the IOREQ code which is in tech preview. So I think the risk is limited. On 05/10/2023 10:21, 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 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. I don't quite understand the last sentence. Is it meant to justify why the two other changes? I.e.: * Masking the value for a write * Masking the value returned by the Device-Model > > Signed-off-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com> > --- > 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; Please use 1U. > + 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; I would add a comment on top with: "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 = 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 = &v->io; > const struct instr_details instr = info->dabt_instr; > struct hsr_dabt dabt = info->dabt; > + const uint8_t access_size = (1 << dabt.size) * 8; Please use 1U. > + 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); > - This change seems to be spurious? > - p.data = get_user_reg(regs, info->dabt.reg); > + p.data = p.dir ? 0 : get_user_reg(regs, info->dabt.reg) & access_mask; For this case, I would add: "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." Cheers,
Hi Julien, > On Oct 5, 2023, at 20:30, Julien Grall <julien@xen.org> wrote: > > (+ Henry) Thanks. > > Hi Andrii, > > @Henry, this patch is a candidate for Xen 4.18. The fix is self-contained in the IOREQ code which is in tech preview. So I think the risk is limited. Sure, with your comments below properly addressed, I think it is fine to include this patch in 4.18, so: Release-acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry > > On 05/10/2023 10:21, 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 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. > > I don't quite understand the last sentence. Is it meant to justify why the two other changes? I.e.: > * Masking the value for a write > * Masking the value returned by the Device-Model > >> > Signed-off-by: Andrii Chepurnyi <andrii_chepurnyi@epam.com> >> --- >> 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; > > Please use 1U. > >> + 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; > > I would add a comment on top with: > > "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 = 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 = &v->io; >> const struct instr_details instr = info->dabt_instr; >> struct hsr_dabt dabt = info->dabt; >> + const uint8_t access_size = (1 << dabt.size) * 8; > > Please use 1U. > >> + 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); >> - > > This change seems to be spurious? > >> - p.data = get_user_reg(regs, info->dabt.reg); >> + p.data = p.dir ? 0 : get_user_reg(regs, info->dabt.reg) & access_mask; > > For this case, I would add: > > "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." > > Cheers, > > -- > Julien Grall
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 = &v->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;
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 <andrii_chepurnyi@epam.com> --- xen/arch/arm/ioreq.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)