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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Julien Grall
> Sent: Wednesday, August 17, 2022 3:00 AM
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: julien@xxxxxxx; Julien Grall <jgrall@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: [PATCH for-4.17] xen/arm: Support properly __ro_after_init on Arm
> 
> 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 out of .data and
> then mark the region read-only once we finish to boot.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> 

Reviewed-by: Penny Zheng <penny.zheng@xxxxxxx>

> ---
> 
> 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.
> +     */

Nit: Maybe it is finish + doing, could be wrong, feel free to change or not~~
 
> +    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();
>  }
> 
> 2.37.1
>