[V1,14/16] xen/ioreq: Use guest_cmpxchg64() instead of cmpxchg()

Commit Message

Oleksandr Tyshchenko Sept. 10, 2020, 8:22 p.m. UTC

From: Oleksandr Tyshchenko <[e-mail]>

The cmpxchg() in hvm_send_buffered_ioreq() operates on memory shared
with the emulator. In order to be on the safe side we need to switch
to guest_cmpxchg64() to prevent a domain to DoS Xen on Arm.

CC: Julien Grall <[e-mail]>
Signed-off-by: Oleksandr Tyshchenko <[e-mail]>

---
Please note, this patch depends on the following patch on a review:
https://patchwork.kernel.org/patch/11715559/

Changes RFC -> V1:
   - new patch
---
---
 xen/common/ioreq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jan Beulich Sept. 16, 2020, 9:04 a.m. UTC | #1

On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> @@ -1325,7 +1327,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
>  
>          new.read_pointer = old.read_pointer - n * IOREQ_BUFFER_SLOT_NUM;
>          new.write_pointer = old.write_pointer - n * IOREQ_BUFFER_SLOT_NUM;
> -        cmpxchg(&pg->ptrs.full, old.full, new.full);
> +        guest_cmpxchg64(d, &pg->ptrs.full, old.full, new.full);

But the memory we're updating is shared with s->emulator, not with d,
if I'm not mistaken.

Jan

Julien Grall Sept. 16, 2020, 9:07 a.m. UTC | #2

On 16/09/2020 10:04, Jan Beulich wrote:
> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
>> @@ -1325,7 +1327,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
>>   
>>           new.read_pointer = old.read_pointer - n * IOREQ_BUFFER_SLOT_NUM;
>>           new.write_pointer = old.write_pointer - n * IOREQ_BUFFER_SLOT_NUM;
>> -        cmpxchg(&pg->ptrs.full, old.full, new.full);
>> +        guest_cmpxchg64(d, &pg->ptrs.full, old.full, new.full);
> 
> But the memory we're updating is shared with s->emulator, not with d,
> if I'm not mistaken.

It is unfortunately shared with both s->emulator and d when using the 
legacy interface.

For Arm, there is no plan to support the legacy interface, so we should 
s->emulator and we should be fully protected.

Cheers,

Paul Durrant Sept. 16, 2020, 9:07 a.m. UTC | #3

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 16 September 2020 10:04
> To: Oleksandr Tyshchenko <[e-mail]>
> Cc: [e-mail]; Oleksandr Tyshchenko <[e-mail]>; Paul Durrant
> <[e-mail]>; Julien Grall <[e-mail]>; Stefano Stabellini <[e-mail]>; Julien
> Grall <[e-mail]>
> Subject: Re: [PATCH V1 14/16] xen/ioreq: Use guest_cmpxchg64() instead of cmpxchg()
> 
> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> > @@ -1325,7 +1327,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
> >
> >          new.read_pointer = old.read_pointer - n * IOREQ_BUFFER_SLOT_NUM;
> >          new.write_pointer = old.write_pointer - n * IOREQ_BUFFER_SLOT_NUM;
> > -        cmpxchg(&pg->ptrs.full, old.full, new.full);
> > +        guest_cmpxchg64(d, &pg->ptrs.full, old.full, new.full);
> 
> But the memory we're updating is shared with s->emulator, not with d,
> if I'm not mistaken.
> 

You're not mistaken.

  Paul

> Jan

Paul Durrant Sept. 16, 2020, 9:09 a.m. UTC | #4

