mbox series

[v9,00/11] acquire_resource size and external IPT monitoring

Message ID 20210201232703.29275-1-andrew.cooper3@citrix.com (mailing list archive)
Headers show
Series acquire_resource size and external IPT monitoring | expand

Message

Andrew Cooper Feb. 1, 2021, 11:26 p.m. UTC
Combined series (as they are dependent).  First, the resource size fixes, and
then the external IPT monitoring built on top.  Some patches got committed
before the feature freeze date last Friday.  This is the remainder.

Everything is suitably reviewed now, unless anyone has any last minute urgent
issues.

Therefore, I'd like to request a release exception.

Patch 1 is a bugfix, and the last in a long line of fixes to the
acquire_resource hypercall.  Technically it ought not to need a release ack at
this point.

The rest of the patches are a feature, originally contributed by CERT.PL for a
project they are working on, which got blocked for reasons outside of their
control (blocked on my acquire_resource fixes, and the extreme quantity of
security work this release cycle).

Intel Processor Trace is a debugging/diagnostic feature, which allows for
reconstruction of the exact execution path of the target.  As implemented
here, a monitoring agent can trace execution within the guest.

There are two production users of this already.

1) KFX - https://github.com/intel/kernel-fuzzer-for-xen-project

   This is a project lead by Tamas which is a fuzzer based on Xen, with AFL
   running in dom0, and backended with introspection and VMFork/reset for
   injecting data and parallel testing.  It uses IPT (this series) to feed the
   taken-path back to AFL, is far more convenient than recompiling the
   subject-under-test, and is far faster than using breakpoints for path
   reconstruction.

2) Drakvuf Sandbox - https://github.com/CERT-Polska/drakvuf-sandbox

   This project, lead by a team at CERT is an automatic malware-analysis SaaS
   offering, which will inspect suspicious files and attempt to provoke them
   to extract their payload, with introspection stepping in once it is fully
   unpacked, to inspect and classify the malware.

Both are very exciting projects, and the addition of IPT support like this
helps keep Xen at the forefront of hypervisor introspection technologies.

When I've got enough free time to do some paperwork, I'm intending to add IPT
as tech-preview (in particular - there are some hardware errata which concern
me, and an as-yet uninvestigated exclusion vs LBR as a hardware restriction).

It has active downstream users and extensive testing, as well as being fairly
isolated in terms of interactions with the rest of Xen, so the changes of a
showstopper affecting other features is very slim.


Andrew Cooper (1):
  xen/memory: Fix mapping grant tables with XENMEM_acquire_resource

Michał Leszczyński (7):
  xen/domain: Add vmtrace_size domain creation parameter
  tools/[lib]xl: Add vmtrace_buf_size parameter
  xen/memory: Add a vmtrace_buf resource type
  x86/vmx: Add Intel Processor Trace support
  xen/domctl: Add XEN_DOMCTL_vmtrace_op
  tools/libxc: Add xc_vmtrace_* functions
  tools/misc: Add xen-vmtrace tool

Tamas K Lengyel (3):
  xen/vmtrace: support for VM forks
  x86/vm_event: Carry the vmtrace buffer position in vm_event
  x86/vm_event: add response flag to reset vmtrace buffer

 docs/man/xl.cfg.5.pod.in                    |   9 ++
 tools/golang/xenlight/helpers.gen.go        |   2 +
 tools/golang/xenlight/types.gen.go          |   1 +
 tools/include/libxl.h                       |   7 ++
 tools/include/xenctrl.h                     |  73 +++++++++++
 tools/libs/ctrl/Makefile                    |   1 +
 tools/libs/ctrl/xc_vmtrace.c                | 128 ++++++++++++++++++++
 tools/libs/light/libxl_cpuid.c              |   1 +
 tools/libs/light/libxl_create.c             |   1 +
 tools/libs/light/libxl_types.idl            |   4 +
 tools/misc/.gitignore                       |   1 +
 tools/misc/Makefile                         |   7 ++
 tools/misc/xen-cpuid.c                      |   2 +-
 tools/misc/xen-vmtrace.c                    | 166 +++++++++++++++++++++++++
 tools/xl/xl_parse.c                         |   4 +
 xen/arch/x86/domain.c                       |  23 ++++
 xen/arch/x86/domctl.c                       |  55 +++++++++
 xen/arch/x86/hvm/vmx/vmcs.c                 |  19 ++-
 xen/arch/x86/hvm/vmx/vmx.c                  | 180 +++++++++++++++++++++++++++-
 xen/arch/x86/mm/mem_sharing.c               |   3 +
 xen/arch/x86/vm_event.c                     |  10 ++
 xen/common/compat/memory.c                  | 114 ++++++++++++++----
 xen/common/domain.c                         |  64 ++++++++++
 xen/common/grant_table.c                    |   3 +
 xen/common/memory.c                         | 153 ++++++++++++++++++-----
 xen/common/vm_event.c                       |   3 +
 xen/include/asm-arm/vm_event.h              |   6 +
 xen/include/asm-x86/cpufeature.h            |   1 +
 xen/include/asm-x86/hvm/hvm.h               |  72 +++++++++++
 xen/include/asm-x86/hvm/vmx/vmcs.h          |   4 +
 xen/include/asm-x86/msr.h                   |  32 +++++
 xen/include/asm-x86/vm_event.h              |   2 +
 xen/include/public/arch-x86/cpufeatureset.h |   1 +
 xen/include/public/domctl.h                 |  38 ++++++
 xen/include/public/memory.h                 |   1 +
 xen/include/public/vm_event.h               |  11 ++
 xen/include/xen/sched.h                     |   6 +
 xen/xsm/flask/hooks.c                       |   1 +
 38 files changed, 1150 insertions(+), 59 deletions(-)
 create mode 100644 tools/libs/ctrl/xc_vmtrace.c
 create mode 100644 tools/misc/xen-vmtrace.c

