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

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

Posted by Andrii Chepurnyi 2 years, 4 months ago

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

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;
-- 
2.25.1

[for-4.18] Re: [PATCH v2] arm/ioreq: guard interaction data on read/write operations

Posted by Julien Grall 2 years, 4 months ago

(+ 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,

-- 
Julien Grall

Re: [for-4.18] [PATCH v2] arm/ioreq: guard interaction data on read/write operations

Posted by Henry Wang 2 years, 4 months ago

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