> -----Original Message-----
> From: Julien Grall <[e-mail]>
> Sent: 16 September 2020 10:07
> To: Jan Beulich <[e-mail]>; Oleksandr Tyshchenko <[e-mail]>
> Cc: [e-mail]; Oleksandr Tyshchenko <[e-mail]>; Paul Durrant
> <[e-mail]>; Stefano Stabellini <[e-mail]>; Julien Grall <[e-mail]>
> Subject: Re: [PATCH V1 14/16] xen/ioreq: Use guest_cmpxchg64() instead of cmpxchg()
> 
> 
> 
> On 16/09/2020 10:04, Jan Beulich wrote:
> > On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> >> @@ -1325,7 +1327,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
> >>
> >>           new.read_pointer = old.read_pointer - n * IOREQ_BUFFER_SLOT_NUM;
> >>           new.write_pointer = old.write_pointer - n * IOREQ_BUFFER_SLOT_NUM;
> >> -        cmpxchg(&pg->ptrs.full, old.full, new.full);
> >> +        guest_cmpxchg64(d, &pg->ptrs.full, old.full, new.full);
> >
> > But the memory we're updating is shared with s->emulator, not with d,
> > if I'm not mistaken.
> 
> It is unfortunately shared with both s->emulator and d when using the
> legacy interface.

When using magic pages they should be punched out of the P2M by the time the code gets here, so the memory should not be guest-visible.

  Paul

> 
> For Arm, there is no plan to support the legacy interface, so we should
> s->emulator and we should be fully protected.
> 
> Cheers,
> 
> --
> Julien Grall

Julien Grall Sept. 16, 2020, 9:12 a.m. UTC | #5

On 16/09/2020 10:09, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall <[e-mail]>
>> Sent: 16 September 2020 10:07
>> To: Jan Beulich <[e-mail]>; Oleksandr Tyshchenko <[e-mail]>
>> Cc: [e-mail]; Oleksandr Tyshchenko <[e-mail]>; Paul Durrant
>> <[e-mail]>; Stefano Stabellini <[e-mail]>; Julien Grall <[e-mail]>
>> Subject: Re: [PATCH V1 14/16] xen/ioreq: Use guest_cmpxchg64() instead of cmpxchg()
>>
>>
>>
>> On 16/09/2020 10:04, Jan Beulich wrote:
>>> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
>>>> @@ -1325,7 +1327,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
>>>>
>>>>            new.read_pointer = old.read_pointer - n * IOREQ_BUFFER_SLOT_NUM;
>>>>            new.write_pointer = old.write_pointer - n * IOREQ_BUFFER_SLOT_NUM;
>>>> -        cmpxchg(&pg->ptrs.full, old.full, new.full);
>>>> +        guest_cmpxchg64(d, &pg->ptrs.full, old.full, new.full);
>>>
>>> But the memory we're updating is shared with s->emulator, not with d,
>>> if I'm not mistaken.
>>
>> It is unfortunately shared with both s->emulator and d when using the
>> legacy interface.
> 
> When using magic pages they should be punched out of the P2M by the time the code gets here, so the memory should not be guest-visible.

Can you point me to the code that doing this?

Cheers,

Oleksandr Tyshchenko Sept. 22, 2020, 8:05 p.m. UTC | #6

On 16.09.20 12:12, Julien Grall wrote:

Hi all.

>
>
> On 16/09/2020 10:09, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Julien Grall <[e-mail]>
>>> Sent: 16 September 2020 10:07
>>> To: Jan Beulich <[e-mail]>; Oleksandr Tyshchenko 
>>> <[e-mail]>
>>> Cc: [e-mail]; Oleksandr Tyshchenko 
>>> <[e-mail]>; Paul Durrant
>>> <[e-mail]>; Stefano Stabellini <[e-mail]>; Julien 
>>> Grall <[e-mail]>
>>> Subject: Re: [PATCH V1 14/16] xen/ioreq: Use guest_cmpxchg64() 
>>> instead of cmpxchg()
>>>
>>>
>>>
>>> On 16/09/2020 10:04, Jan Beulich wrote:
>>>> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
>>>>> @@ -1325,7 +1327,7 @@ static int hvm_send_buffered_ioreq(struct 
>>>>> hvm_ioreq_server *s, ioreq_t *p)
>>>>>
>>>>>            new.read_pointer = old.read_pointer - n * 
>>>>> IOREQ_BUFFER_SLOT_NUM;
>>>>>            new.write_pointer = old.write_pointer - n * 
>>>>> IOREQ_BUFFER_SLOT_NUM;
>>>>> -        cmpxchg(&pg->ptrs.full, old.full, new.full);
>>>>> +        guest_cmpxchg64(d, &pg->ptrs.full, old.full, new.full);
>>>>
>>>> But the memory we're updating is shared with s->emulator, not with d,
>>>> if I'm not mistaken.
>>>
>>> It is unfortunately shared with both s->emulator and d when using the
>>> legacy interface.
>>
>> When using magic pages they should be punched out of the P2M by the 
>> time the code gets here, so the memory should not be guest-visible.
>
> Can you point me to the code that doing this?
>
> Cheers,
>
If we are not going to use legacy interface on Arm we will have a page 
to be mapped in a single domain at the time.

