| 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 |
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 >
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,
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.
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,
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 --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.
{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(-)