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

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

[Julien Grall]

From: Julien Grall <jgrall@amazon.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, 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@amazon.com>

---

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

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

[Henry Wang]

Hi Julien,

> -----Original Message-----
> Subject: [PATCH for-4.17] xen/arm: Support properly __ro_after_init on Arm
> 
> From: Julien Grall <jgrall@amazon.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, 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@amazon.com>

Tested-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

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

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Julien Grall
> Sent: Wednesday, August 17, 2022 3:00 AM
> To: xen-devel@lists.xenproject.org
> Cc: julien@xen.org; Julien Grall <jgrall@amazon.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: [PATCH for-4.17] xen/arm: Support properly __ro_after_init on Arm
> 
> From: Julien Grall <jgrall@amazon.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, 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@amazon.com>
> 

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

> ---
> 
> 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
> 

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

[Jan Beulich]

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?

> --- 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?

Jan

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

[Julien Grall]

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

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

[Bertrand Marquis]

Hi Julien,

> On 16 Aug 2022, at 19:59, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.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, 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@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

(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
> 

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

[Bertrand Marquis]

Hi,

> On 17 Aug 2022, at 10:14, Julien Grall <julien@xen.org> wrote:
> 
> 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.

I agree, we should not ignore and error here.

> 
>>> --- 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).

Agree with Julien here.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

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

[Julien Grall]

On 17/08/2022 07:33, Penny Zheng wrote:
> Hi Julien

Hi Penny,

>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
>> Julien Grall
>> Sent: Wednesday, August 17, 2022 3:00 AM
>> To: xen-devel@lists.xenproject.org
>> Cc: julien@xen.org; Julien Grall <jgrall@amazon.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> Subject: [PATCH for-4.17] xen/arm: Support properly __ro_after_init on Arm
>>
>> From: Julien Grall <jgrall@amazon.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, 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@amazon.com>
>>
> 
> Reviewed-by: Penny Zheng <penny.zheng@arm.com>

Thanks!

> 
>> ---
>>
>> 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~~

I will update.

Cheers,

-- 
Julien Grall

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

[Julien Grall]

On 18/08/2022 15:06, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

> 
>> On 16 Aug 2022, at 19:59, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.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, but moving the sections .data.ro_after_init
> 
> Typo here s/but/by/ and remove ,

I updated it.

> 
>> out of .data and then mark the region read-only once we finish
>> to boot.
> 
> I would s/mark/map/
Ok.

> 
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks!

> 
> (Commit message can be fixed on commit)


I have fixed the commit message, addressed the typo from Penny and 
committed the patch.

Cheers,




-- 
Julien Grall