Comments

Ian Jackson Feb. 2, 2021, 12:20 p.m. UTC | #1
Andrew Cooper writes ("[PATCH v9 00/11] acquire_resource size and external IPT monitoring"):
...
> Therefore, I'd like to request a release exception.

Thanks for this writeup.

There is discussion here of the upside of granting an exception, which
certainly seems substantial enough to give this serious consideration.

> It [is] fairly isolated in terms of interactions with the rest of
> Xen, so the changes of a showstopper affecting other features is
> very slim.

This is encouraging (optimistic, even) but very general.  I would like
to see a frank and detailed assessment of the downside risks, ideally
based on analysis of the individual patches.

When I say a "frank and detailed assessment" I'm hoping to have a list
of the specific design and code changes that pose a risk to non-IPT
configurations, in decreasing order of risk.

For each one there should be brief discussion of the measures that
have exist to control that risk (eg, additional review, additional
testing), and a characterisation of the resulting risk (both in terms
of likelihood and severity of the resulting bug).

All risks that would come to a diligent reviewer's mind should be
mentioned and explicitly delath with, even if it is immediately clear
that they are not a real risk.

Do you think that would be feasible ?  We would want to make a
decision ASAP so it would have to be done quickly too - in the next
few days and certainly by the end of the week.


Since you mentioned patch 1 and asserted it didn't need a release-ack,
I looked at it in a little more detail.  It seems to contain a
moderate amount of (fairly localised) restructuring.  IDK whether
XENMEM_acquire_resource is used by non-IPT configurations but I didn't
see an assertion anywhere that it isn't.

I appreciate that whether something is "straightforward" on the one
hand, vs involving "substantial refactoring" on the ohter, or this is
a matter of judgement, which I have left up to the commiters during
this part of the freeze.  But for the record my view is that this
patch is not a "straightforward bugfix" and needs a release ack.


To give you an idea of what kind of thing I am looking for in a risk
assessment, I have written one up for
  [PATCH v9 03/11] tools/[lib]xl: Add vmtrace_buf_size parameter

Ideally I would like to go through a similar process for the other
patches.


I appreciate that this is rather a more throrough process than we have
adopted in the past.

Thanks,
Ian.
Andrew Cooper Feb. 2, 2021, 12:44 p.m. UTC | #2
On 02/02/2021 12:20, Ian Jackson wrote:
> Since you mentioned patch 1 and asserted it didn't need a release-ack,
> I looked at it in a little more detail.  It seems to contain a
> moderate amount of (fairly localised) restructuring.  IDK whether
> XENMEM_acquire_resource is used by non-IPT configurations but I didn't
> see an assertion anywhere that it isn't.

Acquire resource is used by Qemu/demu/varstored/etc (for IO emulation)
and the the domain builder (seeding the grant table with
xenstore/console details).

None of these usecases used a size calculation, and made blind mapping
calls of 1 page in size.

IPT is the first usecase to want to map more than a single page in one go.

> I appreciate that whether something is "straightforward" on the one
> hand, vs involving "substantial refactoring" on the ohter, or this is
> a matter of judgement, which I have left up to the commiters during
> this part of the freeze.  But for the record my view is that this
> patch is not a "straightforward bugfix" and needs a release ack.

I have extensive testing, demonstrating the bug already present in
staging (unable to map the guests whole grant table in default
configurations), and demonstrating the correctness of the fix.

