Hi Jan,
On 17/08/2022 09:37, Jan Beulich wrote:
On 16.08.2022 20:59, Julien Grall wrote:
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -75,10 +75,24 @@ domid_t __read_mostly max_init_domid;
static __used void init_done(void)
{
+ int rc;
+
/* Must be done past setting system_state. */
unregister_init_virtual_region();
free_init_memory();
+
+ /*
+ * We have finished to boot. Mark the section .data.ro_after_init
+ * read-only.
+ */
+ rc = modify_xen_mappings((unsigned long)&__ro_after_init_start,
+ (unsigned long)&__ro_after_init_end,
+ PAGE_HYPERVISOR_RO);
+ if ( rc )
+ panic("Unable to mark the .data.ro_after_init section read-only (rc =
%d)\n",
+ rc);
Just wondering - is this really worth panic()ing?
The function should never fails and it sounds wrong to me to continue in
the unlikely case it will fail.
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -83,6 +83,13 @@ SECTIONS
_erodata = .; /* End of read-only data */
. = ALIGN(PAGE_SIZE);
+ .data.ro_after_init : {
+ __ro_after_init_start = .;
+ *(.data.ro_after_init)
+ . = ALIGN(PAGE_SIZE);
+ __ro_after_init_end = .;
+ } : text
Again just wondering: Wouldn't it be an option to avoid the initial
page size alignment (and the resulting padding) here, simply making
.data.ro_after_init part of .rodata and do the earlier write-protection
only up to (but excluding) the page containing __ro_after_init_start?
So both this question and the previous one will impair the security of
Xen on Arm (even though the later is only at boot time).
This is not something I will support just because we are going to save <
PAGE_SIZE. If we are concern of the size wasted, then there are other
way to mitigate it (i.e. moving more variables to __ro_after_init).
Cheers,
--
Julien Grall
