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>
There is currently no sanity check on the FDT passed by the bootloader.
Whilst they are stricly not necessary, it will avoid us to spend hours
to try to find out why it does not work.
>From the booting documentation for AArch32 [1] and AArch64 [2] must :
- be placed on 8-byte boundary
- not exceed 2MB (only on AArch64)
Even if AArch32 does not seem to limit the size, Xen is not currently
able to support more the 2MB FDT. It is better to crash rather with a nice
error message than claiming we are supporting any size of FDT.
The checks are mostly borrowed from the Linux code (see fixmap_remap_fdt
in arch/arm64/mm/mmu.c).
[1] Section 2 in linux/Documentation/arm64/booting.txt
[2] Section 4b in linux/Documentation/arm/Booting
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
The FDT will not be accessed before start_xen (begining of C code) is
called and it will be easier to maintain as the code could be common
between AArch32 and AArch64.
A new function early_fdt_map is introduced to map the FDT in the boot
page table.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
This function will be called by other function later one. This will
avoid forward declaration and keep the new function close to sibling
ones.
This was moved just after *_fixmap helpers as they are page table
handling functions too.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
The 2 new defines will help to avoid hardcoding the size and the end of
the slot in the code.
The newlines are added for clarity.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
When we concurrently try to unload and load crash
images we eventually get:
Xen call trace:
[<ffff82d08018b04f>] machine_kexec_add_page+0x3a0/0x3fa
[<ffff82d08018b184>] machine_kexec_load+0xdb/0x107
[<ffff82d080116e8d>] kexec.c#kexec_load_slot+0x11/0x42
[<ffff82d08011724f>] kexec.c#kexec_load+0x119/0x150
[<ffff82d080117c1e>] kexec.c#do_kexec_op_internal+0xab/0xcf
[<ffff82d080117c60>] do_kexec_op+0xe/0x1e
[<ffff82d08025c620>] pv_hypercall+0x20a/0x44a
[<ffff82d080260116>] cpufreq.c#test_all_events+0/0x30
Pagetable walk from ffff820040088320:
L4[0x104] = 00000002979d1063 ffffffffffffffff
L3[0x001] = 00000002979d0063 ffffffffffffffff
L2[0x000] = 00000002979c7063 ffffffffffffffff
L1[0x088] = 80037a91ede97063 ffffffffffffffff
The interesting thing is that the page bits (063) look legit.
The operation on which we blow up is us trying to write
in the L1 and finding that the L2 entry points to some
bizzare MFN. It stinks of a race, and it looks like
the issue is due to no concurrency locks when dealing
with the crash kernel space.
Specifically we concurrently call kimage_alloc_crash_control_page
which iterates over the kexec_crash_area.start -> kexec_crash_area.size
and once found:
if ( page )
{
image->next_crash_page = hole_end;
clear_domain_page(_mfn(page_to_mfn(page)));
}
clears. Since the parameters of what MFN to use are provided
by the callers (and the area to search is bounded) the the 'page'
is probably the same. So #1 we concurrently clear the
'control_code_page'.
The next step is us passing this 'control_code_page' to
machine_kexec_add_page. This function requires the MFNs:
page_to_maddr(image->control_code_page).
And this would always return the same virtual address, as
the MFN of the control_code_page is inside of the
kexec_crash_area.start -> kexec_crash_area.size area.
Then machine_kexec_add_page updates the L1 .. which can be done
concurrently and on subsequent calls we mangle it up.
This is all a theory at this time, but testing reveals
that adding the hypercall_create_continuation() at the
kexec hypercall fixes the crash.
NOTE: This patch follows 5c5216 (kexec: clear kexec_image slot
when unloading kexec image) to prevent crashes during
simultaneous load/unloads.
NOTE: Consideration was given to using the existing flag
KEXEC_FLAG_IN_PROGRESS to denote a kexec hypercall in
progress. This, however, overloads the original intent of
the flag which is to denote that we are about-to/have made
the jump to the crash path. The overloading would lead to
failures in existing checks on this flag as the flag would
always be set at the top level in do_kexec_op_internal().
For this reason, the new flag KEXEC_FLAG_HC_IN_PROGRESS
was introduced.
While at it, fixed the #define mismatched spacing
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
Move microcode loading earlier for the boot CPU and secondary CPUs so
that it takes place before identify_cpu() is called for each CPU.
Without this, the detected features may be wrong if the new microcode
loading adjusts the feature bits. That could mean that some fixes (e.g.
d6e9f8d4f3 ("x86/vmx: fix vmentry failure with TSX bits in LBR"))
don't work as expected.
Previously during boot, the microcode loader was invoked for each
secondary CPU started and then again for each CPU as part of an
initcall. Simplify the code so that it is invoked exactly once for each
CPU during boot.
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-acked-by: Julien Grall <julien.grall@arm.com>
Commit b988e88cc0 ("x86/emul: Add feature check for clzero") added a
feature check to the emulator, which breaks the harness without this
flag being forced to true.
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>
Razvan Cojocaru
Mohit Gambhir
Jan Beulich
Andrew Cooper
Wei Chen
Julien Grall
Anthony PERARD
Quan Xu
Wei Liu
Roger Pau Monné
Jennifer Herbert
Punit Agrawal
Chao Gao
Eric DeVolder
Ross Lagerwall