| Message ID | 20230324220824.3279825-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/boot: Restrict directmap permissions for .text/.rodata | expand |
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
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 )
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(+)