Re: [Xen-devel] [RFC PATCH 21/24] ARM: vITS: handle INVALL command

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 10/11/16 20:42, Stefano Stabellini wrote:
> On Thu, 10 Nov 2016, Julien Grall wrote:
> > On 10/11/16 00:21, Stefano Stabellini wrote:
> > > On Fri, 4 Nov 2016, Andre Przywara wrote:
> > > > On 24/10/16 16:32, Vijay Kilari wrote:
> > > > > On Wed, Sep 28, 2016 at 11:54 PM, Andre Przywara
> > > > >         AFAIK, the initial design is to use tasklet to update property
> > > > > table as it consumes
> > > > > lot of time to update the table.
> > > > This is a possible, but premature optimization.
> > > > Linux (at the moment, at least) only calls INVALL _once_, just after
> > > > initialising the collections. And at this point no LPI is mapped, so the
> > > > whole routine does basically nothing - and that quite fast.
> > > > We can later have any kind of fancy algorithm if there is a need for.
> > > I understand, but as-is it's so expensive that could be a DOS vector.
> > > Also other OSes could issue INVALL much more often than Linux.
> > >
> > > Considering that we might support device assigment with ITS soon, I
> > > think it might be best to parse per-domain virtual tables rather than
> > > the full list of physical LPIs, which theoretically could be much
> > > larger. Or alternatively we need to think about adding another field to
> > > lpi_data, to link together all lpis assigned to the same domain, but
> > > that would cost even more memory. Or we could rate-limit the INVALL
> > > calls to one every few seconds or something. Or all of the above :-)
> > It is not necessary for an ITS implementation to wait until an INVALL/INV
> > command is issued to take into account the change of the LPI configuration
> > tables (aka property table in this thread).
> >
> > So how about trapping the property table? We would still have to go through
> > the property table the first time (i.e when writing into the GICR_PROPBASER),
> > but INVALL would be a nop.
> >
> > The idea would be unmapping the region when GICR_PROPBASER is written. So any
> > read/write access would be trapped. For a write access, Xen will update the
> > LPIs internal data structures and write the value in the guest page unmapped.
> > If we don't want to have an overhead for the read access, we could just
> > write-protect the page in stage-2 page table. So only write access would be
> > trapped.
> >
> > Going further, for the ITS, Xen is using the guest memory to store the ITS
> > information. This means Xen has to validate the information at every access.
> > So how about restricting the access in stage-2 page table? That would remove
> > the overhead of validating data.
> >
> > Any thoughts?
> It is a promising idea. Let me expand on this.
>
> I agree that on INVALL if we need to do anything, we should go through
> the virtual property table rather than the full list of host lpis.
I agree on that.

> Once we agree on that, the two options we have are:
I believe we had a similar discussion when Vijay worked on the vITS (see 
[1]). I would have hoped that this new proposal took into account the 
constraint mentioned back then.
> 1) We let the guest write anything to the table, then we do a full
> validation of the table on INVALL. We also do a validation of the table
> entries used as parameters for any other commands.
>
> 2) We map the table read-only, then do a validation of every guest
> write. INVALL becomes a NOP and parameters validation for many commands
> could be removed or at least reduced.
>
> Conceptually the two options should both lead to exactly the same
> result. Therefore I think the decision should be made purely on
> performance: which one is faster?  If it is true that INVALL is only
> typically called once I suspect that 1) is faster, but I would like to
> see some simple benchmarks, such as the time that it takes to configure
> the ITS from scratch with the two approaches.
The problem is not which one is faster but which one will not take down 
the hypervisor.
The guest is allowed to create a command queue as big as 1MB, a command 
use 32 bytes, so the command queue can fit up 32640 commands.
Now imagine a malicious guest filling up the command queue with INVALL 
and then notify the via (via GITS_CWRITER). Based on patch #5, all those 
commands will be handled in one. So you have to multiple the time of one 
command by 32640 times.
Given that the hypervisor is not preemptible, it likely means a DOS.
A similar problem would happen if an vITS command is translate to an ITS 
command (see the implementation of INVALL). Multiple malicious guest 
could slow down the other guest by filling up the host command queue. 
Worst, a command from a normal guest could be discarded because the host 
ITS command queue is full (see its_send_command in gic-its.c).
That's why in the approach we had on the previous series was "host ITS 
command should be limited when emulating guest ITS command". From my 
recall, in that series the host and guest LPIs was fully separated 
(enabling a guest LPIs was not enabling host LPIs).
That said, a design doc explaining all the constraints and code flow 
would have been really helpful. It took me a while to digest and 
understand the interaction between each part of the code. The design 
document would have also been a good place to discuss about problems 
that span across multiple patches (like the command queue emulation).
> That said, even if 1) turns out to be faster and the approach of choice,
> the idea of making the tables read-only in stage-2 could still be useful
> to simplify parameters validation and protect Xen from concurrent
> changes of the table entries from another guest vcpu. If the tables as
> RW, we need to be very careful in Xen and use barriers to avoid
> re-reading any guest table entry twice, as the guest could be changing
> it in parallel to exploit the hypervisor.
Yes, and this is true for all the tables (PROPBASER, BASER,...) that 
reside on guest memory. Most of them should not be touched by the guest.
This is the same for the command queue (patch #12), accessing the 
command directly from the guest memory is not safe. A guest could modify 
the value behind our back.
Regards,

[1] https://xenbits.xen.org/people/ianc/vits/draftG.html

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel