As the recent work to get PCI Passthrough testing working shows, putting
`/bin/false` as a ROM into guest context doesn't work so well.
For all ROM paths where we're skipping the build, use a slightly-plausible but
likely non-existent path instead.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
The prior check has && vs || mixups, making it tautologically false and thus
providing no safety at all. There are boundary errors too.
First start with a comment describing how the .altinstructions and
.altinstr_replacement sections interact, and perform suitable cross-checking.
Second, rewrite the alt_instr loop entirely from scratch. Origin sites have
non-zero size, and must be fully contained within the livepatches .text
section(s). Any non-zero sized replacements must be fully contained within
the .altinstr_replacement section.
Fixes: f8a10174e8 ("xsplice: Add support for alternatives")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Alternatives auditing for livepatches is currently broken. To fix it, the
livepatch code needs to inspect more fields of alt_instr.
Rename ARM's fields to match x86's, because:
* ARM already exposes alt_offset under the repl name via ALT_REPL_PTR().
* "alt" is ambiguous in a structure entirely about alternatives already.
* "repl", being the same width as orig leads to slightly neater code.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
It is rude to hide width formatting inside a PRI* macro, doubly so when it's
only in one bitness of the macro.
However its fully buggy when all the users use %#"PRI because then it expands
to the common trap of %#08x which does not do what the author intends.
Switch the 32bit ELF PRI formatters to use plain integer PRI's, just like on
the 64bit side already. No practical change.
Fixes: 7597fabca7 ("livepatch: Include sizes when an mismatch occurs")
Fixes: 380b229634 ("xsplice: Implement payload loading")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Right now, trying to apply a livepatch on any system with CET shstk (AMD Zen3
or later, Intel Tiger Lake or Sapphire Rapids and later) fails as follows:
(XEN) livepatch: lp: Verifying enabled expectations for all functions
(XEN) common/livepatch.c:1591: livepatch: lp: timeout is 30000000ns
(XEN) common/livepatch.c:1703: livepatch: lp: CPU28 - IPIing the other 127 CPUs
(XEN) livepatch: lp: Applying 1 functions
(XEN) hi_func: Hi! (called 1 times)
(XEN) Hook executing.
(XEN) Assertion 'local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu))' failed at arch/x86/smp.c:265
(XEN) *** DOUBLE FAULT ***
<many double faults>
The assertion failure is from a global (system wide) TLB flush initiated by
modify_xen_mappings(). I'm not entirely sure when this broke, and I'm not
sure exactly what causes the #DF's, but it doesn't really matter either
because they highlight a latent bug that I'd overlooked with the CET-SS vs
patching work the first place.
While we're careful to arrange for the patching CPU to avoid encountering
non-shstk memory with transient shstk perms, other CPUs can pick these
mappings up too if they need to re-walk for uarch reasons.
Another bug is that for livepatching, we only disable CET if shadow stacks are
in use. Running on Intel CET systems when Xen is only using CET-IBT will
crash in arch_livepatch_quiesce() when trying to clear CR0.WP with CR4.CET
still active.
Also, we never went and cleared the dirty bits on .rodata. This would
matter (for the same reason it matters on .text - it becomes a valid target
for WRSS), but we never actually patch .rodata anyway.
Therefore rework how we do patching for both alternatives and livepatches.
Introduce modify_xen_mappings_lite() with a purpose similar to
modify_xen_mappings(), but stripped down to the bare minimum as it's used in
weird contexts. Leave all complexity to the caller to handle.
Instead of patching by clearing CR0.WP (and having to jump through some
fragile hoops to disable CET in order to do this), just transiently relax the
permissions on .text via l2_identmap[].
Note that neither alternatives nor livepatching edit .rodata, so we don't need
to relax those permissions at this juncture.
The perms are relaxed globally, but this is safe enough. Alternatives run
before we boot APs, and Livepatching runs in a quiesced state where the other
CPUs are not doing anything interesting.
This approach is far more robust.
Fixes: 48cdc15a42 ("x86/alternatives: Clear CR4.CET when clearing CR0.WP")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
The Long Mode consistency checks exist to "ensure that the processor does not
enter an undefined mode or state that results in unpredictable behavior". APM
Vol2 Table 14-5 "Long-Mode Consistency Checks" lists them, but there is no row
preventing the OS from trying to exit Long mode while in 64bit mode. This
could leave the CPU in Protected Mode with an %rip above the 4G boundary.
Experimentally, AMD CPUs really do permit this state transition. An OS which
tries it hits an instant SHUTDOWN, even in cases where the truncation I expect
to be going on behind the scenes ought to result in sane continued execution.
Furthermore, right from the very outset, the APM Vol2 14.7 "Leaving Long Mode"
section instructs peoples to switch to a compatibility mode segment first
before clearing CR0.PG, which does clear out the upper bits in %rip. This is
further backed up by Vol2 Figure 1-6 "Operating Modes of the AMD64
Architecture".
Either way, this appears to have been a genuine oversight in the AMD64 spec.
Intel, on the other hand, rejects this state transition with #GP.
Between revision 71 (Nov 2019) and 72 (May 2020) of SDM Vol3, a footnote to
4.1.2 "Paging-Mode Enable" was altered from
If CR4.PCIDE= 1, an attempt to clear CR0.PG causes a general-protection
exception (#GP); software should clear CR4.PCIDE before attempting to
disable paging.
to
If the logical processor is in 64-bit mode or if CR4.PCIDE= 1, an attempt to
clear CR0.PG causes a general-protection exception (#GP). Software should
transition to compatibility mode and clear CR4.PCIDE before attempting to
disable paging.
which acknowledges this corner case, but there doesn't appear to be any other
discussion even in the relevant Long Mode sections.
So it appears that Intel spotted and addressed the corner case in IA-32e mode,
but were 15 years late to document it.
Xen was written to the AMD spec, and misses the check. Follow the Intel
behaviour, because it is more sensible and avoids hitting a VMEntry failure.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Exceptions and NPF intercepts almost have the same layout, but NPF has bits
above 31 in the error code, and the name for exitinfo2 really does want
distinguishing between cr2 and gpa.
In nsvm_vcpu_vmexit_inject() rearrange VMEXIT_NPF to fall through instead of
repeating the exitinfo1 write. Use the fallthrough pseudo keyword instead of
a comment.
In VMEXIT_NPF, as we're editing the printk() anyway, switch to using the newer
domain_crash() form.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This removes raw number manipulation, and makes the logic easier to follow.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
... rather than having separate definitions locally. EXC_HAS_EC in particular
is missing #CP, #VC and #SX vs X86_EXC_HAVE_EC.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
We have two naming schemes for exceptions; X86_EXC_?? which use the
archtiectural abbreviations, and TRAP_* which is a mix of terminology and
nonstandard abbrevations. Switch to X86_EXC_* uniformly.
No funcational change, confirmed by diffing the disassembly. Only 7 binary
changes, and they're all __LINE__ being passed into printk().
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
With all code areas updated, drop the temporary defines and adjust all
remaining users.
No practical change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Adjust the API and comments appropriately.
x86_cpu_policy_fill_native() will eventually contain MSR reads, but leave a
TODO in the short term.
No practical change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
With cpuid_policy and msr_policy merged to form cpu_policy, merge the
respective fuzzing logic.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
As with struct domain, retain cpuid as a valid alias for local code clarity.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Switch to the newer cpu_policy nomenclature. Do some easy cleanup of
includes.
No practical change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
These are already getting over-large for being inline functions, and are only
going to grow further over time. Out of line them, yielding the following net
delta from bloat-o-meter:
add/remove: 2/0 grow/shrink: 0/4 up/down: 276/-1877 (-1601)
Switch to the newer cpu_policy terminology while doing so.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
With all the complicated callers of x86_cpu_policies_are_compatible() updated
to use a single cpu_policy object, we can drop the final user of struct
old_cpu_policy.
Update x86_cpu_policies_are_compatible() to take (new) cpu_policy pointers,
reducing the amount of internal pointer chasing, and update all callers to
pass their cpu_policy objects directly.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Right now, they're the same underlying type, containing disjoint information.
Use a single object instead. Also take the opportunity to rename 'entries' to
'msrs' which is more descriptive, and more in line with nr_msrs being the
count of MSR entries in the API.
test-tsx uses xg_private.h to access the internals of xc_cpu_policy, so needs
updating at the same time. Take the opportunity to improve the code clarity
by passing a cpu_policy rather than an xc_cpu_policy into some functions.
No practical change. This undoes the transient doubling of storage space from
earlier patches.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Right now, they're the same underlying type, containing disjoint information.
Drop the d->arch.msr pointer, and union d->arch.cpuid to give it a second name
of cpu_policy in the interim.
Merge init_domain_{cpuid,msr}_policy() into a single init_domain_cpu_policy(),
moving the implementation into cpu-policy.c
No practical change. This undoes the transient doubling of storage space from
earlier patches.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Right now, they're the same underlying type, containing disjoint information.
Introduce a new cpu-policy.{h,c} to be the new location for all policy
handling logic. Place the combined objects in __ro_after_init, which is new
since the original logic was written.
As we're trying to phase out the use of struct old_cpu_policy entirely, rework
update_domain_cpu_policy() to not pointer-chase through system_policies[].
This in turn allows system_policies[] in sysctl.c to become static and reduced
in scope to XEN_SYSCTL_get_cpu_policy.
No practical change. This undoes the transient doubling of storage space from
earlier patches.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
As with the cpuid side, use a temporary define to make struct msr_policy still
work.
Note, this means that domains now have two separate struct cpu_policy
allocations with disjoint information, and system policies are in a similar
position, as well as xc_cpu_policy objects in libxenguest. All of these
duplications will be addressed in the following patches.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Also merge lib/x86/cpuid.h entirely into lib/x86/cpu-policy.h
Use a temporary define to make struct cpuid_policy still work.
There's one forward declaration of struct cpuid_policy in
tools/tests/x86_emulator/x86-emulate.h that isn't covered by the define, and
it's easier to rename that now than to rearrange the includes.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
These weren't great names to begin with, and using {leaves,msrs} matches up
better with the existing nr_{leaves,msr} parameters anyway.
Furthermore, by renaming these fields we can get away with using some #define
trickery to avoid the struct {cpuid,msr}_policy merge needing to happen in a
single changeset.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
We want to merge struct cpuid_policy and struct msr_policy together, and the
result wants to be called struct cpu_policy.
The current struct cpu_policy, being a pair of pointers, isn't terribly
useful. Rename the type to struct old_cpu_policy, but it will disappear
entirely once the merge is complete.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Trying to build the test harness fails with:
x86_emulate/blk.c: In function 'x86_emul_blk':
x86_emulate/blk.c:74:15: error: expected ':' or ')' before 'ASM_FLAG_OUT'
74 | ASM_FLAG_OUT(, "; setz %[zf]")
| ^~~~~~~~~~~~
This is because ASM_FLAG_OUT() is still local to x86_emulate.c. Move it into
x86-emulate.h instead so it ends up in all files including private.h. The
main Xen build gets this macro from compiler.h.
Fixes: c80243f943 ("x86emul: move x86_emul_blk() to separate source file")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Strings in Ocaml carry their own length. Absolutely nothing good can come
from having caml_string_length(data) be different to len.
Use the appropriate accessor, String_val(), but retain the workaround for the
Ocaml -safe-string constness bug in the same way as we've done elsewhere in
Xen.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Christian Lindig <christian.lindig@cloud.com>
* Update from GCC 11.2 to 11.3
* Use python3-minimal instead of python
* Use --no-install-recommends, requiring ca-certificates, g++-multilib and
build-essential to be listed explicitly
The resulting container is ~50M smaller
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
For similar reasons to c/s a6b1e2b80f. While this container is still
build-able for now, all the other problems with explicitly-versioned compilers
remain.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
While we've been diligent to ensure that the main text/data/rodata mappings
have suitable restrictions, their aliases via the directmap were left fully
read/write. Worse, we even had pieces of code making use of this as a
feature.
Restrict the permissions for .text/rodata, as we have no legitimate need for
writeability of these areas via the directmap alias. Note that the
compile-time allocated pagetables do get written through their directmap
alias, so need to remain writeable.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
These two early exits skipped re-enabling the watchdog, restoring the NMI
callback, and clearing the nmi_patch global pointer. Always execute the tail
of the function on the way out.
Fixes: 8dd4dfa92d ("x86/microcode: Synchronize late microcode loading")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
In VMX operation, the handling of INIT IPIs is changed. Instead of the CPU
resetting, the next VMEntry fails with EXIT_REASON_INIT. From the TXT spec,
the intent of this behaviour is so that an entity which cares can scrub
secrets from RAM before participating in an orderly shutdown.
Right now, Xen's behaviour is that when an INIT arrives, the HVM VM which
schedules next is killed (citing an unknown VMExit), *and* we ignore the INIT
and continue blindly onwards anyway.
This patch addresses only the first of these two problems by ignoring the INIT
and continuing without crashing the VM in question.
The second wants addressing too, just as soon as we've figured out something
better to do...
Discovered as collateral damage from when an AP triple faults on S3 resume on
Intel TigerLake platforms.
Link: https://github.com/QubesOS/qubes-issues/issues/7283
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
This reverts commit e1d7508444.
After spending ages sorting out Gitlab CI, it appears that OSSTest too has an
out-of-date Lets Encrypt cert. Revert again in the short term while we fix
this up.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
* Delete trailing whitespace
* Replace an opencoded DIV_ROUND_UP()
* Drop bogus smp_rmb() - spin_lock_irqsave() has full smp_mb() semantics.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
All three of these records have tail padding, leaking stack rubble into the
trace buffer. Introduce an explicit _pad field and have the compiler zero the
padding automatically.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
Four TRC_MEM_* records supply custom structures with tail padding, leaking
stack rubble into the trace buffer. Three of the records were fine in 32-bit
builds of Xen, due to the relaxed alignment of 64-bit integers, but
POD_SUPERPAGE_SPLITER was broken right from the outset.
We could pack the datastructures to remove the padding, but xentrace_format
has no way of rendering the upper half of a 16-bit field. Instead, expand all
16-bit fields to 32-bit.
For POD_SUPERPAGE_SPLINTER, introduce an order field as it is relevant
information, and to match DECREASE_RESERVATION, and so it doesn't require a
__packed attribute to drop tail padding.
Update xenalyze's structures to match, and introduce xentrace_format rendering
which was absent previously.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>
In the case that 'extra' isn't a multiple of uint32_t, the calculation rounds
the number of bytes up, causing later logic to read unrelated bytes beyond the
end of the object.
Also, asserting that the object is within TRACE_EXTRA_MAX, but truncating it
in release builds is rude. Instead, reject any out-of-spec records, leaving
enough of a message to identify the faulty caller.
There is one buggy trace record, TRC_RTDS_BUDGET_BURN. As it must remain
__packed (as cur_budget is misaligned), change bool has_extratime to uint32_t
to compensate.
It turns out that the new printk() can also be hit by HVMOP_xentrace, because
the hypercall is broken. It cannot be used outside of custom debugging, as
none of the tooling was ever updated to understand TRC_GUEST, nor is there any
evidence of hypercall ever being used in public.
While the hypercall was clearly intended to be used with units if uint32_t's,
that's not how the API/ABI works - Xen will in fact read the entire structure
rather than the initialised subset out of guest memory (most likely, stack
rubble), then copy up to 3 bytes of it (rounding up to the next uint32_t) into
the real tracebuffer.
There are several possible ways to fix this, but as the hypercall, and does
not plausibly have any users, go with the one that is least logic in Xen, by
rejecting tracing attempts that are not of uint32_t size.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Partly for clarity because there is a lot of subtle magic at work here.
Expand the commentary of what's going on.
Also because there is no need to double copy the stack (32kB). Spilled
content does need accounting for, but this can be sorted by only copying only
a handful of words.
No practical change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Gitlab Randconfig found:
arch/x86/mm/shadow/common.c: In function 'shadow_prealloc':
arch/x86/mm/shadow/common.c:1023:18: error: implicit declaration of function
'paging_logdirty_levels'; did you mean 'paging_log_dirty_init'? [-Werror=implicit-function-declaration]
1023 | count += paging_logdirty_levels();
| ^~~~~~~~~~~~~~~~~~~~~~
| paging_log_dirty_init
arch/x86/mm/shadow/common.c:1023:18: error: nested extern declaration of 'paging_logdirty_levels' [-Werror=nested-externs]
The '#if PG_log_dirty' expression is currently SHADOW_PAGING && !HVM &&
PV_SHIM_EXCLUSIVE. Move the declaration outside.
Fixes: 33fb3a6612 ("x86/shadow: account for log-dirty mode when pre-allocating")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Since c/s 73b13705af ("firmware: provide a stand alone set of headers"),
we've had a proper stddef.h. Actually use it.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
HVMLoader is not relocatable in memory, and 32bit PIC code has a large
overhead. Override the compilers choice of pic/no-pic and force it to be
non-relocatable.
Bloat-o-meter reports a net:
add/remove: 0/0 grow/shrink: 3/107 up/down: 14/-3370 (-3356)
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
There are two problems with domain_crash().
First, that it is frequently not preceded by a printk() at all, or only by a
dprintk(). Either way, critical diagnostic information is missing for an
event which is fatal to the guest.
Second, the embedded __LINE__ is an issue for livepatching, creating unwanted
churn in the binary diff. This is the final __LINE__ remaining in
livepatching-relevant contexts.
The end goal is to have domain_crash() require a print string which gets fed
to printk(), making it far less easy to omit relevant diagnostic information.
However, modifying all callers at once is far too big and complicated, so use
some macro magic to tolerate the old API (no print string) in the short term.
Adjust two callers in load_segments() to demonstrate the new API.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
It turns out that evaluate_nospec() code generation is not safe under Clang.
Given:
void eval_nospec_test(int x)
{
if ( evaluate_nospec(x) )
asm volatile ("nop #true" ::: "memory");
else
asm volatile ("nop #false" ::: "memory");
}
Clang emits:
<eval_nospec_test>:
0f ae e8 lfence
85 ff test %edi,%edi
74 02 je <eval_nospec_test+0x9>
90 nop
c3 ret
90 nop
c3 ret
which is not safe because the lfence has been hoisted above the conditional
jump. Clang concludes that both barrier_nospec_true()'s have identical side
effects and can safely be merged.
Clang can be persuaded that the side effects are different if there are
different comments in the asm blocks. This is fragile, but no more fragile
that other aspects of this construct.
Introduce barrier_nospec_false() with a separate internal comment to prevent
Clang merging it with barrier_nospec_true() despite the otherwise-identical
content. The generated code now becomes:
<eval_nospec_test>:
85 ff test %edi,%edi
74 05 je <eval_nospec_test+0x9>
0f ae e8 lfence
90 nop
c3 ret
0f ae e8 lfence
90 nop
c3 ret
which has the correct number of lfence's, and in the correct place.
Link: https://github.com/llvm/llvm-project/issues/55084
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
We shouldn't be using two struct iovec's to write half of 'rec' each, and
there is no need to malloc() for two struct iovec's at all.
Simplify down to just two - one covering the whole of 'rec', and one covering
the pfns array.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Olaf Hering <olaf@aepfle.de>
OpenSBI already expands \n to \r\n. Don't repeat the expansion, as it doubles
the size of the resulting log with every other line being blank.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Rewrite argo_dprintk() so printk() format typechecking can always be
performed. This also fixes the fact that parameters are not evaulated at all
in the default case.
Emit the messages at XENLOG_DEBUG.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
As stated (correctly) by the comment next to SPEC_CTRL_ENTRY_FROM_PV, between
the two hunks visible in the patch, RET's are not safe prior to this point.
CR4_PV32_RESTORE hides a CALL/RET pair in certain configurations (PV32
compiled in, SMEP or SMAP active), and the RET can be attacked with one of
several known speculative issues.
Furthermore, CR4_PV32_RESTORE also hides a reference to the cr4_pv32_mask
global variable, which is not safe when XPTI is active before restoring Xen's
full pagetables.
This crash has gone unnoticed because it is only AMD CPUs which permit the
SYSCALL instruction in compatibility mode, and these are not vulnerable to
Meltdown so don't activate XPTI by default.
This is XSA-429 / CVE-2022-42331
Fixes: 5e79629011 ("x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point")
Fixes: 5784de3e20 ("x86: Meltdown band-aid against malicious 64-bit PV guests")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This removes most of the opencoded bit logic on the exit qualification.
Unfortunately, size is 1-based not 0-based, so need adjusting in a separate
variable.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This is mandated by the Fedora packaging guidelines because it is a security
vulnerability otherwise in suid scripts. While Xen doesn't have suid scripts,
it's a very good idea generally because it prevents the users local python
environment interfering from system packaged scripts.
pygrub is the odd-script-out, being installed by distutils rather than
manually with INSTALL_PYTHON_PROG. distutils has no nice way of editing the
shebang, so arrange to use INSTALL_PYTHON_PROG for pygrub too.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
* Add X86_{CPUID,MSR}_POLICY_FORMAT checks which were missed previously.
* Drop test_suite(). It hasn't been necessary since the Py2.3 era.
* Drop the __main__ logic. This can't be used without manually adjusting the
include path, and `make test` knows how to do the right thing.
* For `make test`, use `-v` to see which tests have been discovered and run.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
This script is not python3 compatible, but has its shebang altered to say
python3 by INSTALL_PYTHON_PROG.
The most recent reference I can find to this script (which isn't incidental
adjustments in the makefile) is from the Xen book, fileish 561e30b804 which
says
%% <snip> Alternatively, if the
%% Xen machine is connected to a serial-port server then we supply a
%% dumb TCP terminal client, {\tt xencons}.
So this a not-invented-here version of telnet. Delete it.
Resolves: xen-project/xen#159
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
This removes raw number manipulation, and makes the logic easier to follow.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Featuresets are supposed to be disappearing when the CPU policy infrastructure
is complete, but that has taken longer than expected, and isn't going to be
complete imminently either.
In the meantime, Xen does have proper default/max featuresets, and xen-cpuid
can even get them via the XEN_SYSCTL_cpu_policy_* interface, but only knows
now to render them nicely via the featureset interface.
Differences between default and max are a frequent source of errors,
frequently too in secret leading up to an embargo, so extend the featureset
sysctl to allow xen-cpuid to render them all nicely.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Christian Lindig <christian.lindig@cloud.com>
struct fsinfo is the vestigial remnant of an older internal design which
didn't survive very long.
Simplify things by inlining get_featureset() and having a single memory
allocation that gets reused. This in turn changes featuresets[] to be a
simple list of names, so rename it to fs_names[].
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Xapi chose to render its featureset bitmaps with dashes rather than colons as
a separator. Have xen-cpuid support both forms, for convenience.
Fix a trivial style issue in the vicinity while here.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
This was an oversight when adding the Xen parts.
Fixes: cea9ae0622 ("x86/spec-ctrl: Enumeration for new Intel BHI controls")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Edwin, with the help of GCC's -fanalyzer, identified that p2m_frame_list_list
gets leaked. What fanalyzer can't see is that the live_p2m_frame_list_list
and live_p2m_frame_list foreign mappings are leaked too.
Rework the logic so the out path is executed unconditionally, which cleans up
all the intermediate allocations/mappings appropriately.
Fixes: bd7a29c3d0 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support linear p2m table")
Reported-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Use a single fairly generic string as the "all done" message to look for,
which avoids the need to patch qemu-smoke-riscv64.sh each time a new feature
is added.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
struct nestedvm uses mostly plain integer types, except for virt_ext_t which
is a union wrapping two bitfield names.
However, it turns out that this is a write-only variable. Delete it, allowing
us to drop the include of vmcb.h
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
These days, this is just two length helpers. Move into the private svm.h
No functional change intended.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Xenia Ragiadakou <burzalodowa@gmail.com>
Fold svm_asid_g_invlpg() into its single caller, deleting the #if 0 which has
been present for the entire 16 years this helper has existed: c/s 322a078ab1
"[HVM][SVM] Reintroduce ASIDs."
Move the two remaining prototypes into the private svm.h, forward declaring
cpuinfo_x86 in preference to including asm/processor.h
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Xenia Ragiadakou <burzalodowa@gmail.com>
This is generally good practice, and necessary for livepatch binary diffing to
work.
With this, livepatching of the SVM entry path works. The only complication is
with svm_stgi_label which is only used by oprofile to guestimate (not
completely correctly) when an NMI hit guest context.
Livepatching of VMX is still an open question, because the logic doesn't form
anything remotely resembling functions. Both code fragments jump into each
other so need to be updated in tandem. Also, both code fragment entries need
trampolines in the case that patching actually occurs. For now, just treat it
as a single function.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Taking struct cpu_user_regs as a full object is bogus, and while what was
probably meant was to take a struct cpu_user_regs pointer, that's still wrong.
This isn't a function; its an address stored in the VMCS that the CPU resumes
from on VMExit, meaning that it doesn't conform to a normal C API/ABI.
Annotate it with `nocall`, and move the declaration into vmcs.c next to its
sole user.
Fixes: 9c3118a825 ("bitkeeper revision 1.1159.1.483 (41c0c417XYObowWqbfqU0cdLx30C9w)")
`-> "Initial Intel VMX support"
Reported-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
While trying to work around a different Clang-IAS bug (parent changeset), I
stumbled onto:
In file included from arch/x86/asm-macros.c:3:
./arch/x86/include/asm/spec_ctrl_asm.h:144:19: error: \u used with
no following hex digits; treating as '\' followed by identifier [-Werror,-Wunicode]
.L\@_fill_rsb_loop\uniq:
^
It turns out that Clang -E is sensitive to the file extension of the source
file it is processing. Furthermore, C explicitly permits the use of \u
escapes in identifier names, so the diagnostic would be reasonable in
principle if we trying to compile the result.
asm-macros should really have been .S from the outset, as it is ultimately
generating assembly, not C. Rename it, which causes Clang not to complain.
We need to introduce rules for generating a .i file from .S, and substituting
c_flags for a_flags lets us drop the now-redundant -D__ASSEMBLY__.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
https://github.com/llvm/llvm-project/issues/60792
It turns out that Clang-IAS does not expand \@ uniquely in a translaition
unit, and the XSA-426 change tickles this bug:
<instantiation>:4:1: error: invalid symbol redefinition
.L1_fill_rsb_loop:
^
make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1
Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mix %= in
too, which Clang does seem to expand properly.
Fixes: 63305e5392 ("x86/spec-ctrl: Mitigate Cross-Thread Return Address Predictions")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Also print the compiler version before starting. It's not easy to find
otherwise, and does change from time to time.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
It is easier just to make the functions void and let the compiler do the (not
very) hard work.
Passing regs is a bit weird for HVM guests anyway, because the resulting
pointer is invariant (this isn't native exception handling where the regs
pointers *are* important), and all functions calculate current themselves
which is another invariant.
Finally, the compiler can merge the get_cpu_info() calculation which is common
to both current and guest_cpu_user_regs(), meaning the delta in C really is
just one LEA, and not any more expensive than MOV's in ASM anyway.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Clang-15 (as seen in the FreeBSD 14 tests) complains:
xg_main.c:1248 error: a function declaration without a
prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
xg_init()
^
void
The error message is a bit confusing but appears to new as part of
-Wdeprecated-non-prototype which is part of supporting C2x which formally
removes K&R syntax.
Either way, fix the identified function.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Clang-15 (as seen in the FreeBSD 14 tests) complains:
arch/x86/time.c:1364:20: error: a function declaration without a
prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
s_time_t get_s_time()
^
void
The error message is a bit confusing but appears to new as part of
-Wdeprecated-non-prototype which is part of supporting C2x which formally
removes K&R syntax.
Either way, fix the identified functions.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Refer to Intel SDM Rev 70 (Dec 2022), Vol3 17.2.3 "Supervisor Shadow Stack
Token".
Architecturally, an event delivery which starts in CPL<3 and switches shadow
stack will first validate the Supervisor Shadow Stack Token (setting the busy
bit), then pushes CS/LIP/SSP. One example of this is an NMI interrupting Xen.
Some CPUs suffer from an issue called fracturing, whereby a fault/vmexit/etc
between setting the busy bit and completing the event injection renders the
action non-restartable, because when it comes time to restart, the busy bit is
found to be already set.
This is far more easily encountered under virt, yet it is not the fault of the
hypervisor, nor the fault of the guest kernel. The fault lies somewhere
between the architectural specification, and the uarch behaviour.
Intel have allocated CPUID.7[1].ecx[18] CET_SSS to enumerate that supervisor
shadow stacks are safe to use. Because of how Xen lays out its shadow stacks,
fracturing is not expected to be a problem on native.
Detect this case on boot and default to not using shstk if virtualised.
Specifying `cet=shstk` on the command line will override this heuristic and
enable shadow stacks irrespective.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
We don't actually need ecx yet, but adding it in now will reduce the amount to
which leaf 7 is out of order in a featureset.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
All caml_alloc_*() functions can throw exceptions, and longjump out of
context. If this happens, we leak the xch/xce handle.
Reorder the logic to allocate the the Ocaml object first.
Fixes: 8b3c06a3e5 ("tools/ocaml/xenctrl: OCaml 5 support, fix use-after-free")
Fixes: 22d5affdf0 ("tools/ocaml/evtchn: OCaml 5 support, fix potential resource leak")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
The intf->{addr,len} references in the xc_map_foreign_range() call are unsafe.
From the manual:
https://ocaml.org/manual/intfc.html#ss:parallel-execution-long-running-c-code
"After caml_release_runtime_system() was called and until
caml_acquire_runtime_system() is called, the C code must not access any OCaml
data, nor call any function of the run-time system, nor call back into OCaml
code."
More than what the manual says, the intf pointer is (potentially) invalidated
by caml_enter_blocking_section() if another thread happens to perform garbage
collection at just the right (wrong) moment.
Rewrite the logic. There's no need to stash data in the Ocaml object until
the success path at the very end.
Fixes: 8b7ce06a2d ("ocaml: Add XC bindings.")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
* Remove local integers when all we're returning is Val_int() of another
variable. The CAMLlocal*() can't be optimised automatically, as it's
registered with the GC.
* Rename "virq_type" to "virq" and "_port" to "port".
* In stub_eventchn_pending(), rename 'port' to 'rc', to be consistent with
all other stubs that return xenevtchn_port_or_error_t.
* In stub_eventchn_unmask(), check for rc == -1 to be consistent with all
other stubs.
No practical change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
caml_alloc() takes units of Wsize (word size), not bytes. As a consequence,
we're allocating 4 or 8 times too much memory.
Ocaml has a helper, Wsize_bsize(), but it truncates cases which aren't an
exact multiple. Use a BUILD_BUG_ON() to cover the potential for truncation,
as there's no rounding-up form of the helper.
Fixes: 8b7ce06a2d ("ocaml: Add XC bindings.")
Fixes: d3e649277a ("ocaml: add mmap bindings implementation.")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
The OSSTest bisector identified an issue with c/s 1894049fa2 ("x86/shadow:
L2H shadow type is PV32-only") in !HVM builds.
The bug is ultimately caused by sh_type_to_size[] not actually being specific
to HVM guests, and it's position in shadow/hvm.c mislead the reasoning.
To fix the issue that OSSTest identified, SH_type_l2h_64_shadow must still
have the value 1 in any CONFIG_PV32 build. But simply adjusting this leaves
us with misleading logic, and a reasonable chance of making a related error
again in the future.
In hindsight, moving sh_type_to_size[] out of common.c in the first place a
mistake. Therefore, move sh_type_to_size[] back to living in common.c,
leaving a comment explaining why it happens to be inside an HVM conditional.
This effectively reverts the second half of 4fec945409 ("x86/shadow: adjust
and move sh_type_to_size[]") while retaining the other improvements from the
same changeset.
While making this change, also adjust the sh_type_to_size[] declaration to
match its definition.
Fixes: 4fec945409 ("x86/shadow: adjust and move sh_type_to_size[]")
Fixes: 1894049fa2 ("x86/shadow: L2H shadow type is PV32-only")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: George Dunlap <george.dunlap@cloud.com>
With all infrastructure in place, advertise the PKS CPUID bit to HAP guests,
and let them set CR4.PKS.
Experiment with a tweak to the layout of hvm_cr4_guest_valid_bits() so future
additions will be just a single added line.
The current context switching behaviour is tied to how VT-x works, so leave a
safety check in the short term.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
PKS is very similar to the existing PKU behaviour, operating on pagewalks for
any supervisor mapping.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Have guest_{rd,wr}msr(), via hvm_{get,set}_reg(), access either the live
register, or stashed state, depending on context. Include MSR_PKRS for
migration, and let the guest have full access.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Under PKS, MSR_PKRS is available and based on the CPUID policy alone, and
usable independently of CR4.PKS. See the large comment in prot-key.h for
details of the context switching arrangement.
Use WRMSRNS right away, as we don't care about serialsing properties for
context switching this MSR.
Sanitise MSR_PKRS on boot. In anticipation of wanting to use PKS for Xen in
the future, arrange for the sanitisation to occur prior to potentially setting
CR4.PKS; if PKEY0.{AD,WD} leak in from a previous context, we will triple
fault immediately on setting CR4.PKS.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <JBeulich@suse.com>
WRMSR Non-Serialising is an optimisation intended for cases where an MSR needs
updating, but architectural serialising properties are not needed.
In is anticipated that this will apply to most if not all MSRs modified on
context switch paths.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
asm/processor.h is in desperate need of splitting up, and protection key
functionality in only used in the emulator and pagewalk. Introduce a new
asm/prot-key.h and move the relevant content over.
Rename the PKRU_* constants to drop the user part and to use the architectural
terminology.
Drop the read_pkru_{ad,wd}() helpers entirely. The pkru infix is about to
become wrong, and the sole user is shorter and easier to follow without the
helpers.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Protection Key Supervisor works in a very similar way to Protection Key User,
except that instead of a PKRU register used by the {RD,WR}PKRU instructions,
the supervisor protection settings live in MSR_PKRS and is accessed using
normal {RD,WR}MSR instructions.
PKS has the same problematic interactions with PV guests as PKU (more infact,
given the guest kernel's CPL), so we'll only support this for HVM guests for
now.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Andrew Cooper