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

[Julien Grall <julien@xxxxxxx>]

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