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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

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