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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 11 Nov 2016, Julien Grall wrote:
> 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.

I think it can be made to work safely using a rate-limiting technique.
Such as: Xen is only going to emulate an INVALL for a given domain only
once every one or two seconds and no more often than that. x86 has
something like that under xen/arch/x86/irq.c, see irq_ratelimit.

But to be clear, I am not saying that this is necessarily the best way
to do it. I would like to see some benchmarks first.


> 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).
 
Looking at the patches, nothing checks for discarded physical ITS
commands. Not good.


> 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).

I am interested in reading what Ian suggested to do when the physical
ITS queue is full, but I cannot find anything specific about it in the
doc.

Do you have a suggestion for this? 

The only things that come to mind right now are:

1) check if the ITS queue is full and busy loop until it is not (spin_lock 
style)
2) check if the ITS queue is full and sleep until it is not (mutex style)


> 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

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