[Xen-devel] [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()

Julien Grall posted 12 patches 1 year, 11 months ago

[Xen-devel] [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()

Posted by Julien Grall 1 year, 11 months ago
{set, clear}_fixmap() are currently open-coding update to the Xen
page-tables. This can be avoided by using the generic helpers
map_pages_to_xen() and destroy_xen_mappings().

Both function are not meant to fail for fixmap, hence the BUG_ON()
checking the return.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

---
    Changes in v2:
        - Add Andrii's reviewed-by
---
 xen/arch/arm/mm.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 9a40754f44..23ca61e8f0 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -348,19 +348,19 @@ static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
 /* Map a 4k page in a fixmap entry */
 void set_fixmap(unsigned map, mfn_t mfn, unsigned int flags)
 {
-    lpae_t pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
-    pte.pt.table = 1; /* 4k mappings always have this bit set */
-    pte.pt.xn = 1;
-    write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
-    flush_xen_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
+    int res;
+
+    res = map_pages_to_xen(FIXMAP_ADDR(map), mfn, 1, flags);
+    BUG_ON(res != 0);
 }
 
 /* Remove a mapping from a fixmap entry */
 void clear_fixmap(unsigned map)
 {
-    lpae_t pte = {0};
-    write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
-    flush_xen_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
+    int res;
+
+    res = destroy_xen_mappings(FIXMAP_ADDR(map), FIXMAP_ADDR(map) + PAGE_SIZE);
+    BUG_ON(res != 0);
 }
 
 /* Create Xen's mappings of memory.
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()

Posted by Stefano Stabellini 1 year, 10 months ago
On Tue, 14 May 2019, Julien Grall wrote:
> {set, clear}_fixmap() are currently open-coding update to the Xen
> page-tables. This can be avoided by using the generic helpers
> map_pages_to_xen() and destroy_xen_mappings().
> 
> Both function are not meant to fail for fixmap, hence the BUG_ON()
> checking the return.

BUG_ON crashes the hypervisor even in non-DEBUG builds. Would an ASSERT
be a better choice?

The changes in this patch checks out OK.


> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> 
> ---
>     Changes in v2:
>         - Add Andrii's reviewed-by
> ---
>  xen/arch/arm/mm.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 9a40754f44..23ca61e8f0 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -348,19 +348,19 @@ static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
>  /* Map a 4k page in a fixmap entry */
>  void set_fixmap(unsigned map, mfn_t mfn, unsigned int flags)
>  {
> -    lpae_t pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
> -    pte.pt.table = 1; /* 4k mappings always have this bit set */
> -    pte.pt.xn = 1;
> -    write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
> -    flush_xen_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
> +    int res;
> +
> +    res = map_pages_to_xen(FIXMAP_ADDR(map), mfn, 1, flags);
> +    BUG_ON(res != 0);
>  }
>  
>  /* Remove a mapping from a fixmap entry */
>  void clear_fixmap(unsigned map)
>  {
> -    lpae_t pte = {0};
> -    write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
> -    flush_xen_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
> +    int res;
> +
> +    res = destroy_xen_mappings(FIXMAP_ADDR(map), FIXMAP_ADDR(map) + PAGE_SIZE);
> +    BUG_ON(res != 0);
>  }
>  
>  /* Create Xen's mappings of memory.
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()

Posted by Julien Grall 1 year, 10 months ago
Hi Stefano,

On 6/12/19 11:33 PM, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> {set, clear}_fixmap() are currently open-coding update to the Xen
>> page-tables. This can be avoided by using the generic helpers
>> map_pages_to_xen() and destroy_xen_mappings().
>>
>> Both function are not meant to fail for fixmap, hence the BUG_ON()
>> checking the return.
> 
> BUG_ON crashes the hypervisor even in non-DEBUG builds. Would an ASSERT
> be a better choice?
The ASSERT() would disappear in non-debug potentially leading to unknown 
consequence.

If we imagine that map_pages_to_xen() fails, then it likely means that 
mapping has not been done/removed.

As set_fixmap() does not return an error, this means that the user may 
try to access an invalid mapping and therefore crash the hypervisor.

As clear_fixmap() does not return an error, this means that subsequent 
set_fixmap() may fail because map_pages_to_xen() does not allow to 
replace valid mapping.

Ideally we would want to propagate the error, however all the call to 
the functions happen during boot. So most likely the user will 
panic/BUG_ON as you this hint something has gone really wrong and we 
don't want to continue further.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()

Posted by Stefano Stabellini 1 year, 10 months ago
On Thu, 13 Jun 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 6/12/19 11:33 PM, Stefano Stabellini wrote:
> > On Tue, 14 May 2019, Julien Grall wrote:
> > > {set, clear}_fixmap() are currently open-coding update to the Xen
> > > page-tables. This can be avoided by using the generic helpers
> > > map_pages_to_xen() and destroy_xen_mappings().
> > > 
> > > Both function are not meant to fail for fixmap, hence the BUG_ON()
> > > checking the return.
> > 
> > BUG_ON crashes the hypervisor even in non-DEBUG builds. Would an ASSERT
> > be a better choice?
> The ASSERT() would disappear in non-debug potentially leading to unknown
> consequence.
> 
> If we imagine that map_pages_to_xen() fails, then it likely means that mapping
> has not been done/removed.
> 
> As set_fixmap() does not return an error, this means that the user may try to
> access an invalid mapping and therefore crash the hypervisor.
> 
> As clear_fixmap() does not return an error, this means that subsequent
> set_fixmap() may fail because map_pages_to_xen() does not allow to replace
> valid mapping.
>
> Ideally we would want to propagate the error, however all the call to the
> functions happen during boot. So most likely the user will panic/BUG_ON as you
> this hint something has gone really wrong and we don't want to continue
> further.

I think the basic principle is that with BUG_ON is "easy" for a guest to
be able to trigger it, potentially causing a DOS. Without the BUG_ON,
the guest is unlikely to be able to trigger a crash. However, if all the
calls happen during boot in regards to operations that have nothing to
do with guests behavior, then it is fine.

I checked all the call sites and I agree that in this case they are all
done during boot only. So in this case it is OK to have the
panic/BUG_ON.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()

Posted by Julien Grall 1 year, 10 months ago
Hi Stefano,

On 13/06/2019 19:51, Stefano Stabellini wrote:
> On Thu, 13 Jun 2019, Julien Grall wrote:
>> On 6/12/19 11:33 PM, Stefano Stabellini wrote:
>>> On Tue, 14 May 2019, Julien Grall wrote:
> I think the basic principle is that with BUG_ON is "easy" for a guest to
> be able to trigger it, potentially causing a DOS. Without the BUG_ON,
> the guest is unlikely to be able to trigger a crash. However, if all the
> calls happen during boot in regards to operations that have nothing to
> do with guests behavior, then it is fine.

Sadly, we don't seem to have used that approach on Arm so far. We have 
quite a few BUG_ON() that could be triggered by the guest. For instance, 
we used it to confirm that we interpreted correctly the spec... (see 
GUEST_BUG_ON). The rationale was that a DOS is better than data leak.

I have a series to try to reduce such BUG_ON.

> 
> I checked all the call sites and I agree that in this case they are all
> done during boot only. So in this case it is OK to have the
> panic/BUG_ON.

Can I consider this as an acked-by/reviewed-by?

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH MM-PART3 v2 11/12] xen/arm: mm: Don't open-code Xen PT update in {set, clear}_fixmap()

Posted by Stefano Stabellini 1 year, 10 months ago
On Thu, 13 Jun 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 13/06/2019 19:51, Stefano Stabellini wrote:
> > On Thu, 13 Jun 2019, Julien Grall wrote:
> >> On 6/12/19 11:33 PM, Stefano Stabellini wrote:
> >>> On Tue, 14 May 2019, Julien Grall wrote:
> > I think the basic principle is that with BUG_ON is "easy" for a guest to
> > be able to trigger it, potentially causing a DOS. Without the BUG_ON,
> > the guest is unlikely to be able to trigger a crash. However, if all the
> > calls happen during boot in regards to operations that have nothing to
> > do with guests behavior, then it is fine.
> 
> Sadly, we don't seem to have used that approach on Arm so far. We have 
> quite a few BUG_ON() that could be triggered by the guest. For instance, 
> we used it to confirm that we interpreted correctly the spec... (see 
> GUEST_BUG_ON). The rationale was that a DOS is better than data leak.
> 
> I have a series to try to reduce such BUG_ON.

Good!


> > 
> > I checked all the call sites and I agree that in this case they are all
> > done during boot only. So in this case it is OK to have the
> > panic/BUG_ON.
> 
> Can I consider this as an acked-by/reviewed-by?

Yes

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel