While we've been diligent to ensure that the main text/data/rodata mappings
have suitable restrictions, their aliases via the directmap were left fully
read/write.  Worse, we even had pieces of code making use of this as a
feature.

Restrict the permissions for .text/rodata, as we have no legitimate need for
writeability of these areas via the directmap alias.  Note that the
compile-time allocated pagetables do get written through their directmap
alias, so need to remain writeable.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v2:
 * Update comments and commit message for clarity, and over changes.

Notes:
 * The stubs are still have RX via one alias, RW via another, and these need
   to stay.  We should harden this using PKS (available on SPR and later) to
   block incidental writes.
 * Backing memory for livepatch text/rodata needs similar treatment.
 * For backporting, this patch depends on c/s e7f147bf4ac7 ("x86/crash: Drop
   manual hooking of exception_table[]") and c/s e7db635f4428 ("x86/pv-shim:
   Don't modify the hypercall table").  No compile error will occur from
   getting these dependencies wrong.
---
 xen/arch/x86/setup.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2b44a3ae26dd..b29229933d8c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1667,6 +1667,16 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         destroy_xen_mappings((unsigned long)&__2M_rwdata_end,
                              ROUNDUP((unsigned long)&__2M_rwdata_end, MB(2)));
 
+    /*
+     * Mark all of .text and .rodata as RO in the directmap - we don't want
+     * these sections writeable via any alias.  The compile-time allocated
+     * pagetables are written via their directmap alias, so data/bss needs to
+     * remain writeable.
+     */
+    modify_xen_mappings((unsigned long)__va(__pa(_start)),
+                        (unsigned long)__va(__pa(__2M_rodata_end)),
+                        PAGE_HYPERVISOR_RO);
+
     nr_pages = 0;
     for ( i = 0; i < e820.nr_map; i++ )
         if ( e820.map[i].type == E820_RAM )
-- 
2.30.2

On 24.03.2023 23:08, Andrew Cooper wrote:
> While we've been diligent to ensure that the main text/data/rodata mappings
> have suitable restrictions, their aliases via the directmap were left fully
> read/write.  Worse, we even had pieces of code making use of this as a
> feature.
> 
> Restrict the permissions for .text/rodata, as we have no legitimate need for
> writeability of these areas via the directmap alias.  Note that the
> compile-time allocated pagetables do get written through their directmap
> alias, so need to remain writeable.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> Notes:
>  * The stubs are still have RX via one alias, RW via another, and these need
>    to stay.  We should harden this using PKS (available on SPR and later) to
>    block incidental writes.
>  * Backing memory for livepatch text/rodata needs similar treatment.

Right, but there it's somewhat more involved because upon removal the
attributes also need restoring.

>  * For backporting, this patch depends on c/s e7f147bf4ac7 ("x86/crash: Drop
>    manual hooking of exception_table[]") and c/s e7db635f4428 ("x86/pv-shim:
>    Don't modify the hypercall table").  No compile error will occur from
>    getting these dependencies wrong.

I suppose the latter isn't strictly a prereq, as the modification was done
from an __init function (i.e. before this new code runs).

Iirc we didn't backport prior similar hardening work? So I'm not sure we'd
want/need to do so in this case.

Jan

On 27/03/2023 4:43 pm, Jan Beulich wrote:
> On 24.03.2023 23:08, Andrew Cooper wrote:
>> While we've been diligent to ensure that the main text/data/rodata mappings
>> have suitable restrictions, their aliases via the directmap were left fully
>> read/write.  Worse, we even had pieces of code making use of this as a
>> feature.
>>
>> Restrict the permissions for .text/rodata, as we have no legitimate need for
>> writeability of these areas via the directmap alias.  Note that the
>> compile-time allocated pagetables do get written through their directmap
>> alias, so need to remain writeable.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
>> Notes:
>>  * The stubs are still have RX via one alias, RW via another, and these need
>>    to stay.  We should harden this using PKS (available on SPR and later) to
>>    block incidental writes.
>>  * Backing memory for livepatch text/rodata needs similar treatment.
> Right, but there it's somewhat more involved because upon removal the
> attributes also need restoring.

Yeah, I'm going to defer it to a gitlab ticket for now.  And the PKS
addition too.

>>  * For backporting, this patch depends on c/s e7f147bf4ac7 ("x86/crash: Drop
>>    manual hooking of exception_table[]") and c/s e7db635f4428 ("x86/pv-shim:
>>    Don't modify the hypercall table").  No compile error will occur from
>>    getting these dependencies wrong.
> I suppose the latter isn't strictly a prereq, as the modification was done
> from an __init function (i.e. before this new code runs).

This code here runs long before pv_shim_setup_dom().  This dependency
was found experimentally via testing IIRC.

> Iirc we didn't backport prior similar hardening work? So I'm not sure we'd
> want/need to do so in this case.

That patch wasn't backported.  I was originally planning to, as part of
the CET-IBT work for Intel-retbleed, but in the end didn't. 

We went for the "ENDBR on every function" approach on older trees
because it was better than nothing, and far less invasive than
backporting cf_check everywhere.

~Andrew

On 28.03.2023 12:27, Andrew Cooper wrote:
> On 27/03/2023 4:43 pm, Jan Beulich wrote:
>> On 24.03.2023 23:08, Andrew Cooper wrote:
>>>  * For backporting, this patch depends on c/s e7f147bf4ac7 ("x86/crash: Drop
>>>    manual hooking of exception_table[]") and c/s e7db635f4428 ("x86/pv-shim:
>>>    Don't modify the hypercall table").  No compile error will occur from
>>>    getting these dependencies wrong.
>> I suppose the latter isn't strictly a prereq, as the modification was done
>> from an __init function (i.e. before this new code runs).
> 
> This code here runs long before pv_shim_setup_dom().  This dependency
> was found experimentally via testing IIRC.

Oh, of course. I was mistakenly associating the new code with init_done().
(Iirc this was because the only sensible search hit for ".rodata" was
there, and I didn't pay enough attention to context.)

Jan