| Message ID | 20220816185954.31945-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.17] xen/arm: Support properly __ro_after_init on Arm | expand |
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
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 >
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
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,
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 >
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
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,
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,
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 = .;