Re: [PATCH v2 4/7] x86/vmx: add do_vmtrace_op

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

On Wed, Jun 24, 2020 at 6:40 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 24/06/2020 11:03, Jan Beulich wrote:
> > On 23.06.2020 19:24, Andrew Cooper wrote:
> >> On 23/06/2020 09:51, Jan Beulich wrote:
> >>> On 23.06.2020 03:04, Michał Leszczyński wrote:
> >>>> ----- 22 cze 2020 o 18:16, Jan Beulich jbeulich@xxxxxxxx napisał(a):
> >>>>
> >>>>> On 22.06.2020 18:02, Michał Leszczyński wrote:
> >>>>>> ----- 22 cze 2020 o 17:22, Jan Beulich jbeulich@xxxxxxxx napisał(a):
> >>>>>>> On 22.06.2020 16:35, Michał Leszczyński wrote:
> >>>>>>>> ----- 22 cze 2020 o 15:25, Jan Beulich jbeulich@xxxxxxxx napisał(a):
> >>>>>>>>> Is any of what you do in this switch() actually legitimate without
> >>>>>>>>> hvm_set_vmtrace_pt_size() having got called for the guest? From
> >>>>>>>>> remarks elsewhere I imply you expect the param that you currently
> >>>>>>>>> use to be set upon domain creation time, but at the very least the
> >>>>>>>>> potentially big buffer should imo not get allocated up front, but
> >>>>>>>>> only when tracing is to actually be enabled.
> >>>>>>>> Wait... so you want to allocate these buffers in runtime?
> >>>>>>>> Previously we were talking that there is too much runtime logic
> >>>>>>>> and these enable/disable hypercalls should be stripped to absolute
> >>>>>>>> minimum.
> >>>>>>> Basic arrangements can be made at domain creation time. I don't
> >>>>>>> think though that it would be a good use of memory if you
> >>>>>>> allocated perhaps many gigabytes of memory just for possibly
> >>>>>>> wanting to enable tracing on a guest.
> >>>>>>>
> >>>>>> From our previous conversations I thought that you want to have
> >>>>>> as much logic moved to the domain creation as possible.
> >>>>>>
> >>>>>> Thus, a parameter "vmtrace_pt_size" was introduced. By default it's
> >>>>>> zero (= disabled), if you set it to a non-zero value, then trace 
> >>>>>> buffers
> >>>>>> of given size will be allocated for the domain and you have possibility
> >>>>>> to use ipt_enable/ipt_disable at any moment.
> >>>>>>
> >>>>>> This way the runtime logic is as thin as possible. I assume user knows
> >>>>>> in advance whether he/she would want to use external monitoring with 
> >>>>>> IPT
> >>>>>> or not.
> >>>>> Andrew - I think you requested movement to domain_create(). Could
> >>>>> you clarify if indeed you mean to also allocate the big buffers
> >>>>> this early?
> >>>> I would like to recall what Andrew wrote few days ago:
> >>>>
> >>>> ----- 16 cze 2020 o 22:16, Andrew Cooper andrew.cooper3@xxxxxxxxxx wrote:
> >>>>> Xen has traditionally opted for a "and turn this extra thing on
> >>>>> dynamically" model, but this has caused no end of security issues and
> >>>>> broken corner cases.
> >>>>>
> >>>>> You can see this still existing in the difference between
> >>>>> XEN_DOMCTL_createdomain and XEN_DOMCTL_max_vcpus, (the latter being
> >>>>> required to chose the number of vcpus for the domain) and we're making
> >>>>> good progress undoing this particular wart (before 4.13, it was
> >>>>> concerning easy to get Xen to fall over a NULL d->vcpu[] pointer by
> >>>>> issuing other hypercalls between these two).
> >>>>>
> >>>>> There is a lot of settings which should be immutable for the lifetime of
> >>>>> the domain, and external monitoring looks like another one of these.
> >>>>> Specifying it at createdomain time allows for far better runtime
> >>>>> behaviour (you are no longer in a situation where the first time you try
> >>>>> to turn tracing on, you end up with -ENOMEM because another VM booted in
> >>>>> the meantime and used the remaining memory), and it makes for rather
> >>>>> more simple code in Xen itself (at runtime, you can rely on it having
> >>>>> been set up properly, because a failure setting up will have killed the
> >>>>> domain already).
> >>>>>
> >>>>> ...
> >>>>>
> >>>>> ~Andrew
> >>>> according to this quote I've moved buffer allocation to the 
> >>>> create_domain,
> >>>> the updated version was already sent to the list as patch v3.
> >>> I'd still like to see an explicit confirmation by him that this
> >>> use of memory is indeed what he has intended. There are much smaller
> >>> amounts of memory which we allocate on demand, just to avoid
> >>> allocating some without then ever using it.
> >> PT is a debug/diagnostic tool.  Its not something you'd run in
> >> production against a production VM.
> > Well, the suggested use with introspection made me assume differently.
> > If this is (now and forever) truly debug/diag only, so be it then.
>
> At the end of the day, it is a fine grain profiling tool, meaning that
> it is not used in the common case.
>
> The usecase presented is for fuzzing, using the execution trace
> generated by the CPU, rather than the current scheme which allegedly
> involves shuffling breakpoints around.

That's indeed the usecase we are looking to use it for. But there is
also some experimental malware analysis scenarios it is targeted for
which is what I believe the CERT folks are mostly interested in, like
https://www.vmray.com/cyber-security-blog/back-to-the-past-using-intels-processor-trace-for-enhanced-analysis.
I could also imagine this to be useful for IDS/IPS solutions like what
Bitdefender is building but I'm just speculating since the overhead
might be prohibitive for that use-case. I would consider all these
scenarios experimental at this point and absolutely not something to
enable by default. If someone presents a need for this to be supported
on production VMs that require turning it on/off at runtime not
knowing whether the feature will be used when the domain is created we
can certainly revisit the topic. But I don't expect that to happen any
time soon, if at all.

Tamas