Commit f2bebf72c4 "xenstore: rework of transaction handling" moved
out from copying the entire database everytime a new transaction is
opened to track the list of nodes changed.
The content of all the nodes accessed during a transaction will be
temporarily stored in TDB using a different key.
The function create_node() may write/update multiple nodes if the child
doesn't exist. In case of a failure, the function will revert any
changes (this include any update to TDB). Unfortunately, the function
which reverts the changes (i.e. destroy_node()) will not use the correct
key to delete any update or even request the transaction to fail.
This means that if a client decide to go ahead with committing the
transaction, orphan nodes will be created because they were not linked
to an existing node (create_node() will write the nodes backwards).
Once some nodes have been partially updated in a transaction, it is not
easily possible to undo any changes. So rather than continuing and hit
weird issue while committing, it is much saner to fail the transaction.
This will have an impact on any client that decides to commit even if it
can't write a node. Although, it is not clear why a normal client would
want to do that...
Lastly, update destroy_node() to use the correct key for deleting the
node. Rather than recreating it (this will allocate memory and
therefore fail), stash the key in the structure node.
This is XSA-415 / CVE-2022-42310.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
XSA-115 extended destroy_node() to update the node accounting for the
connection. The implementation is assuming the connection is the parent
of the node, however all the nodes are allocated using a separate context
(see process_message()). This will result to crash (or corrupt) xenstored
as the pointer is wrongly used.
In case of an error, any changes to the database or update to the
accounting will now be reverted in create_node() by calling directly
destroy_node(). This has the nice advantage to remove the loop to unset
the destructors in case of success.
Take the opportunity to free the nodes right now as they are not
going to be reachable (the function returns NULL) and are just wasting
resources.
This is XSA-414 / CVE-2022-42309.
Fixes: 0bfb2101f2 ("tools/xenstore: fix node accounting after failed node creation")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
The list p2m->pages contain all the pages used by the P2M. On large
instance this can be quite large and the time spent to call
d->arch.paging.free_page() will take more than 1ms for a 80GB guest
on a Xen running in nested environment on a c5.metal.
By extrapolation, it would take > 100ms for a 8TB guest (what we
current security support). So add some preemption in p2m_teardown()
and propagate to the callers. Note there are 3 places where
the preemption is not enabled:
- hap_final_teardown()/shadow_final_teardown(): We are
preventing update the P2M once the domain is dying (so
no more pages could be allocated) and most of the P2M pages
will be freed in preemptive manneer when relinquishing the
resources. So this is fine to disable preemption.
- shadow_enable(): This is fine because it will undo the allocation
that may have been made by p2m_alloc_table() (so only the root
page table).
The preemption is arbitrarily checked every 1024 iterations.
We now need to include <xen/event.h> in p2m-basic in order to
import the definition for local_events_need_delivery() used by
general_preempt_check(). Ideally, the inclusion should happen in
xen/sched.h but it opened a can of worms.
Note that with the current approach, Xen doesn't keep track on whether
the alt/nested P2Ms have been cleared. So there are some redundant work.
However, this is not expected to incurr too much overhead (the P2M lock
shouldn't be contended during teardown). So this is optimization is
left outside of the security event.
This is part of CVE-2022-33746 / XSA-410.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
----
Changes since v12:
- Correct altp2m preemption check placement.
Changes since v9:
- Integrate patch into series.
Changes since v2:
- Rework the loop doing the preemption
- Add a comment in shadow_enable() to explain why p2m_teardown()
doesn't need to be preemptible.
Changes since v1:
- Update the commit message
- Rebase on top of Roger's v8 series
- Fix preemption check
- Use 'unsigned int' rather than 'unsigned long' for the counter
At the moment the P2M page tables will be freed when the domain structure
is freed without any preemption. As the P2M is quite large, iterating
through this may take more time than it is reasonable without intermediate
preemption (to run softirqs and perhaps scheduler).
Split p2m_teardown() in two parts: one preemptible and called when
relinquishing the resources, the other one non-preemptible and called
when freeing the domain structure.
As we are now freeing the P2M pages early, we also need to prevent
further allocation if someone call p2m_set_entry() past p2m_teardown()
(I wasn't able to prove this will never happen). This is done by
the checking domain->is_dying from previous patch in p2m_set_entry().
Similarly, we want to make sure that no-one can accessed the free
pages. Therefore the root is cleared before freeing pages.
This is part of CVE-2022-33746 / XSA-410.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Tested-by: Henry Wang <Henry.Wang@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
During the domain destroy process, the domain will still be accessible
until it is fully destroyed. So does the P2M because we don't bail
out early if is_dying is non-zero. If a domain has permission to
modify the other domain's P2M (i.e. dom0, or a stubdomain), then
foreign mapping can be added past relinquish_p2m_mapping().
Therefore, we need to prevent mapping to be added when the domain
is dying. This commit prevents such adding of mapping by adding the
d->is_dying check to p2m_set_entry(). Also this commit enhances the
check in relinquish_p2m_mapping() to make sure that no mappings can
be added in the P2M after the P2M lock is released.
This is part of CVE-2022-33746 / XSA-410.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Tested-by: Henry Wang <Henry.Wang@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Since commit 01280dc19c "evtchn: simplify port_is_valid()", the event
channels code assumes that all the buckets below d->valid_evtchns are
always allocated.
This assumption hold in most of the situation because a guest is not
allowed to chose the port. Instead, it will be the first free from port
0.
When static event channel support will be added for dom0less domains
user can request to allocate the evtchn port numbers that are scattered
in nature.
The existing implementation of evtchn_allocate_port() is not able to
deal with such situation and will end up to override bucket or/and leave
some bucket unallocated. The latter will result to a droplet crash if
the event channel belongs to an unallocated bucket.
This can be solved by making sure that all the buckets below
d->valid_evtchns are allocated. There should be no impact for most of
the situation but LM/LU as only one bucket would be allocated. For
LM/LU, we may end up to allocate multiple buckets if ports in use are
sparse.
A potential alternative is to check that the bucket is valid in
is_port_valid(). This should still possible to do it without taking
per-domain lock but will result a couple more of memory access.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Unlike arm64, on arm32 there are no extra information dumped (e.g.
page table walk) for hypervisor data abort.
For data abort, the HSR will be set properly and so call
do_trap_hyp_sync() instead of do_unexpected_trap() on arm32 to have
the print the same information as arm64.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Currently the output is looking like:
(XEN) 1ST[0x1] = 0x000000004015ff7f
(XEN) 2ND[0x1f] = 0x00500000bfe00f7d
The content of the entries are not aligned making a bit trickier to
read (I appreciate this is a matter of taste).
Align the values by forcing the index to be always printed using
3 characters (enough to cover 512 in hexadecimal).
New output:
(XEN) 1ST[0x001] = 0x000000004015ff7f
(XEN) 2ND[0x01f] = 0x00500000bfe00f7d
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Reviewed-by: Henry Wang <Henry.Wang@arm.com>
At the moment, the strings are in text right after each use because
the instruction 'adr' has specific requirement on the location
and the compiler will forbid cross section label.
The macro 'adr_l' was recently reworked so the caller doesn't need
to know whether the MMU is on. This makes it easier to use where
instructions can be run in both context.
This also means that the strings don't need to be part of .text
anymore. So move them to .rodata.str.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Jiamei Xie <jiamei.xie@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Tested-by: Bertrand Marquis <bertrand.marquis@arm.com>
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
At the moment, the macro addr_l needs to know whether the caller
is running with the MMU on. This is fine today because there are
only two possible cases:
1) MMU off
2) MMU on and linked to the virtual address
This is still cumbersome to use for the developer as they need
to know if the MMU is on.
Thankfully, Linux developpers came up with a great way to allow
adr_l to work within the range +/- 4GB of PC by emitting a PC-relative
reference [1].
Re-use the same approach on Arm and drop the parameter 'mmu'.
[1] 0b1674638a5c ("ARM: assembler: introduce adr_l, ldr_l and str_l macros")
Signed-off-by: Julien Grall <jgrall@amazon.com>
There are a few places in the code that need to find the slot at a
given page-table level.
So create a new macro get_table_slot() for that. This will reduce
the effort to figure out whether the code is doing the right thing.
The new macro is using 'ubfx' (or 'lsr' for the first level) rather
than the existing sequence (mov_w, lsr, and) because it doesn't require
a scratch register and reduce the number of instructions (4 -> 1).
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Tested-by: Bertrand Marquis <bertrand.marquis@arm.com>
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
There are a few places in the code that need to find the slot
at a given page-table level.
So create a new macro get_table_slot() for that. This will reduce
the effort to figure out whether the code is doing the right thing.
Take the opportunity to use 'ubfx'. The only benefits is reducing
the number of instructions from 2 to 1.
The new macro is used everywhere we need to compute the slot. This
requires to tweak the parameter of create_table_entry() to pass
a level rather than shift.
Note, for slot 0 the code is currently skipping the masking part. While
this is fine, it is safer to mask it as technically slot 0 only covers
bit 48 - 39 bit (assuming 4KB page granularity).
Take the opportunity to correct the comment when finding the second
slot for the identity mapping (we are computing the second slot
rather than first).
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Since commit 7e14a47e7c ("xen/arm64: head Rework and document
launch()"), the boot code is setting x22 but not read it.
So remove the two instructions setting x22 and update the documentation
to show x22 has no specific purpose.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
__ro_after_init was introduced recently to prevent modifying
some variables after init.
At the moment, on Arm, the variables will still be accessible
because the region permission is not updated.
Address that by moving the sections .data.ro_after_init
out of .data and then map the region read-only once we finish
to boot.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Penny Zheng <penny.zheng@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Tested-by: Henry Wang <Henry.Wang@arm.com>
At the moment, xen_second is used to cover the first 2GB of the
virtual address space. With the recent rework of the page-tables,
only the first 1GB region (where Xen resides) is effectively used.
In addition to that, I would like to reshuffle the memory layout.
So Xen mappings may not be anymore in the first 2GB of the virtual
address space.
Therefore, rework xen_second so it only covers the 1GB region where
Xen will reside.
With this change, xen_second doesn't cover anymore the xenheap area
on arm32. So, we first need to add memory to the boot allocator before
setting up the xenheap mappings.
Take the opportunity to update the comments on top of xen_fixmap and
xen_xenmap.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
The file xen/arch/mm.c has been growing quite a lot. It now contains
various independent part of the MM subsytem.
One of them is the helpers to map/unmap a page which is only used
by arm32 and protected by CONFIG_ARCH_MAP_DOMAIN_PAGE. Move them in a
new file xen/arch/arm/domain_page.c.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
move it to Kconfig.
The define CONFIG_DOMAIN_PAGE indicates whether the architecture provide
helpers to map/unmap a domain page. Rename it to CONFIG_ARCH_MAP_DOMAIN_PAGE
so it is clearer that support for domain page is not something that
can be disabled in Xen.
Take the opportunity to move CONFIG_MAP_DOMAIN_PAGE to Kconfig as this
will soon be necessary to use it in the Makefile.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> #arm part
At the moment, the domheap mappings initialization is done separately for
the boot CPU and secondary CPUs. The main difference is for the former
the pages are part of Xen binary whilst for the latter they are
dynamically allocated.
It would be good to have a single helper so it is easier to rework
on the domheap is initialized.
For CPU0, we still need to use pre-allocated pages because the
allocators may use domain_map_page(), so we need to have the domheap
area ready first. But we can still delay the initialization to setup_mm().
Introduce a new helper init_domheap_mappings() that will be called
from setup_mm() for the boot CPU and from init_secondary_pagetables()
for secondary CPUs.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.com>
At the moment, *_VIRT_END may either point to the address after the end
or the last address of the region.
The lack of consistency make quite difficult to reason with them.
Furthermore, there is a risk of overflow in the case where the address
points past to the end. I am not aware of any cases, so this is only a
latent bug.
Start to solve the problem by removing all the *_VIRT_END exclusively used
by the Arm code and add *_VIRT_SIZE when it is not present.
Take the opportunity to rename BOOT_FDT_SLOT_SIZE to BOOT_FDT_VIRT_SIZE
for better consistency and use _AT(vaddr_t, ).
Also take the opportunity to fix the coding style of the comment touched
in mm.c.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-By: Luca Fancellu <luca.fancellu@arm.com>
Both destroy_xen_mappings() and modify_xen_mappings() will take in
parameter a range [start, end[. Both end should be page aligned.
Add extra ASSERT() to ensure start and end are page aligned. Take the
opportunity to rename 'v' to 's' to be consistent with the other helper.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
At the moment, init_heap_pages() will call free_heap_pages() page
by page. To reduce the time to initialize the heap, we will want
to provide multiple pages at the same time.
init_heap_pages() is now split in two parts:
- init_heap_pages(): will break down the range in multiple set
of contiguous pages. For now, the criteria is the pages should
belong to the same NUMA node.
- _init_heap_pages(): will initialize a set of pages belonging to
the same NUMA node. In a follow-up patch, new requirements will
be added (e.g. pages should belong to the same zone). For now the
pages are still passed one by one to free_heap_pages().
Note that the comment before init_heap_pages() is heavily outdated and
does not reflect the current code. So update it.
This patch is a merge/rework of patches from David Woodhouse and
Hongyan Xia.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
init_heap_pages() is using an open-code version of IS_ALIGNED(). Replace
it to improve the readability of the code.
No functional change intended.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Both destroy_xen_mappings() and modify_xen_mappings() will take in
parameter a range [start, end[. Both end should be page aligned.
Add extra ASSERT() to ensure start and end are page aligned. Take the
opportunity to rename 'v' to 's' to be consistent with the other helper.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
----
Changes in v2:
- Also modify prototype. Note that on x86, the first parameter
was not matching in the declaration and prototype.
- Add Bertrand's reviewed-by
Write to SCTLR_EL2/HSCTLR may not be visible until the next context
synchronization. When initializing the CPU, we want the update to take
effect right now. So add an isb afterwards.
Spec references:
- AArch64: D13.1.2 ARM DDI 0406C.d
- AArch32 v8: G8.1.2 ARM DDI 0406C.d
- AArch32 v7: B5.6.3 ARM DDI 0406C.d
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
A lot of places in the ARM32 assembly requires to load the physical address
of a symbol. Rather than open-coding the translation, introduce a new macro
that will load the phyiscal address of a symbol.
Lastly, use the new macro to replace all the current open-coded version.
Note that most of the comments associated to the code changed have been
removed because the code is now self-explanatory.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Some tools (e.g. xenstored) always expect EINVAL to be first in xsd_errors.
To be conservative, mandate that new errors should be added at the end
of the array.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Henry Wang <Henry.Wang@arm.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
C Xenstored is using EPERM when the client is not allowed to change
the owner (see GET_PERMS). However, the xenstore protocol doesn't
describe EPERM so EINVAL will be sent to the client.
When writing test, it would be useful to differentiate between EINVAL
(e.g. parsing error) and EPERM (i.e. no permission). So extend
xsd_errors[] to support return EPERM.
Looking at previous time xsd_errors was extended (8b2c441a1b), it was
considered to be safe to add a new error because at least Linux driver
and libxenstore treat an unknown error code as EINVAL.
This statement doesn't cover other possible OSes, however I am not
aware of any breakage.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Commit 5047cd1d5d "xen/common: Use enhanced ASSERT_ALLOC_CONTEXT in
xmalloc()" extended the checks in _xmalloc() to catch any use of the
helpers from context with interrupts disabled.
Unfortunately, the rule is not followed when initializing the per-CPU
IRQs:
(XEN) Xen call trace:
(XEN) [<002389f4>] _xmalloc+0xfc/0x314 (PC)
(XEN) [<00000000>] 00000000 (LR)
(XEN) [<0021a7c4>] init_one_irq_desc+0x48/0xd0
(XEN) [<002807a8>] irq.c#init_local_irq_data+0x48/0xa4
(XEN) [<00280834>] init_secondary_IRQ+0x10/0x2c
(XEN) [<00288fa4>] start_secondary+0x194/0x274
(XEN) [<40010170>] 40010170
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 2:
(XEN) Assertion '!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 1)' failed at common/xmalloc_tlsf.c:601
(XEN) ****************************************
This is happening because zalloc_cpumask_var() may allocate memory
if NR_CPUS is > 2 * sizeof(unsigned long).
Avoid the problem by initializing the per-CPU IRQs while preparing the
CPU.
This also has the benefit to remove a BUG_ON() in the secondary CPU
code.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Tested-by: Bertrand Marquis <bertrand.marquis@arm.com>
At the moment, corrupt() is neither checking for allocation failure
nor freeing the allocated memory.
Harden the code by printing ENOMEM if the allocation failed and
free 'str' after the last use.
This is not considered to be a security issue because corrupt() should
only be called when Xenstored thinks the database is corrupted. Note
that the trigger (i.e. a guest reliably provoking the call) would be
a security issue.
Fixes: 06d17943f0 ("Added a basic integrity checker, and some basic ability to recover from store")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Commit 5047cd1d5d "xen/common: Use enhanced ASSERT_ALLOC_CONTEXT in
xmalloc()" extended the checks in _xmalloc() to catch any use of the
helpers from context with interrupts disabled.
Unfortunately, the rule is not followed when allocating the CPU
sibling/core maps.
(XEN) Xen call trace:
(XEN) [<00238a5c>] _xmalloc+0xfc/0x314 (PC)
(XEN) [<00000000>] 00000000 (LR)
(XEN) [<00238c8c>] _xzalloc+0x18/0x4c
(XEN) [<00288cb4>] smpboot.c#setup_cpu_sibling_map+0x38/0x138
(XEN) [<00289024>] start_secondary+0x1b4/0x270
(XEN) [<40010170>] 40010170
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 2:
(XEN) Assertion '!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 1)' failed at common/xmalloc_tlsf.c:601
(XEN) ****************************************
This is happening because zalloc_cpumask_var() may allocate memory
if NR_CPUS is > 2 * sizeof(unsigned long).
Avoid the problem by allocating the CPU sibling/core maps while
preparing the CPU.
This also has the benefit to remove a panic() in the secondary CPU
code.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Currently, memory is added to the boot allocator after the xenheap
mappings are done. This will break if the first mapping is more than
512GB of RAM.
In addition to that, a follow-up patch will rework setup_xenheap_mappings()
to use smaller mappings (e.g. 2MB, 4KB). So it will be necessary to have
memory in the boot allocator earlier.
Only free memory (e.g. not reserved or modules) can be added to the boot
allocator. It might be possible that some regions (including the first
one) will have no free memory.
So we need to add all the free memory to the boot allocator first
and then add do the mappings.
Populating the boot allocator is nearly the same between arm32 and
arm64. The only difference is on the former we need to exclude the
xenheap for the boot allocator. Gate the difference with CONFIG_ARM_32
so the code be re-used on arm64.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
In a follow-up patch, we will want to populate the boot allocator
separately for arm64. The code will end up to be very similar to the one
on arm32. So move out the code in a new helper populate_boot_allocator().
For now the code is still protected by CONFIG_ARM_32 to avoid any build
failure on arm64.
Take the opportunity to replace mfn_add(xen_mfn_start, xenheap_pages) with
xenheap_mfn_end as they are equivalent.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
During early boot, it is not possible to use xen_{,un}map_table()
if the page tables are not residing the Xen binary.
This is a blocker to switch some of the helpers to use xen_pt_update()
as we may need to allocate extra page tables and access them before
the domheap has been initialized (see setup_xenheap_mappings()).
xen_{,un}map_table() are now updated to use the PMAP helpers for early
boot map/unmap. Note that the special case for page-tables residing
in Xen binary has been dropped because it is "complex" and was
only added as a workaround in 8d4f1b8878 ("xen/arm: mm: Allow
generic xen page-tables helpers to be called early").
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
The numbers of includes in mm.c has been growing quite a lot. However
some of them (e.g. xen/device_tree.h, xen/softirq.h) doesn't look
to be directly used by the file or other will be included by
larger headers (e.g asm/flushtlb.h will be included by xen/mm.h).
So trim down the number of includes. Take the opportunity to order
them with the xen headers first, then asm headers and last public
headers.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
To use properly the fixmap definitions, their user would need
also new to include <xen/acpi.h>. This is not very great when
the user itself is not meant to directly use ACPI definitions.
Including <xen/acpi.h> in <asm/config.h> is not option because
the latter header is included by everyone. So move out the fixmap
entries definition in a new header.
Take the opportunity to also move {set, clear}_fixmap() prototypes
in the new header.
Note that most of the definitions in <xen/acpi.h> now need to be
surrounded with #ifndef __ASSEMBLY__ because <asm/fixmap.h> will
be used in assembly (see EARLY_UART_VIRTUAL_ADDRESS).
The split will become more helpful in a follow-up patch where new
fixmap entries will be defined.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
xen_{un,}map_table() already uses the helper to map/unmap pages
on-demand (note this is currently a NOP on arm64). So switching to
domheap don't have any disavantage.
But this as the benefit:
- to keep the page tables unmapped if an arch decided to do so
- reduce xenheap use on arm32 which can be pretty small
Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Now that map_pages_to_xen() has been extended to support 2MB mappings,
we can replace the create_mappings() call by map_pages_to_xen() call.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Currently, the function xen_pt_update() will flush the TLBs even when
the mappings are inserted. This is a bit wasteful because we don't
allow mapping replacement. Even if we were, the flush would need to
happen earlier because mapping replacement should use Break-Before-Make
when updating the entry.
A single call to xen_pt_update() can perform a single action. IOW, it
is not possible to, for instance, mix inserting and removing mappings.
Therefore, we can use `flags` to determine what action is performed.
This change will be particularly help to limit the impact of switching
boot time mapping to use xen_pt_update().
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.com>
In follow-up patches, we will use xen_pt_update() (or its callers)
to handle large mappings (e.g. frametable, xenheap). They are also
not going to be modified once created.
The page-table entries have an hint to indicate that whether an
entry is contiguous to another 16 entries (assuming 4KB). When the
processor support the hint, one TLB entry will be created per
contiguous region.
For now this is tied to _PAGE_BLOCK. We can untie it in the future
if there are use-cases where we may want to use _PAGE_BLOCK without
setting the contiguous (couldn't think of any yet).
Note that to avoid extra complexity, mappings with the contiguous
bit set cannot be removed. Given the expected use, this restriction
ought to be fine.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Hongda Deng <Hongda.Deng@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
xsm_deassign_dtdevice() will indicate whether the caller is allowed
to issue the operation. So the return value has to be checked.
Spotted by clang static analyzer.
Fixes: fe36cccc48 ("xen/passthrough: Extend XEN_DOMCTL_*assign_device to support DT device")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
It is not possible to have a negative number of banks. So switch to
unsigned int.
The type change is also propagated to any users of nr_banks that were
using "int" (there are not that many).
Note that fdt_num_mem_rsv() can actually returns a negative value in
case of an error. So the return should be checked before assigning the
result to an unsigned variable.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Commit 88a037e2cf "page_alloc: assert IRQs are enabled in heap
alloc/free" extended the checks in the buddy allocator to catch any
use of the helpers from context with interrupts disabled.
Unfortunately, the rule is not followed in the LPI code when allocating
the pending table:
(XEN) Xen call trace:
(XEN) [<000000000022a678>] alloc_xenheap_pages+0x178/0x194 (PC)
(XEN) [<000000000022a670>] alloc_xenheap_pages+0x170/0x194 (LR)
(XEN) [<0000000000237770>] _xmalloc+0x144/0x294
(XEN) [<00000000002378d4>] _xzalloc+0x14/0x30
(XEN) [<000000000027b4e4>] gicv3_lpi_init_rdist+0x54/0x324
(XEN) [<0000000000279898>] arch/arm/gic-v3.c#gicv3_cpu_init+0x128/0x46
(XEN) [<0000000000279bfc>] arch/arm/gic-v3.c#gicv3_secondary_cpu_init+0x20/0x50
(XEN) [<0000000000277054>] gic_init_secondary_cpu+0x18/0x30
(XEN) [<0000000000284518>] start_secondary+0x1a8/0x234
(XEN) [<0000010722aa4200>] 0000010722aa4200
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 2:
(XEN) Assertion '!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 1)' failed at common/page_alloc.c:2212
(XEN) ****************************************
For now the patch extending the checks has been reverted, but it would
be good to re-introduce it (allocation with interrupt is not desirable).
The logic is reworked to allocate the pending table when preparing the
CPU.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Commit a5968a553f "SUPPORT.MD: Correct the amount of physical memory
supported for Arm" added a support statement split over two lines.
Unfortunately, docs/support-matrix-generate throw an error for it:
Generating support matrix (origin/stable-NN )
+ docs/support-matrix-generate HEAD https://xenbits.xen.org/docs/unstable/SUPPORT.html origin/stable-NN https://xenbits.xen.org/docs/NN-testing/SUPPORT.html
Status, x86: Supported up to 8 TiB. Hosts with more memory are
supported, but not security supported.
Status, Arm32: Supported up to 12 GiB
Status, Arm64: Supported up to 2 TiB
^ cannot parse status codeblock line:
supported, but not security supported.
? at docs/parse-support-md line 172, <F> chunk 1.
It would be good to allow split support statement (to keep lines below
80 characters) but my knowledge of the script is very limited.
Therefore, workaround the error by describing the support statement
in one long line.
Fixes: a5968a553f "SUPPORT.MD: Correct the amount of physical memory supported for Arm"
Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
Xen provides hypercalls to shutdown (SCHEDOP_shutdown{,_code}) and
resume a domain (XEN_DOMCTL_resumedomain). They can be used for checkpoint
where the expectation is the domain should continue as nothing happened
afterwards.
hvmemul_do_io() and handle_pio() will act differently if the return
code of hvm_send_ioreq() (resp. hvmemul_do_pio_buffer()) is X86EMUL_RETRY.
In this case, the I/O state will be reset to STATE_IOREQ_NONE (i.e
no I/O is pending) and/or the PC will not be advanced.
If the shutdown request happens right after the I/O was sent to the
IOREQ, then emulation code will end up to re-execute the instruction
and therefore forward again the same I/O (at least when reading IO port).
This would be problem if the access has a side-effect. A dumb example,
is a device implementing a counter which is incremented by one for every
access. When running shutdown/resume in a loop, the value read by the
OS may not be the old value + 1.
Add an extra boolean in the structure hvm_vcpu_io to indicate whether
the I/O was suspended. This is then used in place of checking the domain
is shutting down in hvmemul_do_io() and handle_pio() as they should
act on suspend (i.e. vcpu_start_shutdown_deferral() returns false) rather
than shutdown.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Paul Durrant <paul@xen.org>
As part of XSA-385, SUPPORT.MD gained a statement regarding the amount
of physical memory supported.
However, booting Xen on a Arm platform with that amount of memory would
result to a breakage because the frametable area is too small.
The wiki [1] (as of April 2022) claims we were able to support up to
5 TiB on Arm64 and 16 GiB. However, this is not the case because
the struct page_info has always been bigger than expected (56 bytes
for 64-bit and 32-bytes for 32-bit).
I don't have any HW with such amount of memory. So rather than
modifying the code, take the opportunity to use the limit that should
work on Arm (2 TiB for 64-bit and 12 GiB for 32-bit).
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> #arm part
Commit 88a037e2cf "page_alloc: assert IRQs are enabled in heap
alloc/free" extended the checks in the buddy allocator to catch
any use of the helpers from context with interrupts disabled.
Unfortunately, the rule is not followed in the alternative code and
this will result to crash at boot with debug enabled:
(XEN) Xen call trace:
(XEN) [<0022a510>] alloc_xenheap_pages+0x120/0x150 (PC)
(XEN) [<00000000>] 00000000 (LR)
(XEN) [<002736ac>] arch/arm/mm.c#xen_pt_update+0x144/0x6e4
(XEN) [<002740d4>] map_pages_to_xen+0x10/0x20
(XEN) [<00236864>] __vmap+0x400/0x4a4
(XEN) [<0026aee8>] arch/arm/alternative.c#__apply_alternatives_multi_stop+0x144/0x1ec
(XEN) [<0022fe40>] stop_machine_run+0x23c/0x300
(XEN) [<002c40c4>] apply_alternatives_all+0x34/0x5c
(XEN) [<002ce3e8>] start_xen+0xcb8/0x1024
(XEN) [<00200068>] arch/arm/arm32/head.o#primary_switched+0xc/0x1c
The interrupts will be disabled by the state machine in stop_machine_run(),
hence why the ASSERT is hit.
For now the patch extending the checks has been reverted, but it would
be good to re-introduce it (allocation with interrupts disabled is not
desirable).
So move the re-mapping of Xen to the caller of stop_machine_run().
Signed-off-by: Julien Grall <jgrall@amazon.com>
Cc: David Vrabel <dvrabel@amazon.co.uk>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
The boot code expects the regions XEN_VIRT_START, FIXMAP_ADDR(0),
BOOT_FDT_VIRT_START to use the same 0th (arm64 only) and 1st slot.
Add some BUILD_BUG_ON() to confirm that. This is helpful if one wants
to re-order the memory layout.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Since commit 54c4ae18d1 ("xen/arm32: head: Rework and document
launch()"), the boot code is setting r12 but not read it.
So remove the two instructions setting r12 and update the documentation
to show r12 has no specific purpose.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Commit 13c03002c5 ("xen/arm64: head: Mark the end of subroutines
with ENDPROC") intended to mark all the subroutines with ENDPROC.
Unfortunately, I missed fail(), switch_ttbr() and init_uart(). Add
ENDPROC for the benefits of static analysis tools and the reader.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Commit 2ac705a59e ("xen/arm32: head: Mark the end of subroutines
with ENDPROC") intended to mark all the subroutines with ENDPROC.
Unfortunately, I missed fail(), switch_ttbr(), init_uart() and
__lookup_processor_type(). Add ENDPROC for the benefits of
static analysis tools and the reader.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
The array level_orders and level_masks can be replaced with the
recently introduced macros LEVEL_ORDER and LEVEL_MASK.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Currently, Xen PT helpers are only working with 4KB page granularity
and open-code the generic helpers. To allow more flexibility, we can
re-use the generic helpers and pass Xen's page granularity
(PAGE_SHIFT).
As Xen PT helpers are used in both C and assembly, we need to move
the generic helpers definition outside of the !__ASSEMBLY__ section.
Take the opportunity to prefix LPAE_SHIFT, LPAE_ENTRIES and
LPAE_ENTRY_MASK with XEN_PT_.
Note the aliases for each level are still kept for the time being so we
can avoid a massive patch to change all the callers.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Commit 05031fa873 "xen/arm: guest_walk: Only generate necessary
offsets/masks" introduced LPAE_ENTRIES_MASK_GS. In a follow-up patch,
we will use it to define LPAE_ENTRY_MASK.
This will lead to inconsistent naming. As LPAE_ENTRY_MASK is used in
many places, it is better to rename LPAE_ENTRIES_MASK_GS and avoid
some churn.
So rename LPAE_ENTRIES_MASK_GS to LPAE_ENTRY_MASK_GS.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
The local variable pg_offlined in free_heap_pages() can only take two
values. So switch it to a bool.
Fixes: 289610483f ("mm: fix broken tainted value in mark_page_free")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
free_heap_pages() has an ASSERT() checking that node is >= 0. However
node is defined as an unsigned int. So it cannot be negative.
Therefore remove the check as it will always be true.
Coverity-ID: 1055631
Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com><mailto:andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
The name of arch_mfn_in_directmap() suggests that it will check against
that the passed MFN should be in the directmap.
However, the current callers are passing the next MFN and the
implementation will return true for up to one MFN past the directmap.
It would be more meaningful to test the exact MFN rather than the
next one.
That said, the current expectation is the memory will be direct-mapped
from 0 up to a given MFN. This may not be a valid assumption on all
the architectures.
For instance, on Arm32 only the xenheap that will be direct-mapped.
This may not be allocated a the beginning of the RAM.
So take the opportunity to rework the parameters and pass the
number of pages we want to check. This also requires to rename
the helper to better match the implementation.
Note that the implementation of the helper on arm32 is left as-is
for now.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
pt_pirq_iterate() will iterate in batch over all the PIRQs. The outer
loop will bail out if 'rc' is non-zero but the inner loop will continue.
This means 'rc' will get clobbered and we may miss any errors (such as
-ERESTART in the case of the callback pci_clean_dpci_irq()).
This is CVE-2022-23035 / XSA-395.
Fixes: c24536b636 ("replace d->nr_pirqs sized arrays with radix tree")
Fixes: f6dd295381 ("dpci: replace tasklet with softirq")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
The grant unmapping hypercall (GNTTABOP_unmap_grant_ref) is not a
simple revert of the changes done by the grant mapping hypercall
(GNTTABOP_map_grant_ref).
Instead, it is possible to partially (or even not) clear some flags.
This will leave the grant is mapped until a future call where all
the flags would be cleared.
XSA-380 introduced a refcounting that is meant to only be dropped
when the grant is fully unmapped. Unfortunately, unmap_common() will
decrement the refcount for every successful call.
A consequence is a domain would be able to underflow the refcount
and trigger a BUG().
Looking at the code, it is not clear to me why a domain would
want to partially clear some flags in the grant-table. But as
this is part of the ABI, it is better to not change the behavior
for now.
Fix it by checking if the maptrack handle has been released before
decrementing the refcounting.
This is CVE-2022-23034 / XSA-394.
Fixes: 9781b51efd ("gnttab: replace mapkind()")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Commit 2148a125b7 ("xen/arm: Track page accessed between batch of
Set/Way operations") allowed an entry to be invalid from the CPU PoV
(lpae_is_valid()) but valid for Xen (p2m_is_valid()). This is useful
to track which page is accessed and only perform an action on them
(e.g. clean & invalidate the cache after a set/way instruction).
Unfortunately, __p2m_set_entry() is only zeroing the P2M entry when
lpae_is_valid() returns true. This means the entry will not be zeroed
if the entry was valid from Xen PoV but invalid from the CPU PoV for
tracking purpose.
As a consequence, this will allow a domain to continue to access the
page after it was removed.
Resolve the issue by always zeroing the entry if it the LPAE bit is
set or the entry is about to be removed.
This is CVE-2022-23033 / XSA-393.
Reported-by: Dmytro Firsov <Dmytro_Firsov@epam.com>
Fixes: 2148a125b7 ("xen/arm: Track page accessed between batch of Set/Way operations")
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Signed-off-by: Julien Grall <jgrall@amazon.com>
domain_tot_pages() and d->max_pages are 32-bit values. While the order
should always be quite small, it would still be possible to overflow
if domain_tot_pages() is near to (2^32 - 1).
As this code may be called by a guest via XENMEM_increase_reservation
and XENMEM_populate_physmap, we want to make sure the guest is not going
to be able to allocate more than it is allowed.
Rework the allocation check to avoid any possible overflow. While the
check domain_tot_pages() < d->max_pages should technically not be
necessary, it is probably best to have it to catch any possible
inconsistencies in the future.
This is CVE-2021-28706 / part of XSA-385.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Commit 939775cfd3 "handle dying domains in live update" was meant to
handle gracefully dying domain. However, the @releaseDomain watch
will end up to be sent as soon as we finished to restore Xenstored
state.
This may be before Xen reports the domain to be dying (such as if
the guest decided to revoke access to the xenstore page). Consequently
daemon like xenconsoled will not clean-up the domain and it will be
left as a zombie.
To avoid the problem, mark the connection as ignored. This also
requires to tweak conn_can_write() and conn_can_read() to prevent
dereferencing a NULL pointer (the interface will not mapped).
The check conn->is_ignored was originally added after the callbacks
because the helpers for a socket connection may close the fd. However,
ignore_connection() will close a socket connection directly. So it is
fine to do the re-order.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
Reviewed-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Currently, both dom0less domUs and dom0 can allocate an "unlimited"
amount of memory because d->max_pages is set to ~0U.
In particular, the former are meant to be unprivileged. Therefore the
memory they could allocate should be bounded. As the domain are not yet
officially aware of Xen (we don't expose advertise it in the DT, yet
the hypercalls are accessible), they should not need to allocate more
than the initial amount. So cap set d->max_pages directly the amount of
memory we are meant to allocate.
Take the opportunity to also restrict the memory for dom0 as the
domain is direct mapped (e.g. MFN == GFN) and therefore cannot
allocate outside of the pre-allocated region.
This is CVE-2021-28700 / XSA-383.
Reported-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
At the moment, do_control_lu() will set errno to 0 before calling
lu_arch() and then check errno. The expectation is nothing in lu_arch()
will change the value unless there is an error.
However, per errno(3), a function that succeeds is allowed to change
errno. In fact, syslog() will overwrite errno if the logs are rotated
at the time it is called.
To prevent any further issue, errno is now always set before
returning NULL.
Additionally, errno is only checked when returning NULL so the client
can see the error message if there is any.
Reported-by: Michael Kurth <mku@amazon.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
lu_start() will only set errno when it returns NULL. For all the
other cases, the value is unknown.
This means that when lu_start() returns an error message, it may not
be propagated to the client.
The check that errno is a non-zero value is now dropped and instead
the value is returned when no error message is provided. This
relies on errno to always be set when ret == NULL.
Fixes: af216a99fb ("tools/xenstore: add the basic framework for doing the live update")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
The maximum path length supported by Xenstored protocol is
XENSTORE_ABS_PATH_MAX (i.e 3072). This doesn't take into account the
NUL at the end of the path.
However, the code to dump the nodes will allocate a buffer
of XENSTORE_ABS_PATH. As a result it may not be possible to live-update
if there is a node name of XENSTORE_ABS_PATH.
Fix it by allocating a buffer of XENSTORE_ABS_PATH_MAX + 1 characters.
Take the opportunity to pass the max length of the buffer as a
parameter of dump_state_node_tree(). This will be clearer that the
check in the function is linked to the allocation in dump_state_nodes().
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Commit 0dbb4be739 add the inclusion of xenctrl.h from private.h and
wreck the build in an interesting way:
In file included from xen/stubdom/include/xen/domctl.h:39:0,
from xen/tools/include/xenctrl.h:36,
from private.h:4,
from minios.c:29:
xen/include/public/memory.h:407:5: error: expected specifier-qualifier-list before ‘XEN_GUEST_HANDLE_64’
XEN_GUEST_HANDLE_64(const_uint8) buffer;
^~~~~~~~~~~~~~~~~~~
This is happening because xenctrl.h defines __XEN_TOOLS__ and therefore
the public headers will start to expose the non-stable ABI. However,
xen.h has already been included by a mini-OS header before hand. So
there is a mismatch in the way the headers are included.
For now solve it in a very simple (and gross) way by including
xenctrl.h before the mini-os headers.
Fixes: 0dbb4be739 ("tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Commit 918b8842a8 ("arm64: Change type of hsr, cpsr, spsr_el1 to
uint64_t") updated the size of the structure vcpu_guest_core_regs and
indirectly vcpu_guest_context.
On Arm, the two structures are only accessible to the tools and the
hypervisor (and therefore stable). However, they are still checked
by the scripts in tools/include/xen-foreign are not able to understand
that.
Ideally we should rework the scripts so we don't have to update
the size for non-stable structure. But I don't have limited time
to spend on the issue. So chose the simple solution and update
the size accordingly.
Note that we need to keep vcpu_guest_core_regs around because
the structure is used by vcpu_guest_context and therefore the
scripts expects the generated header to contain it.
Fixes: 918b8842a8 ("arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t")
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Tested-by: Michal Orzel <michal.orzel@arm.com>
When Live-Updating with some load, Xenstored may hit the assert
req->in == lu_status->in in do_lu_start().
This is happening because the request is stashed when Live-Update
begins. This happens in a different request (see call lu_begin()
when select the new binary) from the one performing Live-Update.
To avoid the problem, stash the request in lu_start().
Fixes: 65f19ed62a ("tools/xenstore: Don't assume conn->in points to the LU request")
Reported-by: Michael Kurth <mku@amazon.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: luca.fancellu@arm.com
Reviewed-by: Juergen Gross <jgross@suse.com>
Commit c0fe360f42 ("tools/xenstored: Extend restore code to handle
multiple input buffer") extend the read_buffered_state() to support
multiple input buffers. Unfortunately, the commit didn't go far
enough and still used sc->data (start of the buffers) for retrieving
the header. This would lead to read the wrong headers for second and
follow-up commands.
Use data in place for sc->data for the source of the memcpy()s.
Fixes: c0fe360f42 ("tools/xenstored: Extend restore code to handle multiple input buffer")
Reported-by: Raphael Ning <raphning@amazon.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Commit 3adfb50315 ("tools/xenstored: Introduce a wrapper for
conn->funcs->can_{read, write}") consolidated the check
!conn->is_ignored in two new wrappers.
This means the check in socket_can_process() is now redundant. In
fact it should have been removed in orignal commit (as it was done
for the domain helpers).
Reported-by: Raphael Ning <raphning@amazon.com
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
As Live-Update is asynchronous, it is possible to receive a request to
cancel it (either on the same connection or from a different one).
Currently, this will crash xenstored because do_lu_start() assumes
lu_status will be valid. This is not the case when Live-Update has been
cancelled. This will result to dereference a NULL pointer and
crash Xenstored.
Rework do_lu_start() to check if lu_status is NULL and return an
error in this case.
Fixes: af216a99fb ("tools/xenstore: add the basic framework for doing the live update")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
At the moment, Live-Update will, by default, not proceed if there are
in-flight transactions. It is possible force it by passing -F but this
will break any connection with in-flight transactions.
There are PV drivers out that may never terminate some transaction. On
host running such guest, we would need to use -F. Unfortunately, this
also risks to break well-behaving guests (and even dom0) because
Live-Update will happen as soon as the timeout is hit.
Ideally, we would want to preserve transactions but this requires
some work and a lot of testing to be able to use it in production.
As a stop gap, we want to limit the damage of -F. This patch will delay
any transactions that are started after Live-Update has been requested.
If the request cannot be delayed, the connection will be stalled to
avoid loosing requests.
If the connection has already a pending transaction before Live-Update,
then new transaction will not be delayed. This is to avoid the connection
to stall.
With this stop gap in place, domains with long running transactions will
still break when using -F, but other domains which starts a transaction
in the middle of Live-Update will continue to work.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Currently, only Live-Update request can be delayed. In a follow-up,
we will want to delay more requests (e.g. transaction start).
Therefore we want to preserve delayed requests across Live-Update.
Delayed requests are just complete "in" buffer. So the code is
refactored to allow sharing the code to dump "in" buffer.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Currently, the restore code is considering the stream will contain at
most one in-flight request per connection. In a follow-up changes, we
will want to transfer multiple in-flight requests.
The function read_state_buffered() is now extended to restore multiple
in-flight request. Complete requests will be queued as delayed
requests, if there a partial request (only the last one can) then it
will used as the current in-flight request.
Note that we want to bypass the quota check for delayed requests as
the new Xenstore may have a lower limit.
Lastly, there is no need to change the specification as there was
no restriction on the number of in-flight requests preserved.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
delay_request() is currently assuming that the request delayed is
always conn->in. This is currently correct, but it is a call for
a latent bug as the function allows the caller to specify any request.
To prevent any future surprise, check if the request delayed is the
current one.
Fixes: c5ca1404b4 ("tools/xenstore: add support for delaying execution of a xenstore request")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Currently, the callbacks can_read and can_write are called directly. This
doesn't allow us to add generic check and therefore requires duplication.
At the moment, one check that could benefit to be common is whether the
connection should ignored. The position is slightly different between
domain and socket because for the latter we want to check the state of
the file descriptor first.
In follow-up patches, there will be more potential generic checks.
This patch provides wrappers to read/write a connection and move
the check ->is_ignored after the callback for everyone.
This also requires to replace the direct call to domain_can_read()
and domain_can_write() with the new wrapper. At the same time,
both functions can now be static. Note that the implementations need
to be moved earlier in the file xenstored_domain.c to avoid
declaring the prototype.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
xenstored_core.h will consider live-udpate is not supported if
O_CLOEXEC doesn't exist. However, the header doesn't include the one
defining O_CLOEXEC (i.e. fcntl.h). This means that depending on
the header included, some source file will think Live-Update is not
supported.
I am not aware of any issue with the existing. Therefore this is just
a latent bug so far.
Prevent any potential issue by including fcntl.h in xenstored_core.h
Fixes: cd831ee438 ("tools/xenstore: handle CLOEXEC flag for local files and pipes")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Currently, only liveupdate request can be delayed. The request can only
be performed by a privileged connection (e.g. dom0). So it is fine to
have no limits.
In a follow-up patch we will want to delay request for unprivileged
connection as well. So it is best to apply a limit.
For now and for simplicity, only a single request can be delayed
for a given unprivileged connection.
Take the opportunity to tweak the prototype and provide a way to
bypass the quota check. This would be useful when the function
is called from the restore code.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
call_delayed() is currently assuming that conn->in is NULL when
handling delayed request. However, the connection is not paused.
Therefore new request can be processed and conn->in may be non-NULL
if we have only received a partial request.
Furthermore, as we overwrite conn->in, the current partial request
will not be transferred. This will result to corrupt the connection.
Rather than updating conn->in, stash the LU request in lu_status and
let each callback for delayed request to update conn->in when
necessary.
To keep a sane interface, the code to write the "OK" response the
LU request is moved in xenstored_core.c.
Fixes: c5ca1404b4 ("tools/xenstore: add support for delaying execution of a xenstore request")
Fixes: ed6eebf17d ("tools/xenstore: dump the xenstore state for live update")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
At the moment, dump_state_buffered_data() is taking two connections
in parameters (one is the connection to dump, the other is the
connection used to request LU). The naming doesn't help to
distinguish (c vs conn) them and this already lead to several mistake
while modifying the function.
To remove the confusion, introduce an help lu_get_connection() that
will return the connection used to request LU and use it
in place of the existing parameter.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Since XSA-228 (commit 02cbeeb620 "gnttab: split maptrack lock
to make it fulfill its purpose again"), v->maptrack_head,
v->maptrack_tail and the content of the freelist are accessed with
the lock v->maptrack_freelist_lock held.
Therefore it is not necessary to update the fields using cmpxchg()
and also read them atomically.
Note that there are two cases where v->maptrack_tail is accessed without
the lock. They both happen in get_maptrack_handle() when initializing
the free list of the current vCPU. Therefore there is no possible race.
The code is now reworked to remove any use of cmpxch() and read_atomic()
when accessing the fields v->maptrack_{head, tail} as wel as the
freelist.
Take the opportunity to add a comment on top of the lock definition
and explain what it protects.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
The function to initialize the pages (see init_heap_pages()) will request
scrub when the admin request idle bootscrub (default) and state ==
SYS_STATE_active. When bootscrub=on, Xen will scrub any free pages in
heap_init_late().
Currently, the boot modules (e.g. kernels, initramfs) will be discarded/
freed after heap_init_late() is called and system_state switched to
SYS_STATE_active. This means the pages associated with the boot modules
will not get scrubbed before getting re-purposed.
If the memory is assigned to an untrusted domU, it may be able to
retrieve secrets from the modules.
This is part of XSA-372 / CVE-2021-28693.
Fixes: 1774e9b1df ("xen/arm: introduce create_domUs")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
In a follow-up patch we will need to unallocate the boot modules
before heap_init_late() is called.
The modules will contain the domUs kernel and initramfs. Therefore Xen
will need to create extra domUs (used by dom0less) before heap_init_late().
This has two consequences on dom0less:
1) Domains will not be unpaused as soon as they are created but
once all have been created. However, Xen doesn't guarantee an order
to unpause, so this is not something one could rely on.
2) The memory allocated for a domU will not be scrubbed anymore when an
admin select bootscrub=on. This is not something we advertised, but if
this is a concern we can introduce either force scrub for all domUs or
a per-domain flag in the DT. The behavior for bootscrub=off and
bootscrub=idle (default) has not changed.
This is part of XSA-372 / CVE-2021-28693.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Tested-by: Stefano Stabellini <sstabellini@kernel.org>
Since commit 1aac966e24 "xen: support RAM at addresses 0 and 4096",
bits_to_zone() will never return 0 and it is expected that we have
minimum 2 zones.
Therefore the check in alloc_domheap_pages() is unnecessary and can
be removed. However, for sanity, it is replaced with an ASSERT().
Also take the opportunity to switch from min_t() to min() as
bits_to_zone() cannot return a negative value. The macro is tweaked
to make it clearer.
This bug was discovered and resolved using Coverity Static Analysis
Security Testing (SAST) by Synopsys, Inc.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
The parameter of check_domains() is not used within the function. In fact,
this was a left over of the original implementation as the version merged
doesn't need to know whether we are restoring.
So remove it.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Literal strings are not meant to be modified. So we should use const
char * rather than char * when we want to store a pointer to them.
Take the opportunity to remove the cast (char *) in console_init(). It
is unnecessary and will remove the const.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: Wei Liu <wl@xen.org>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Literal strings are not meant to be modified. So we should use const
char * rather than char * when we want to store a pointer to them.
The array should also not be modified at all and is only used by
xenlog_update_val(). So take the opportunity to add an extra const and
move the definition in the function.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Gitlab CI spotted an issue when building the tools Arm:
xg_dom_arm.c: In function 'meminit':
xg_dom_arm.c:401:50: error: passing argument 3 of 'set_mode' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
401 | rc = set_mode(dom->xch, dom->guest_domid, dom->guest_type);
| ~~~^~~~~~~~~~~~
This is because the const was not propagated in the Arm code. Fix it
by constifying the 3rd parameter of set_mode().
Fixes: 8fc4916daf ("tools/libs: guest: Use const whenever we point to literal strings")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
The field text in stat_map_t will point to string literals. So mark it
as const to allow the compiler to catch any modified of the string.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Wei Liu <wl@xen.org>
Neither string parameter in set_prompt() and set_delay() are meant to
be modified. In particular, new_prompt can point to a literal string.
So mark the two parameters as const and propagate it.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Wei Liu <wl@xen.org>
literal strings are not meant to be modified. So we should use const
char * rather than char * when we we to store a pointer to them.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Wei Liu <wl@xen.org>
literal strings are not meant to be modified. So we should use const
char * rather than char * when we want to store a pointer to them.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Wei Liu <wl@xen.org>
literal strings are not meant to be modified. So we should use const
*char rather than char * when we want to store a pointer to them.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Wei Liu <wl@xen.org>
Currently, we are ignoring any error from perform_gunzip() and replacing
the compressed kernel with the "uncompressed" kernel.
If there is a gzip failure, then it means that the output buffer may
contain garbagge. So it can result to various sort of behavior that may
be difficult to root cause.
In case of failure, free the output buffer and propagate the error.
We also need to adjust the return check for kernel_compress() as
perform_gunzip() may return a positive value.
Take the opportunity to adjust the code style for the check.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Commit 78e67c99eb "arm/gic: Get rid of READ/WRITE_SYSREG32"
mistakenly converted all the cases in gicv3_read_apr() to fall-through.
Rather than re-instating a return per case, add the missing break and
keep a single return at the end of the fucntion.
Fixes: 78e67c99eb ("arm/gic: Get rid of READ/WRITE_SYSREG32")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Julien Grall