Re: [PATCH for-4.17] xen/arm: Support properly __ro_after_init on Arm

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 16 Aug 2022, at 19:59, Julien Grall <julien@xxxxxxx> wrote:
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> __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, but moving the sections .data.ro_after_init

Typo here s/but/by/ and remove ,

> out of .data and then mark the region read-only once we finish
> to boot.

I would s/mark/map/

> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

(Commit message can be fixed on commit)

Cheers
Bertrand

> 
> ---
> 
> This patch is targeting Xen 4.17. There are quite a few arm
> specific variables that could be switch to use __ro_after_init.
> 
> This is not addressed by the commit. We could consider to switch
> some of them for Xen 4.17. So the benefits for now is any common
> variables using __ro_after_init.
> ---
> xen/arch/arm/include/asm/setup.h |  2 ++
> xen/arch/arm/setup.c             | 14 ++++++++++++++
> xen/arch/arm/xen.lds.S           |  7 +++++++
> 3 files changed, 23 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/setup.h 
> b/xen/arch/arm/include/asm/setup.h
> index 2bb01ecfa88f..5815ccf8c5cc 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -137,6 +137,8 @@ u32 device_tree_get_u32(const void *fdt, int node,
> int map_range_to_domain(const struct dt_device_node *dev,
>                         u64 addr, u64 len, void *data);
> 
> +extern const char __ro_after_init_start[], __ro_after_init_end[];
> +
> #endif
> /*
>  * Local variables:
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 500307edc08d..5bde321b9d07 100644
> --- 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);
> +
>     startup_cpu_idle_loop();
> }
> 
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 1e986e211f68..92c298405259 100644
> --- 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
> +
>   .data.read_mostly : {
>        /* Exception table */
>        __start___ex_table = .;
> -- 
> 2.37.1
>