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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel