This reinstates the capability to patch .rodata in load/unload hooks, which
was lost when we stopped using CR0.WP=0 to patch.

This turns out to be rather less of a large TODO than I thought at the time.

Fixes: 8676092a0f16 ("x86/livepatch: Fix livepatch application when CET is active")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/livepatch.c    |  4 ++--
 xen/common/virtual_region.c | 12 ++++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index ee539f001b73..4f76127e1f77 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -62,7 +62,7 @@ int arch_livepatch_safety_check(void)
 int noinline arch_livepatch_quiesce(void)
 {
     /*
-     * Relax perms on .text to be RWX, so we can modify them.
+     * Relax perms on .text/.rodata, so we can modify them.
      *
      * This relaxes perms globally, but all other CPUs are waiting on us.
      */
@@ -75,7 +75,7 @@ int noinline arch_livepatch_quiesce(void)
 void noinline arch_livepatch_revive(void)
 {
     /*
-     * Reinstate perms on .text to be RX.  This also cleans out the dirty
+     * Reinstate perms on .text/.rodata.  This also cleans out the dirty
      * bits, which matters when CET Shstk is active.
      *
      * The other CPUs waiting for us could in principle have re-walked while
diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
index d2efe9e11492..f45812483b8e 100644
--- a/xen/common/virtual_region.c
+++ b/xen/common/virtual_region.c
@@ -91,9 +91,15 @@ void relax_virtual_region_perms(void)
 
     rcu_read_lock(&rcu_virtual_region_lock);
     list_for_each_entry_rcu( region, &virtual_region_list, list )
+    {
         modify_xen_mappings_lite((unsigned long)region->text_start,
                                  PAGE_ALIGN((unsigned long)region->text_end),
                                  PAGE_HYPERVISOR_RWX);
+        if ( region->rodata_start )
+            modify_xen_mappings_lite((unsigned long)region->rodata_start,
+                                     ROUNDUP((unsigned long)region->rodata_end, PAGE_SIZE),
+                                     PAGE_HYPERVISOR_RW);
+    }
     rcu_read_unlock(&rcu_virtual_region_lock);
 }
 
@@ -103,9 +109,15 @@ void tighten_virtual_region_perms(void)
 
     rcu_read_lock(&rcu_virtual_region_lock);
     list_for_each_entry_rcu( region, &virtual_region_list, list )
+    {
         modify_xen_mappings_lite((unsigned long)region->text_start,
                                  PAGE_ALIGN((unsigned long)region->text_end),
                                  PAGE_HYPERVISOR_RX);
+        if ( region->rodata_start )
+            modify_xen_mappings_lite((unsigned long)region->rodata_start,
+                                     ROUNDUP((unsigned long)region->rodata_end, PAGE_SIZE),
+                                     PAGE_HYPERVISOR_RO);
+    }
     rcu_read_unlock(&rcu_virtual_region_lock);
 }
 #endif /* CONFIG_X86 */
-- 
2.30.2

On 05/03/2024 12:11 pm, Andrew Cooper wrote:
> diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
> index d2efe9e11492..f45812483b8e 100644
> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -91,9 +91,15 @@ void relax_virtual_region_perms(void)
>  
>      rcu_read_lock(&rcu_virtual_region_lock);
>      list_for_each_entry_rcu( region, &virtual_region_list, list )
> +    {
>          modify_xen_mappings_lite((unsigned long)region->text_start,
>                                   PAGE_ALIGN((unsigned long)region->text_end),
>                                   PAGE_HYPERVISOR_RWX);
> +        if ( region->rodata_start )
> +            modify_xen_mappings_lite((unsigned long)region->rodata_start,
> +                                     ROUNDUP((unsigned long)region->rodata_end, PAGE_SIZE),

I missed the final refresh to turn this to PAGE_ALIGN().  Fixed locally.

~Andrew

On Tue, Mar 5, 2024 at 1:02 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 05/03/2024 12:11 pm, Andrew Cooper wrote:
> > diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
> > index d2efe9e11492..f45812483b8e 100644
> > --- a/xen/common/virtual_region.c
> > +++ b/xen/common/virtual_region.c
> > @@ -91,9 +91,15 @@ void relax_virtual_region_perms(void)
> >
> >      rcu_read_lock(&rcu_virtual_region_lock);
> >      list_for_each_entry_rcu( region, &virtual_region_list, list )
> > +    {
> >          modify_xen_mappings_lite((unsigned long)region->text_start,
> >                                   PAGE_ALIGN((unsigned long)region->text_end),
> >                                   PAGE_HYPERVISOR_RWX);
> > +        if ( region->rodata_start )
> > +            modify_xen_mappings_lite((unsigned long)region->rodata_start,
> > +                                     ROUNDUP((unsigned long)region->rodata_end, PAGE_SIZE),
>
> I missed the final refresh to turn this to PAGE_ALIGN().  Fixed locally.

In that case,

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

On Tue, Mar 05, 2024 at 01:02:37PM +0000, Andrew Cooper wrote:
> On 05/03/2024 12:11 pm, Andrew Cooper wrote:
> > diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
> > index d2efe9e11492..f45812483b8e 100644
> > --- a/xen/common/virtual_region.c
> > +++ b/xen/common/virtual_region.c
> > @@ -91,9 +91,15 @@ void relax_virtual_region_perms(void)
> >  
> >      rcu_read_lock(&rcu_virtual_region_lock);
> >      list_for_each_entry_rcu( region, &virtual_region_list, list )
> > +    {
> >          modify_xen_mappings_lite((unsigned long)region->text_start,
> >                                   PAGE_ALIGN((unsigned long)region->text_end),
> >                                   PAGE_HYPERVISOR_RWX);
> > +        if ( region->rodata_start )
> > +            modify_xen_mappings_lite((unsigned long)region->rodata_start,
> > +                                     ROUNDUP((unsigned long)region->rodata_end, PAGE_SIZE),
> 
> I missed the final refresh to turn this to PAGE_ALIGN().  Fixed locally.

With that:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.