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