diff mbox series

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

Message ID 20190514123125.29086-12-julien.grall@arm.com (mailing list archive)
State Superseded
Headers show
Series xen/arm: Provide a generic function to update Xen PT | expand

Commit Message

Julien Grall May 14, 2019, 12:31 p.m. UTC
{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(-)

Comments

Stefano Stabellini June 12, 2019, 10:33 p.m. UTC | #1
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
>
Julien Grall June 13, 2019, 8:31 a.m. UTC | #2
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,
Stefano Stabellini June 13, 2019, 6:51 p.m. UTC | #3
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.
Julien Grall June 13, 2019, 9:21 p.m. UTC | #4
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,
Stefano Stabellini June 13, 2019, 10:55 p.m. UTC | #5
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
diff mbox series

Patch

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.