[PATCH] xen: grant-table: Check truncation when giving access to a frame

[PATCH] xen: grant-table: Check truncation when giving access to a frame

[Julien Grall]

The version 1 of the grant-table protocol only supports frame encoded on
32-bit.

When the platform is supporting 48-bit physical address, the frame will
be encoded on 36-bit which will lead a truncation and give access to
the wrong frame.

On ARM Xen will always allow the guest to use all the physical address,
although today the RAM is always located under 40-bits (see
xen/include/public/arch-arm.h).

Add a truncation check in gnttab_update_entry_v1 to prevent the guest to
give access to the wrong frame.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    This is limiting us to a 44-bit address space whilst ARM can support
    up to 48-bit today. This number of bit will increase to 52-bit in
    upcoming processors [1].

    It might be good to start thinking to extend the version 1 of the
    protocol to use 64-bit frame number.

    [1] https://community.arm.com/groups/processors/blog/2016/01/05/armv8-a-architecture-evolution
---
 drivers/xen/grant-table.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index bb36b1e..f47c2e99 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -224,6 +224,13 @@ static void gnttab_update_entry_v1(grant_ref_t ref, domid_t domid,
 {
 	gnttab_shared.v1[ref].domid = domid;
 	gnttab_shared.v1[ref].frame = frame;
+
+	/*
+	 * V1 only supports 32-bit frame, check the truncation
+	 * to avoid giving access to the wrong frame.
+	 */
+	BUG_ON(gnttab_shared.v1[ref].frame != frame);
+
 	wmb();
 	gnttab_shared.v1[ref].flags = flags;
 }
-- 
1.9.1

Re: [Xen-devel] [PATCH] xen: grant-table: Check truncation when giving access to a frame

[David Vrabel]

On 13/06/16 11:50, Julien Grall wrote:
> The version 1 of the grant-table protocol only supports frame encoded on
> 32-bit.
> 
> When the platform is supporting 48-bit physical address, the frame will
> be encoded on 36-bit which will lead a truncation and give access to
> the wrong frame.
> 
> On ARM Xen will always allow the guest to use all the physical address,
> although today the RAM is always located under 40-bits (see
> xen/include/public/arch-arm.h).
> 
> Add a truncation check in gnttab_update_entry_v1 to prevent the guest to
> give access to the wrong frame.

In hindsight, we shouldn't have dropped the V2 support from Linux.
Should we reinstate it?

David

> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     This is limiting us to a 44-bit address space whilst ARM can support
>     up to 48-bit today. This number of bit will increase to 52-bit in
>     upcoming processors [1].
> 
>     It might be good to start thinking to extend the version 1 of the
>     protocol to use 64-bit frame number.
> 
>     [1] https://community.arm.com/groups/processors/blog/2016/01/05/armv8-a-architecture-evolution
> ---
>  drivers/xen/grant-table.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index bb36b1e..f47c2e99 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -224,6 +224,13 @@ static void gnttab_update_entry_v1(grant_ref_t ref, domid_t domid,
>  {
>  	gnttab_shared.v1[ref].domid = domid;
>  	gnttab_shared.v1[ref].frame = frame;
> +
> +	/*
> +	 * V1 only supports 32-bit frame, check the truncation
> +	 * to avoid giving access to the wrong frame.
> +	 */
> +	BUG_ON(gnttab_shared.v1[ref].frame != frame);
> +
>  	wmb();
>  	gnttab_shared.v1[ref].flags = flags;
>  }
> 

Re: [Xen-devel] [PATCH] xen: grant-table: Check truncation when giving access to a frame

[Julien Grall]

Hi David,

On 13/06/16 11:57, David Vrabel wrote:
> On 13/06/16 11:50, Julien Grall wrote:
>> The version 1 of the grant-table protocol only supports frame encoded on
>> 32-bit.
>>
>> When the platform is supporting 48-bit physical address, the frame will
>> be encoded on 36-bit which will lead a truncation and give access to
>> the wrong frame.
>>
>> On ARM Xen will always allow the guest to use all the physical address,
>> although today the RAM is always located under 40-bits (see
>> xen/include/public/arch-arm.h).
>>
>> Add a truncation check in gnttab_update_entry_v1 to prevent the guest to
>> give access to the wrong frame.
>
> In hindsight, we shouldn't have dropped the V2 support from Linux.
> Should we reinstate it?

What were the reasons to drop the v2 support from Linux? More 
importantly why people did choose to stay on v1?

Cheers,

-- 
Julien Grall

RE: [Xen-devel] [PATCH] xen: grant-table: Check truncation when giving access to a frame

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Julien Grall
> Sent: 13 June 2016 12:10
> To: David Vrabel; boris.ostrovsky@oracle.com; jgross@suse.com;
> sstabellini@kernel.org; konrad.wilk@oracle.com
> Cc: Andrew Cooper; steve.capper@arm.com; linux-kernel@vger.kernel.org;
> JBeulich@suse.com; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH] xen: grant-table: Check truncation when
> giving access to a frame
> 
> Hi David,
> 
> On 13/06/16 11:57, David Vrabel wrote:
> > On 13/06/16 11:50, Julien Grall wrote:
> >> The version 1 of the grant-table protocol only supports frame encoded on
> >> 32-bit.
> >>
> >> When the platform is supporting 48-bit physical address, the frame will
> >> be encoded on 36-bit which will lead a truncation and give access to
> >> the wrong frame.
> >>
> >> On ARM Xen will always allow the guest to use all the physical address,
> >> although today the RAM is always located under 40-bits (see
> >> xen/include/public/arch-arm.h).
> >>
> >> Add a truncation check in gnttab_update_entry_v1 to prevent the guest
> to
> >> give access to the wrong frame.
> >
> > In hindsight, we shouldn't have dropped the V2 support from Linux.
> > Should we reinstate it?
> 
> What were the reasons to drop the v2 support from Linux? More
> importantly why people did choose to stay on v1?
> 

One of the main reasons for v2's existence was to support a version of the netif protocol that pushed guest receive-side copy into the guest itself. This was done by granting pages from dom0, or from other guests, to the guest performing the copy. To do this securely a couple of things were needed:

- The ability to have (copy only) sub-page grants.
- The ability to transitively grant a ref from one domain to another.

Unfortunately the idea did not scale as it became bottle-necked on dom0's grant table size, and there were some nasty corner cases to work around (which is why we also have a swap-grant-ref hypercall). In the end, guest copy was dropped and then there was really no need to use grant table v2. Using version 1 is simpler, and gives you more grant entries per page of table, so everyone stuck with that.

  Paul

> Cheers,
> 
> --
> Julien Grall
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

RE: [Xen-devel] [PATCH] xen: grant-table: Check truncation when giving access to a frame

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Julien Grall
> Sent: 13 June 2016 11:51
> To: boris.ostrovsky@oracle.com; David Vrabel; jgross@suse.com;
> sstabellini@kernel.org; konrad.wilk@oracle.com
> Cc: steve.capper@arm.com; Andrew Cooper; linux-kernel@vger.kernel.org;
> xen-devel@lists.xen.org; Julien Grall; JBeulich@suse.com
> Subject: [Xen-devel] [PATCH] xen: grant-table: Check truncation when giving
> access to a frame
> 
> The version 1 of the grant-table protocol only supports frame encoded on
> 32-bit.
> 
> When the platform is supporting 48-bit physical address, the frame will
> be encoded on 36-bit which will lead a truncation and give access to
> the wrong frame.
> 
> On ARM Xen will always allow the guest to use all the physical address,
> although today the RAM is always located under 40-bits (see
> xen/include/public/arch-arm.h).
> 
> Add a truncation check in gnttab_update_entry_v1 to prevent the guest to
> give access to the wrong frame.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     This is limiting us to a 44-bit address space whilst ARM can support
>     up to 48-bit today. This number of bit will increase to 52-bit in
>     upcoming processors [1].
> 
>     It might be good to start thinking to extend the version 1 of the
>     protocol to use 64-bit frame number.

...or simply use version 2 of the protocol.

  Paul

Re: [Xen-devel] [PATCH] xen: grant-table: Check truncation when giving access to a frame

[Julien Grall]

Hello Paul,

On 13/06/16 13:12, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
>> Julien Grall
>> Sent: 13 June 2016 11:51
>> To: boris.ostrovsky@oracle.com; David Vrabel; jgross@suse.com;
>> sstabellini@kernel.org; konrad.wilk@oracle.com
>> Cc: steve.capper@arm.com; Andrew Cooper; linux-kernel@vger.kernel.org;
>> xen-devel@lists.xen.org; Julien Grall; JBeulich@suse.com
>> Subject: [Xen-devel] [PATCH] xen: grant-table: Check truncation when giving
>> access to a frame
>>
>> The version 1 of the grant-table protocol only supports frame encoded on
>> 32-bit.
>>
>> When the platform is supporting 48-bit physical address, the frame will
>> be encoded on 36-bit which will lead a truncation and give access to
>> the wrong frame.
>>
>> On ARM Xen will always allow the guest to use all the physical address,
>> although today the RAM is always located under 40-bits (see
>> xen/include/public/arch-arm.h).
>>
>> Add a truncation check in gnttab_update_entry_v1 to prevent the guest to
>> give access to the wrong frame.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      This is limiting us to a 44-bit address space whilst ARM can support
>>      up to 48-bit today. This number of bit will increase to 52-bit in
>>      upcoming processors [1].
>>
>>      It might be good to start thinking to extend the version 1 of the
>>      protocol to use 64-bit frame number.
>
> ...or simply use version 2 of the protocol.

On another mail [1], you said that "[v2] didn't scale it became 
bottle-necked on dom0's grant table size,...".

So it looks like to me that version 2 is the wrong way to go.
The performance should stay the same whether the platform support 
40-bit, 44-bit, 48-bit, 52-bit address space.

Regards,

-- 
Julien Grall

Re: [Xen-devel] [PATCH] xen: grant-table: Check truncation when giving access to a frame

[Julien Grall]

On 13/06/16 13:41, Julien Grall wrote:
> Hello Paul,
>
> On 13/06/16 13:12, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
>>> Julien Grall
>>> Sent: 13 June 2016 11:51
>>> To: boris.ostrovsky@oracle.com; David Vrabel; jgross@suse.com;
>>> sstabellini@kernel.org; konrad.wilk@oracle.com
>>> Cc: steve.capper@arm.com; Andrew Cooper; linux-kernel@vger.kernel.org;
>>> xen-devel@lists.xen.org; Julien Grall; JBeulich@suse.com
>>> Subject: [Xen-devel] [PATCH] xen: grant-table: Check truncation when
>>> giving
>>> access to a frame
>>>
>>> The version 1 of the grant-table protocol only supports frame encoded on
>>> 32-bit.
>>>
>>> When the platform is supporting 48-bit physical address, the frame will
>>> be encoded on 36-bit which will lead a truncation and give access to
>>> the wrong frame.
>>>
>>> On ARM Xen will always allow the guest to use all the physical address,
>>> although today the RAM is always located under 40-bits (see
>>> xen/include/public/arch-arm.h).
>>>
>>> Add a truncation check in gnttab_update_entry_v1 to prevent the guest to
>>> give access to the wrong frame.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>
>>> ---
>>>      This is limiting us to a 44-bit address space whilst ARM can
>>> support
>>>      up to 48-bit today. This number of bit will increase to 52-bit in
>>>      upcoming processors [1].
>>>
>>>      It might be good to start thinking to extend the version 1 of the
>>>      protocol to use 64-bit frame number.
>>
>> ...or simply use version 2 of the protocol.
>
> On another mail [1], you said that "[v2] didn't scale it became
> bottle-necked on dom0's grant table size,...".
>
> So it looks like to me that version 2 is the wrong way to go.
> The performance should stay the same whether the platform support
> 40-bit, 44-bit, 48-bit, 52-bit address space.

I forgot the link.

[1] 
http://lists.xenproject.org/archives/html/xen-devel/2016-06/msg01606.html

-- 
Julien Grall

RE: [Xen-devel] [PATCH] xen: grant-table: Check truncation when giving access to a frame

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 13 June 2016 13:42
> To: Paul Durrant; boris.ostrovsky@oracle.com; David Vrabel;
> jgross@suse.com; sstabellini@kernel.org; konrad.wilk@oracle.com
> Cc: Andrew Cooper; xen-devel@lists.xen.org; linux-kernel@vger.kernel.org;
> JBeulich@suse.com; steve.capper@arm.com
> Subject: Re: [Xen-devel] [PATCH] xen: grant-table: Check truncation when
> giving access to a frame
> 
> 
> 
> On 13/06/16 13:41, Julien Grall wrote:
> > Hello Paul,
> >
> > On 13/06/16 13:12, Paul Durrant wrote:
> >>> -----Original Message-----
> >>> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf
> Of
> >>> Julien Grall
> >>> Sent: 13 June 2016 11:51
> >>> To: boris.ostrovsky@oracle.com; David Vrabel; jgross@suse.com;
> >>> sstabellini@kernel.org; konrad.wilk@oracle.com
> >>> Cc: steve.capper@arm.com; Andrew Cooper; linux-
> kernel@vger.kernel.org;
> >>> xen-devel@lists.xen.org; Julien Grall; JBeulich@suse.com
> >>> Subject: [Xen-devel] [PATCH] xen: grant-table: Check truncation when
> >>> giving
> >>> access to a frame
> >>>
> >>> The version 1 of the grant-table protocol only supports frame encoded
> on
> >>> 32-bit.
> >>>
> >>> When the platform is supporting 48-bit physical address, the frame will
> >>> be encoded on 36-bit which will lead a truncation and give access to
> >>> the wrong frame.
> >>>
> >>> On ARM Xen will always allow the guest to use all the physical address,
> >>> although today the RAM is always located under 40-bits (see
> >>> xen/include/public/arch-arm.h).
> >>>
> >>> Add a truncation check in gnttab_update_entry_v1 to prevent the guest
> to
> >>> give access to the wrong frame.
> >>>
> >>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> >>>
> >>> ---
> >>>      This is limiting us to a 44-bit address space whilst ARM can
> >>> support
> >>>      up to 48-bit today. This number of bit will increase to 52-bit in
> >>>      upcoming processors [1].
> >>>
> >>>      It might be good to start thinking to extend the version 1 of the
> >>>      protocol to use 64-bit frame number.
> >>
> >> ...or simply use version 2 of the protocol.
> >
> > On another mail [1], you said that "[v2] didn't scale it became
> > bottle-necked on dom0's grant table size,...".
> >
> > So it looks like to me that version 2 is the wrong way to go.
> > The performance should stay the same whether the platform support
> > 40-bit, 44-bit, 48-bit, 52-bit address space.
> 

No, I meant the guest receive-side copy didn't scale, not grant table v2 itself. Ok the table is bigger with v2, but to do guest receive-side copy required a huge table in dom0 if it was going to scale to 100s of VMs and the perf. benefits were never that great (if they were there at all).

  Paul

> I forgot the link.
> 
> [1]
> http://lists.xenproject.org/archives/html/xen-devel/2016-06/msg01606.html
> 
> --
> Julien Grall

Re: [Xen-devel] [PATCH] xen: grant-table: Check truncation when giving access to a frame

[Julien Grall]

On 13/06/16 13:45, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@arm.com]
>> Sent: 13 June 2016 13:42
>> To: Paul Durrant; boris.ostrovsky@oracle.com; David Vrabel;
>> jgross@suse.com; sstabellini@kernel.org; konrad.wilk@oracle.com
>> Cc: Andrew Cooper; xen-devel@lists.xen.org; linux-kernel@vger.kernel.org;
>> JBeulich@suse.com; steve.capper@arm.com
>> Subject: Re: [Xen-devel] [PATCH] xen: grant-table: Check truncation when
>> giving access to a frame
>>
>>
>>
>> On 13/06/16 13:41, Julien Grall wrote:
>>> Hello Paul,
>>>
>>> On 13/06/16 13:12, Paul Durrant wrote:
>>>>> -----Original Message-----
>>>>> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf
>> Of
>>>>> Julien Grall
>>>>> Sent: 13 June 2016 11:51
>>>>> To: boris.ostrovsky@oracle.com; David Vrabel; jgross@suse.com;
>>>>> sstabellini@kernel.org; konrad.wilk@oracle.com
>>>>> Cc: steve.capper@arm.com; Andrew Cooper; linux-
>> kernel@vger.kernel.org;
>>>>> xen-devel@lists.xen.org; Julien Grall; JBeulich@suse.com
>>>>> Subject: [Xen-devel] [PATCH] xen: grant-table: Check truncation when
>>>>> giving
>>>>> access to a frame
>>>>>
>>>>> The version 1 of the grant-table protocol only supports frame encoded
>> on
>>>>> 32-bit.
>>>>>
>>>>> When the platform is supporting 48-bit physical address, the frame will
>>>>> be encoded on 36-bit which will lead a truncation and give access to
>>>>> the wrong frame.
>>>>>
>>>>> On ARM Xen will always allow the guest to use all the physical address,
>>>>> although today the RAM is always located under 40-bits (see
>>>>> xen/include/public/arch-arm.h).
>>>>>
>>>>> Add a truncation check in gnttab_update_entry_v1 to prevent the guest
>> to
>>>>> give access to the wrong frame.
>>>>>
>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>>
>>>>> ---
>>>>>       This is limiting us to a 44-bit address space whilst ARM can
>>>>> support
>>>>>       up to 48-bit today. This number of bit will increase to 52-bit in
>>>>>       upcoming processors [1].
>>>>>
>>>>>       It might be good to start thinking to extend the version 1 of the
>>>>>       protocol to use 64-bit frame number.
>>>>
>>>> ...or simply use version 2 of the protocol.
>>>
>>> On another mail [1], you said that "[v2] didn't scale it became
>>> bottle-necked on dom0's grant table size,...".
>>>
>>> So it looks like to me that version 2 is the wrong way to go.
>>> The performance should stay the same whether the platform support
>>> 40-bit, 44-bit, 48-bit, 52-bit address space.
>>
>
> No, I meant the guest receive-side copy didn't scale, not grant table v2 itself. Ok the table is bigger with v2, but to do guest receive-side copy required a huge table in dom0 if it was going to scale to 100s of VMs and the perf. benefits were never that great (if they were there at all).

