Architecturally, all 64bit stacks are aligned on a 16 byte boundary before an
exception frame is pushed. The failsafe frame should not special in this
regard.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
Testing has revealed two issues:
1) Passing a NULL handle to set_trap_table() is intended to flush the entire
table. The 64bit guest case (and 32bit guest on 32bit Xen, when it
existed) called init_int80_direct_trap() to reset int80_bounce, but c/s
cda335c279 which introduced the 32bit guest on 64bit Xen support omitted
this step. Previously therefore, it was impossible for a 32bit guest to
reset its registered int80_bounce details.
2) init_int80_direct_trap() doesn't honour the guests request to have
interrupts disabled on entry. PVops Linux requests that interrupts are
disabled, but Xen currently leaves them enabled when following the int80
fastpath.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
Currently we crash Xen if we see an ESR_EL2.EC value we don't recognise.
As configurable disables/enables are added to the architecture
(controlled by RES1/RESO bits respectively), with associated synchronous
exceptions, it may be possible for a guest to trigger exceptions with
classes that we don't recognise.
While we can't service these exceptions in a manner useful to the guest,
we can avoid bringing down the host. Per ARM DDI 0487A.k_iss10775, page
D7-1937, EC values within the range 0x00 - 0x2c are reserved for future
use with synchronous exceptions, and EC within the range 0x2d - 0x3f may
be used for either synchronous or asynchronous exceptions.
The patch makes Xen handle any unknown EC by injecting an UNDEFINED
exception into the guest, with a corresponding (ratelimited) warning in
the log.
This patch is based on Linux commit f050fe7a9164 "arm: KVM: Survive unknown
traps from the guest".
Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
The function do_trap_hypervisor is currently handling both trap coming
from the hypervisor and the guest. This makes difficult to get specific
behavior when a trap is coming from either the guest or the hypervisor.
Split the function into two parts:
- do_trap_guest_sync to handle guest traps
- do_trap_hyp_sync to handle hypervisor traps
On AArch32, the Hyp Trap Exception provides the standard mechanism for
trapping Guest OS functions to the hypervisor (see B1.14.1 in ARM DDI
0406C.c). It cannot be generated when generated when the processor is in
Hyp Mode, instead other exception will be used. So it is fine to replace
the call to do_trap_hypervisor by do_trap_guest_sync.
For AArch64, there are two distincts exception depending whether the
exception was taken from the current level (hypervisor) or lower level
(guest).
Note that the unknown traps from guests will lead to panic Xen. This is
already behavior and is left unchanged for simplicy. A follow-up patch
will address that.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Per Table B1-3 in ARM DDI 0406C.c, the vector 0x8 for hyp is called
"Hypervisor Call".
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Currently usbdevice_list is only checked for nullity. But the OCaml
binding will convert empty list to a pointer to NULL, instead of a
NULL pointer. That means the OCaml binding will fail to disable USB.
This patch will check emptiness of usbdevice_list. And NULL is still a
valid empty list.
Signed-off-by: Robin Lee <robinlee.sysu@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
While using alloc_domheap_pages() and assuming the allocated memory is
directly accessible is okay at boot time (as we run on the idle page
tables there), memory hotplug code too assumes it can access the
resulting page tables without using map_domain_page() or alike, and
hence we need to obtain memory suitable for ordinary page table use
here.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
Commit d9b7ef209a ("x86: drop failsafe callback invocation from
assembly") didn't go quite far enough with the cleanup it did: The
changed maximum frame size should also have been reflected in the early
address range check (which has now been pointed out to have been wrong
anyway, using 60 instead of 0x60), and it should have updated the
comment ahead of the function.
Also adjust the lower bound - all is fine (for our purposes) if the
initial guest kernel stack pointer points right at the hypervisor base
address, as only memory _below_ that address is going to be written.
Additionally limit the number of times %rsi is being adjusted to what
is really needed.
Finally move exception fixup code into the designated .fixup section
and macroize the stores to guest stack.
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-acked-by: Julien grall <julien.grall@arm.com>
The introspection agent can reply to a vm_event faster than
vmx_vmexit_handler() can complete in some cases, where it is then
not safe for vm_event_set_registers() to modify v->arch.user_regs.
In the test scenario, we were stepping over an INT3 breakpoint by
setting RIP += 1. The quick reply tended to complete before the VCPU
triggering the introspection event had properly paused and been
descheduled. If the reply occurs before __context_switch() happens,
__context_switch() clobbers the reply by overwriting
v->arch.user_regs from the stack. If we don't pass through
__context_switch() (due to switching to the idle vCPU), reply data
wouldn't be picked up when switching back straight to the original
vCPU.
This patch ensures that vm_event_resume() code only sets per-VCPU
data to be used for the actual setting of registers later in
hvm_do_resume() (similar to the model used to control setting of CRs
and MSRs).
The patch additionally removes the sync_vcpu_execstate(v) call from
vm_event_resume(), which is no longer necessary, which removes the
associated broadcast TLB flush (read: performance improvement).
Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h,
where HVM-specific vm_event-related code will live. This cleans up
hvm_do_resume() and ensures that the vm_event maintainers are
responsible for changes to that code.
Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
Setting Pin Control (PC) bit (19) in MSR_P6_EVNTSEL results in a General
Protection Fault and thus results in a hypervisor crash. This behavior has
been observed on two generations of Intel processors namely, Haswell and
Broadwell. Other Intel processor generations were not tested. However, it
does seem to be a possible erratum that hasn't yet been confirmed by Intel.
To fix the problem this patch masks PC bit and returns an error in
case any guest tries to write to it on any Intel processor. In addition
to the fact that setting this bit crashes the hypervisor on Haswell and
Broadwell, the PC flag bit toggles a hardware pin on the physical CPU
every time the programmed event occurs and the hardware behavior in
response to the toggle is undefined in the SDM, which makes this bit
unsafe to be used by guests and hence should be masked on all machines.
Signed-off-by: Mohit Gambhir <mohit.gambhir@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
This is because that code, added by commit 997382b771 ("y86/vmx: dump
PIR and vIRR before ASSERT()"), was meant to be removed by the time we
finalize 4.9, but the root cause of the ASSERT() wrongly(?) triggering
still wasn't found.
Take the opportunity and also correct the format specifiers, which I
had got wrong when editing said change while committing.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
Release-Acked-by: Julien Grall <julien.grall@arm.com>
Commit 897129deab ("x86: use unambiguous register names") went a little
too far: With it we also get register names like _e15 and e15 for
non-Xen consumers using a gcc compatible compiler. Correct this.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
The primary purpose is correcting a latent bug in __get_user_check()
(the macro has no active user at present): The access_ok() check should
be before the actual access, or else any PV guest could initiate MMIO
reads with side effects.
Clean up all four macros at once:
- all arguments evaluated exactly once
- build the "check" flavor using the "nocheck" ones, instead of open
coding them
- "int" is wide enough for error codes
- name local variables without using underscores as prefixes
- avoid pointless parentheses
- add blanks after commas separating parameters or arguments
- consistently use tabs for indentation
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-acked-by: Julien grall <julien.grall@arm.com>
In the presence of bugs such as XSA-214 where a 32bit PV guest can get its
hands on a long mode segment, this change prevents register content leaking
between domains.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
With its sole other user removed, fold LOAD_C_CLOBBERED into RESTORE_ALL to
reduce the cognitive load of trying to work out which registers get modified.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
This is more readable, maintainable, and livepatchable.
This involves declaring check_for_unexpected_msi(), untrusted_msi and
pv_hypercall() suitably for use by C. While making these changes,
untrusted_msi is switched over to being a C99 bool.
No behavioural change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
This follows the Linux example of naming the entry point by how it is arrived
at, rather than its purpose.
Doing so highlights that the SAVE_VOLATILE instantiation sets up the wrong
entry_vector on the stack (although this is currently benign as we never
sysret back to a 32bit PV, and the iret path doesn't care).
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
As originally reported, the Linear Pagetable slot maps 512GB of ram as RWX,
where the guest has full read access and a lot of direct or indirect control
over the written content. It isn't hard for a PV guest to hide shellcode
here.
Therefore, increase defence in depth by auditing our current pagetable
mappings.
* The regular linear, shadow linear, and per-domain slots have no business
being executable (but need to be written), so are updated to be NX.
* The Read Only mappings of the M2P (compat and regular) don't need to be
writeable or executable.
* The PV GDT mappings and bits of the directmap don't need to be executable.
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>
Acked-by: George Dunlap <george.dunlap@citrix.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
This is for additional defence-in-depth following LDT/GDT/IDT corruption.
It causes attempted control transfers to ring 1 or 2 (via a call gate), or
attempts to use IST 3 through 7 to yield #SS, rather than executing with a
stack starting at the top of virtual address space.
Express the TSS setup in terms of structure assignment, which should be less
fragile if the IST indexes need to change, and has the useful side effect of
zeroing the reserved fields.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
The backlink field doesn't exist in a 64bit TSS, and union for esp{0..2} is of
no practical use. Specify everything with stdint types, and empty bitfields
for reserved values.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
ARM32 doesn't have an exception similar to hyp_sync of ARM64 to catch
the synchronous data abort (For example, a NULL pointer has been referenced).
Hence the SError and sync data abort will be caught by the same data abort
exception.
Since commit "3f16c8cb" we treat all data aborts caught by this excetpion
as SError. This means, we will forward Xen synchronous data abort to guest,
if the serror_op=FORWARD. This is obviously incorrect. But we don't have
any method to distinguish SError from Xen data aborts.
But we can distinguish guest generated SError from Xen data aborts. So we
want to change the policy to handle data aborts for ARM32:
1. If this data abort is guest generated SError, we will handle this data
abort follow the SError handle option setting.
2. If this data abort is synchronous data abort or Xen generate SError, we
will PANIC the whole system.
Signed-off-by: Wei Chen <Wei.Chen@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Commit 2c77db77 "xen/arm: efi: Avoid duplicating the addition of a new
bank", introduced a new function meminfo_add_bank that add a new bank.
This new code fails to check correctly the size of the array which may
result to an out-of-bounds write.
Coverity-ID: 1433183
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
get_page() logs a message when it fails (dom_cow is never dying or
paging_mode_external()), so better avoid the call when it's pointless
to do anyway.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
While a page having just a single general reference left necessarily
has a zero type reference count too, its type may still be valid (and
in validated state; at present this is only possible and relevant for
PGT_seg_desc_page, as page tables have their type forcibly zapped when
their type reference count drops to zero, and
PGT_{writable,shared}_page pages don't require any validation). In
such a case when the page is being re-used with the same type again,
validation is being skipped. As validation criteria differ between
32- and 64-bit guests, pages to be transferred between guests need to
have their validation indicator zapped (and with it we zap all other
type information at once).
This is XSA-214.
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
In particular changes to guest privilege level require the multicall
sequence to be aborted, as hypercalls are permitted from kernel mode
only. While likely not very useful in a multicall, also properly handle
the return value in the HYPERVISOR_iret case (which should be the guest
specified value).
This is XSA-213.
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Julien Grall <julien.grall@arm.com>
While the hypervisor side of commit cd91ab08ea ("x86emul: correct stub
invocation constraints") was fine, the tools side triggered a bogus
error with old gcc (4.3 and 4.4 at least). Use a slightly less
appropriate variant instead, proven to be good enough to not
re-introduce the original problem: Which of the addresses is actually
used doesn't matter much as long as the compiler can't prove that the
two pointers don't alias one another.
Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Release-acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Stub invocations need to have the space the stub occupies as an input,
to prevent the compiler from re-ordering (or omitting) writes to it.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
Commit 407a3c00ff ("compat/memory: fix build with old gcc") "fixed" a
build issue by switching to the use of uninitialized data. Due to
- the bounding of the uninitialized data item
- the accessed area being outside of Xen space
- arguments being properly verified by the native hypercall function
this is not a security issue.
Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
iommu_inclusive_mapping is enabled by default (and has been for a long time).
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Also "correct" -> "incorrect" in the text.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
This new lib devicemodel call allows multiple extents of pages to be
marked as modified in a single call. This is something needed for a
usecase I'm working on.
The xen side of the modified_memory call has been modified to accept
an array of extents. The devicemodel library either provides an array
of length 1, to support the original library function, or a second
function allows an array to be provided.
Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
copy_{to,from}_guest_buf() are now implemented using an offset of 0.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
This does only extend to the functionality here, specifically not to
the use of all-upper-case names for the macros:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
This also allows the usual cases to be simplified, by omitting an unnecessary
buf parameters, and because the macros can appropriately size the object.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
This does only extend to the functionality here, specifically not to
the use of all-upper-case names for the macros:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
This makes copying to or from a buf that isn't big enough an error.
If the buffer isnt big enough, trying to carry on regardless
can only cause trouble later on.
Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
This avoids the log message being followed by
<G><1>mm.c:5374:d0v0 could not get_page_from_l1e()
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
Use the correct vcpuop name and delete one trailing white space.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Unflattening the device tree when booting with "acpi=force" leads to the
following stack trace on AMD Seattle platform -
(XEN) Xen call trace:
(XEN) [<0000000000204bfc>] dt_irq_translate+0x48/0x58 (PC)
(XEN) [<0000000000204f5c>] dt_device_get_irq+0x34/0x38 (LR)
(XEN) [<0000000000251a08>] platform_get_irq+0x14/0x44
(XEN) [<00000000002952bc>] smmu.c#arm_smmu_dt_init+0x190/0x100c
(XEN) [<0000000000299310>] device_init+0xa8/0xdc
(XEN) [<00000000002950f0>] iommu_hardware_setup+0x34/0x68
(XEN) [<0000000000294ef0>] iommu_setup+0x48/0x1c8
(XEN) [<000000000029cecc>] start_xen+0xb94/0xd34
(XEN) [<00000083fbba91dc>] 00000083fbba91dc
The problem arises due to the unflattened device tree being
unconditionally used in iommu_hardware_setup().
Let's re-arrange the code without changing boot order to unflatten the
device tree only when acpi is disabled.
Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
* Use gprintk rather than gdprintk. These logging messages shouldn't
disappear in release builds, as they usually happen immediately before a
domain crash. Raise them from WARNING to ERR.
* Format the vmexit reason in the same base as is used in the vendor
manuals (decimal for Intel, hex for AMD), and consistently use 0x for hex
numbers.
* Consistently use "Unexpected vmexit" terminology.
In particular, this corrects the information printed for nested VT-x, and
actually prints information for nested SVM.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
Match rcu_lock_domain(), and remove the slightly misleading comment:
This isn't just the companion to rcu_lock_domain_by_id() (and that
latter function indeed also keeps the domain locked, not the domain
list).
No functional change, as rcu_read_{,un}lock() ignore their arguments
anyway.
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
According to SDM "ADVANCED PROGRAMMABLE INTERRUPT CONTROLLER (APIC) ->
"EXTENDED XAPIC (X2APIC)" -> "x2APIC State Transitions", the APIC mode
and APIC ID are preserved when handling INIT signal and a reset places
APIC to xAPIC mode and APIC base address to 0xFEE00000h (this part
is in "Local APIC" -> "Local APIC Status and Location"). So there are
two problems in current code:
1. Using reset logic (aka vlapic_reset) to handle INIT signal.
2. Forgetting resetting APIC mode and base address in vlapic_reset()
This patch introduces a new function vlapic_do_init() and replaces the
wrongly used vlapic_reset(). Also reset APIC mode and APIC base address
in vlapic_reset().
Note that: LDR is read only in x2APIC mode. Resetting it to zero in x2APIC
mode is unreasonable. This patch also doesn't reset LDR when handling INIT
signal in x2APIC mode.
Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>