I will update patch to use "s->emulator" if no objections.

Julien Grall Sept. 23, 2020, 6:05 p.m. UTC | #7

Hi Oleksandr,

On 10/09/2020 21:22, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> The cmpxchg() in hvm_send_buffered_ioreq() operates on memory shared
> with the emulator. In order to be on the safe side we need to switch
> to guest_cmpxchg64() to prevent a domain to DoS Xen on Arm.
> 
> CC: Julien Grall <[e-mail]>
> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>


For bisection purpose, we need this series to at least build at every 
patch. It is fine if the IOREQ feature doesn't work.

So this patch wants to be earlier in the series to avoid breaking arm32 
compilation.

> 
> ---
> Please note, this patch depends on the following patch on a review:
> https://patchwork.kernel.org/patch/11715559/
> 
> Changes RFC -> V1:
>     - new patch
> ---
> ---
>   xen/common/ioreq.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
> index e24a481..645d8a1 100644
> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -30,6 +30,8 @@
>   #include <xen/trace.h>
>   #include <xen/vpci.h>
>   
> +#include <asm/guest_atomics.h>
> +
>   #include <public/hvm/dm_op.h>
>   #include <public/hvm/ioreq.h>
>   #include <public/hvm/params.h>
> @@ -1325,7 +1327,7 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
>   
>           new.read_pointer = old.read_pointer - n * IOREQ_BUFFER_SLOT_NUM;
>           new.write_pointer = old.write_pointer - n * IOREQ_BUFFER_SLOT_NUM;
> -        cmpxchg(&pg->ptrs.full, old.full, new.full);
> +        guest_cmpxchg64(d, &pg->ptrs.full, old.full, new.full);
>       }
>   
>       notify_via_xen_event_channel(d, s->bufioreq_evtchn);
>

Julien Grall Sept. 23, 2020, 6:12 p.m. UTC | #8

On 22/09/2020 21:05, Oleksandr wrote:
> 
> On 16.09.20 12:12, Julien Grall wrote:
> 
> Hi all.
> 
>>
>>
>> On 16/09/2020 10:09, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Julien Grall <[e-mail]>
>>>> Sent: 16 September 2020 10:07
>>>> To: Jan Beulich <[e-mail]>; Oleksandr Tyshchenko 
>>>> <[e-mail]>
>>>> Cc: [e-mail]; Oleksandr Tyshchenko 
>>>> <[e-mail]>; Paul Durrant
>>>> <[e-mail]>; Stefano Stabellini <[e-mail]>; Julien 
>>>> Grall <[e-mail]>
>>>> Subject: Re: [PATCH V1 14/16] xen/ioreq: Use guest_cmpxchg64() 
>>>> instead of cmpxchg()
>>>>
>>>>
>>>>
>>>> On 16/09/2020 10:04, Jan Beulich wrote:
>>>>> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
>>>>>> @@ -1325,7 +1327,7 @@ static int hvm_send_buffered_ioreq(struct 
>>>>>> hvm_ioreq_server *s, ioreq_t *p)
>>>>>>
>>>>>>            new.read_pointer = old.read_pointer - n * 
>>>>>> IOREQ_BUFFER_SLOT_NUM;
>>>>>>            new.write_pointer = old.write_pointer - n * 
>>>>>> IOREQ_BUFFER_SLOT_NUM;
>>>>>> -        cmpxchg(&pg->ptrs.full, old.full, new.full);
>>>>>> +        guest_cmpxchg64(d, &pg->ptrs.full, old.full, new.full);
>>>>>
>>>>> But the memory we're updating is shared with s->emulator, not with d,
>>>>> if I'm not mistaken.
>>>>
>>>> It is unfortunately shared with both s->emulator and d when using the
>>>> legacy interface.
>>>
>>> When using magic pages they should be punched out of the P2M by the 
>>> time the code gets here, so the memory should not be guest-visible.
>>
>> Can you point me to the code that doing this?
>>
>> Cheers,
>>
> If we are not going to use legacy interface on Arm we will have a page 
> to be mapped in a single domain at the time.
Right, but this is common code... You have to think what would be the 
implication if we are using the legacy interface.