Sorry I misunderstood your previous mail. So the only downside is the 
size of the table.

Looking at the structure in the header (public/grant_table.h), this is 
effectively much bigger. A commit in Linux [1] suggests that grant v2 
only supports 256 grants per page rather than 512 for v1.

How would that impact a guest?

Regards,

[1] commit 11c7ff17c9b6dbf3a4e4f36be30ad531a6cf0ec9
Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date:   Mon Jan 6 10:44:39 2014 -0500
     xen/grant-table: Force to use v1 of grants.

     We have the framework to use v2, but there are no backends that
     actually use it. The end result is that on PV we use v2 grants
     and on PVHVM v1. The v1 has a capacity of 512 grants per page while
     the v2 has 256 grants per page. This means we lose about 50%
     capacity - and if we want more than 16 VIFs (each VIF takes
     512 grants), then we are hitting the max per guest of 32.

So from my understanding the table is much more bigger.

-- 
Julien Grall

RE: [Xen-devel] [PATCH] xen: grant-table: Check truncation when giving access to a frame

[Paul Durrant]

> -----Original Message-----
[snip]
> >
> > No, I meant the guest receive-side copy didn't scale, not grant table v2
> itself. Ok the table is bigger with v2, but to do guest receive-side copy
> required a huge table in dom0 if it was going to scale to 100s of VMs and the
> perf. benefits were never that great (if they were there at all).
> 
> Sorry I misunderstood your previous mail. So the only downside is the
> size of the table.
> 
> Looking at the structure in the header (public/grant_table.h), this is
> effectively much bigger. A commit in Linux [1] suggests that grant v2
> only supports 256 grants per page rather than 512 for v1.
> 

That's correct.

> How would that impact a guest?
> 

Well, for the same table size you'd get half as many refs.

If you don't have many vifs and vbds in your guest then you'll probably be fine. If you do then you might see pressure for free entries and some drivers may actually crash if they can't get what they want. I don't believe the size of the guest's grant table can be tuned by the toolstack... probably about time to add that capability.

  Paul

> Regards,
> 
> [1] commit 11c7ff17c9b6dbf3a4e4f36be30ad531a6cf0ec9
> Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date:   Mon Jan 6 10:44:39 2014 -0500
>      xen/grant-table: Force to use v1 of grants.
> 
>      We have the framework to use v2, but there are no backends that
>      actually use it. The end result is that on PV we use v2 grants
>      and on PVHVM v1. The v1 has a capacity of 512 grants per page while
>      the v2 has 256 grants per page. This means we lose about 50%
>      capacity - and if we want more than 16 VIFs (each VIF takes
>      512 grants), then we are hitting the max per guest of 32.
> 
> So from my understanding the table is much more bigger.
> 
> --
> Julien Grall