Some of this testing (specifically, the toos/tests/* binary) is
something I plan to fix up over the ARM IOERQ and other series, and
submit later this week.  It will demonstrate the current bug in staging,
and show it fixed with patch 1 committed.  (This is something I want to
become an autotest in due course.)

Other parts of this testing cannot be submitted.  To get the compat
layer correct, I needed an XTF test and modified Xen which had a known
pattern, to check the marshalling logic didn't lose anything when a
continuation hit an interesting.  I can talk you through these tests and
assert that I have run them, but its not testing logic we can commit
into Xen, and its not anything which gets tested by OSSTest because we
don't test 32bit PVH dom0's in anger.

~Andrew
Andrew Cooper Feb. 2, 2021, 8:19 p.m. UTC | #3
On 02/02/2021 12:20, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH v9 00/11] acquire_resource size and external IPT monitoring"):
> ...
>> Therefore, I'd like to request a release exception.
> Thanks for this writeup.
>
> There is discussion here of the upside of granting an exception, which
> certainly seems substantial enough to give this serious consideration.
>
>> It [is] fairly isolated in terms of interactions with the rest of
>> Xen, so the changes of a showstopper affecting other features is
>> very slim.
> This is encouraging (optimistic, even) but very general.  I would like
> to see a frank and detailed assessment of the downside risks, ideally
> based on analysis of the individual patches.
>
> When I say a "frank and detailed assessment" I'm hoping to have a list
> of the specific design and code changes that pose a risk to non-IPT
> configurations, in decreasing order of risk.
>
> For each one there should be brief discussion of the measures that
> have exist to control that risk (eg, additional review, additional
> testing), and a characterisation of the resulting risk (both in terms
> of likelihood and severity of the resulting bug).
>
> All risks that would come to a diligent reviewer's mind should be
> mentioned and explicitly delath with, even if it is immediately clear
> that they are not a real risk.
>
> Do you think that would be feasible ?  We would want to make a
> decision ASAP so it would have to be done quickly too - in the next
> few days and certainly by the end of the week.

Honestly, I think this is an unreasonably large paperwork expectation,
particularly for changes this-clearly isolated in terms of functionality.

I'm going to explicitly disregard build/compile issues because we're not
even at code freeze or -rc1 yet, with multiple weeks yet before any
potential release, and loads of tooling.


Patch 2 adds a new domain creation parameter, which is an internal
tools/xen interface.  Default is off, and it needs explicit opting in to
(patch 3), so it will get all the testing it needs in an OSSTest smoke run.

This patch does introduce one new use of a preexisting refcounting
pattern currently under discussion.  It many leak memory in theoretical
circumstances, not practical ones.  Work to figure out how to unbreak
this pattern is in progress, and orthogonal, and needs applying uniformly

Patch 4 adds a new resource type, which is an API/ABI with userspace. 
It is a new type/index so has no current users.

Patch 5 adds enumeration for the IPT feature in hardware, as well as
context switching logic.  All context switching changes are behind an
opt-in flag, so a smoke run will be sufficient to prove no adverse
interaction in !vmtrace case.

Patch 6 adds a new domctl and subops for controlling vmtrace.  All brand
new functionality with no users, and bounded by the opt-ins from patch 2
and 5.

Patch 7 adds libxc library functions wrapping the domctl of patch 6.  No
users.

Patch 8 is example code demonstrating how to use all of the new
functionality.  It is built, but not installed.

Patch 9 extends the existing VMFork feature to cope with VMs configured
with this new functionality.  It is a no-op for regular VMs.

Patch 10 extends vm_event requests with additional optional metadata
about the tail pointer of data in the vmtrace buffer.  Doesn't alter the
behaviour for regular VMs.

Patch 11 extends vm_event responses with an optional request to reset
the vmtrace buffer position.  No users, and a no-op for regular VMs.


All of this new functionality is off-by-default and needs an explicit
opt in, for any behavioural changes to occur.  While there is no
guarantee that the implementation of the new functionality is perfect,
the development of it has found and fixed a whole slew bugs elsewhere in
Xen, and the new functionality does have extensive testing itself.

~Andrew
Ian Jackson Feb. 5, 2021, 3:36 p.m. UTC | #4
Thanks, Andy, for those writeups.  I still have substantial misgivings
I don't feel confident.  However, I don't think continuing attempts to
try to understand and/or mitigate this risk will be helpful.  I need
to make a decision now.

I think there are significant downsides to either choice here.  At
this stage of the freeze I am going to err on the side of saying
"yes", so, for the whole series:

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Provied this is committed by the end of Monday at the very latest.  I
would appreciate it if it could be committed today.

Thanks,
Ian.