Thankfully the only user of the legacy interface is x86 so far and there 
is not concern regarding the atomics operations.

If we are happy to consider that the legacy interface will never be used 
(I am starting to be worry that one will ask it on Arm...) then we 
should be fine.

I think would be worth documenting in the commit message and the code 
(hvm_allow_set_param()) that the legacy interface *must* not be used 
without revisiting the code.

Cheers,

Oleksandr Tyshchenko Sept. 23, 2020, 8:29 p.m. UTC | #9

On 23.09.20 21:12, Julien Grall wrote:

Hi Julien

>
>>>>>
>>>>>
>>>>> On 16/09/2020 10:04, Jan Beulich wrote:
>>>>>> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
>>>>>>> @@ -1325,7 +1327,7 @@ static int hvm_send_buffered_ioreq(struct 
>>>>>>> hvm_ioreq_server *s, ioreq_t *p)
>>>>>>>
>>>>>>>            new.read_pointer = old.read_pointer - n * 
>>>>>>> IOREQ_BUFFER_SLOT_NUM;
>>>>>>>            new.write_pointer = old.write_pointer - n * 
>>>>>>> IOREQ_BUFFER_SLOT_NUM;
>>>>>>> -        cmpxchg(&pg->ptrs.full, old.full, new.full);
>>>>>>> +        guest_cmpxchg64(d, &pg->ptrs.full, old.full, new.full);
>>>>>>
>>>>>> But the memory we're updating is shared with s->emulator, not 
>>>>>> with d,
>>>>>> if I'm not mistaken.
>>>>>
>>>>> It is unfortunately shared with both s->emulator and d when using the
>>>>> legacy interface.
>>>>
>>>> When using magic pages they should be punched out of the P2M by the 
>>>> time the code gets here, so the memory should not be guest-visible.
>>>
>>> Can you point me to the code that doing this?
>>>
>>> Cheers,
>>>
>> If we are not going to use legacy interface on Arm we will have a 
>> page to be mapped in a single domain at the time.
> Right, but this is common code... You have to think what would be the 
> implication if we are using the legacy interface.
>
> Thankfully the only user of the legacy interface is x86 so far and 
> there is not concern regarding the atomics operations.
>
> If we are happy to consider that the legacy interface will never be 
> used (I am starting to be worry that one will ask it on Arm...) then 
> we should be fine.
>
> I think would be worth documenting in the commit message and the code 
> (hvm_allow_set_param()) that the legacy interface *must* not be used 
> without revisiting the code.

Will do.

Patch

diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index e24a481..645d8a1 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -30,6 +30,8 @@ 
 #include <xen/trace.h>
 #include <xen/vpci.h>
 
+#include <asm/guest_atomics.h>
+
 #include <public/hvm/dm_op.h>
 #include <public/hvm/ioreq.h>
 #include <public/hvm/params.h>
@@ -1325,7 +1327,7 @@  static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
 
         new.read_pointer = old.read_pointer - n * IOREQ_BUFFER_SLOT_NUM;
         new.write_pointer = old.write_pointer - n * IOREQ_BUFFER_SLOT_NUM;
-        cmpxchg(&pg->ptrs.full, old.full, new.full);
+        guest_cmpxchg64(d, &pg->ptrs.full, old.full, new.full);
     }
 
     notify_via_xen_event_channel(d, s->bufioreq_evtchn);