[PATCH] x86/boot: Restrict directmap permissions for .text/.rodata

[PATCH] x86/boot: Restrict directmap permissions for .text/.rodata

[Andrew Cooper]

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
RW.  Worse, we even had pieces of code making use of this as a feature.

Restrict the permissions, as we have no legitimate need for writeability of
these areas via the directmap alias.

Note that the pagetables and cpu0_stack do get written through their directmap
alias, so we can't just read-only the whole image.

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>

Ever so slightly RFC, as it has only had light testing.

Notes:
 * The stubs are still have RX via one alias, RW via another, and these need
   to stay.  Hardening options include splitting the stubs so the SYSCALL ones
   can be read-only after setup, and/or expanding the stub size to 4k per CPU
   so we really can keep the writeable alias as not present when the stub
   isn't in active use.
 * Future CPUs with Protection Key Supervisor (Sapphire Rapids and later)
   would be able to inhibit writeability outside of a permitted region, and
   because the protection key is per logical thread, we woulnd't need to
   expand the stubs to 4k per CPU.
 * At the time of writing, PV Shim still makes use of .rodata's read/write
   alias in the directmap to patch the hypercall table, but that runs earlier
   on boot.  Also, there are patches out to address this.
 * For backporting, this patch depends on c/s e7f147bf4ac7 ("x86/crash: Drop
   manual hooking of exception_table[]"), and nothing would break at compile
   time if the dependency was missing.
---
 xen/arch/x86/setup.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f40a9fe5d351..c8641c227d9a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1566,6 +1566,14 @@ 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.
+     */
+    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.11.0

Re: [PATCH] x86/boot: Restrict directmap permissions for .text/.rodata

[Jan Beulich]

On 06.12.2021 14: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
> RW.  Worse, we even had pieces of code making use of this as a feature.
> 
> Restrict the permissions, as we have no legitimate need for writeability of
> these areas via the directmap alias.

Where do we end up reading .text and/or .rodata through the directmap? Can't
we zap the mappings altogether?

As to superpage shattering - I understand this is not deemed to be an issue
in the common case since, with Xen moved as high up below 4G as possible,
it wouldn't normally live inside a 1G mapping anyway? This may want calling
out here. Plus, in non-EFI, non-XEN_ALIGN_2M builds isn't this going to
shatter a 2M page at the tail of .rodata?

> Note that the pagetables and cpu0_stack do get written through their directmap
> alias, so we can't just read-only the whole image.
> 
> 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>
> 
> Ever so slightly RFC, as it has only had light testing.
> 
> Notes:
>  * The stubs are still have RX via one alias, RW via another, and these need
>    to stay.  Hardening options include splitting the stubs so the SYSCALL ones
>    can be read-only after setup, and/or expanding the stub size to 4k per CPU
>    so we really can keep the writeable alias as not present when the stub
>    isn't in active use.
>  * Future CPUs with Protection Key Supervisor (Sapphire Rapids and later)
>    would be able to inhibit writeability outside of a permitted region, and
>    because the protection key is per logical thread, we woulnd't need to
>    expand the stubs to 4k per CPU.

I'm afraid I don't follow: The keys still apply to entire pages, don't they?
This would still allow write access by all 16 CPUs sharing a page for their
stubs.

>  * At the time of writing, PV Shim still makes use of .rodata's read/write
>    alias in the directmap to patch the hypercall table, but that runs earlier
>    on boot.  Also, there are patches out to address this.

I did consider committing that change, but it wasn't clear to me whether
there were dependencies on earlier parts of the series that it's part of.

>  * For backporting, this patch depends on c/s e7f147bf4ac7 ("x86/crash: Drop
>    manual hooking of exception_table[]"), and nothing would break at compile
>    time if the dependency was missing.

Hmm, not nice. I'm likely to forget if we would indeed decide to backport
the one here.

Jan

Re: [PATCH] x86/boot: Restrict directmap permissions for .text/.rodata

[Andrew Cooper]

On 06/12/2021 13:58, Jan Beulich wrote:
> On 06.12.2021 14: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
>> RW.  Worse, we even had pieces of code making use of this as a feature.
>>
>> Restrict the permissions, as we have no legitimate need for writeability of
>> these areas via the directmap alias.
> Where do we end up reading .text and/or .rodata through the directmap? Can't
> we zap the mappings altogether?

I felt it was safer to keep readability via the directmap.

I'm not aware of any logic we have which reads the directmap in order,
but it ought to be possible.

> As to superpage shattering - I understand this is not deemed to be an issue
> in the common case since, with Xen moved as high up below 4G as possible,
> it wouldn't normally live inside a 1G mapping anyway? This may want calling
> out here. Plus, in non-EFI, non-XEN_ALIGN_2M builds isn't this going to
> shatter a 2M page at the tail of .rodata?

cpu0_stack has already shattered down to 4k, which is likely in the same
superpage as rodata in a non-2M build.

But at the end of the day, it is a security/performance tradeoff.

memcpy(__va(__pa(divide_error)), "\x0f\x0b", 2);
asm ("div %ecx" :: "c" (0));

is an especially low barrier for an attacker who has a partial write gadget.

The security benefits are substantial, and the perf downsides are a
handful of extra pagetables, and a handful of pagewalks taking extra
steps, in non-fast paths (i.e. distinctly marginal).

It occurs to me while writing this that the same applies to livepatches.

>
>> Note that the pagetables and cpu0_stack do get written through their directmap
>> alias, so we can't just read-only the whole image.
>>
>> 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>
>>
>> Ever so slightly RFC, as it has only had light testing.
>>
>> Notes:
>>  * The stubs are still have RX via one alias, RW via another, and these need
>>    to stay.  Hardening options include splitting the stubs so the SYSCALL ones
>>    can be read-only after setup, and/or expanding the stub size to 4k per CPU
>>    so we really can keep the writeable alias as not present when the stub
>>    isn't in active use.
>>  * Future CPUs with Protection Key Supervisor (Sapphire Rapids and later)
>>    would be able to inhibit writeability outside of a permitted region, and
>>    because the protection key is per logical thread, we woulnd't need to
>>    expand the stubs to 4k per CPU.
> I'm afraid I don't follow: The keys still apply to entire pages, don't they?
> This would still allow write access by all 16 CPUs sharing a page for their
> stubs.

It would be all stubs, because there are only 16 protection keys and we
wouldn't want to interleave adjacent stub mappings.

The logic would now be:

pks_allow_write_access();
write new stub;
pks_revoke_write_access();

so as to limit writeability of any stub to just the critical intending
to modify it.

This way, an unrelated buggy hypercall couldn't write into the stub.

>>  * At the time of writing, PV Shim still makes use of .rodata's read/write
>>    alias in the directmap to patch the hypercall table, but that runs earlier
>>    on boot.  Also, there are patches out to address this.
> I did consider committing that change, but it wasn't clear to me whether
> there were dependencies on earlier parts of the series that it's part of.

I've got a dis-entangled version in my CET series.

https://github.com/andyhhp/xen/commit/8d55e1c8ff1d979c985b3fb75c23627348c15209

which needed some header file adjustments to build.

But yes - I too was thinking that it ought to be committed.

~Andrew

Re: [PATCH] x86/boot: Restrict directmap permissions for .text/.rodata

[Jan Beulich]

On 06.12.2021 16:11, Andrew Cooper wrote:
> On 06/12/2021 13:58, Jan Beulich wrote:
>> On 06.12.2021 14: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
>>> RW.  Worse, we even had pieces of code making use of this as a feature.
>>>
>>> Restrict the permissions, as we have no legitimate need for writeability of
>>> these areas via the directmap alias.
>> Where do we end up reading .text and/or .rodata through the directmap? Can't
>> we zap the mappings altogether?
> 
> I felt it was safer to keep readability via the directmap.
> 
> I'm not aware of any logic we have which reads the directmap in order,
> but it ought to be possible.

Could you add a sentence to this effect to this description, please?

>> As to superpage shattering - I understand this is not deemed to be an issue
>> in the common case since, with Xen moved as high up below 4G as possible,
>> it wouldn't normally live inside a 1G mapping anyway? This may want calling
>> out here. Plus, in non-EFI, non-XEN_ALIGN_2M builds isn't this going to
>> shatter a 2M page at the tail of .rodata?
> 
> cpu0_stack has already shattered down to 4k, which is likely in the same
> superpage as rodata in a non-2M build.
> 
> But at the end of the day, it is a security/performance tradeoff.
> 
> memcpy(__va(__pa(divide_error)), "\x0f\x0b", 2);
> asm ("div %ecx" :: "c" (0));
> 
> is an especially low barrier for an attacker who has a partial write gadget.
> 
> The security benefits are substantial, and the perf downsides are a
> handful of extra pagetables, and a handful of pagewalks taking extra
> steps, in non-fast paths (i.e. distinctly marginal).

How do you easily know what paths there are accessing data on the same
(potential) superpage? However, thinking about it, with the directmap
mapping presumably not getting used at all, how the mapping is arranged
doesn't really matter (except for the extra memory needed, but as you
say that's probably marginal).

Jan

Re: [PATCH] x86/boot: Restrict directmap permissions for .text/.rodata

[Andrew Cooper]

On 06/12/2021 3:21 pm, Jan Beulich wrote:
> On 06.12.2021 16:11, Andrew Cooper wrote:
>> On 06/12/2021 13:58, Jan Beulich wrote:
>>> On 06.12.2021 14: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
>>>> RW.  Worse, we even had pieces of code making use of this as a
>>>> feature.
>>>>
>>>> Restrict the permissions, as we have no legitimate need for
>>>> writeability of
>>>> these areas via the directmap alias.
>>> Where do we end up reading .text and/or .rodata through the
>>> directmap? Can't
>>> we zap the mappings altogether?
>>
>> I felt it was safer to keep readability via the directmap.
>>
>> I'm not aware of any logic we have which reads the directmap in order,
>> but it ought to be possible.
>
> Could you add a sentence to this effect to this description, please?

Ok.  The commit message a rewrite anyway, given changes to the boot cpu
stack.

>
>>> As to superpage shattering - I understand this is not deemed to be
>>> an issue
>>> in the common case since, with Xen moved as high up below 4G as
>>> possible,
>>> it wouldn't normally live inside a 1G mapping anyway? This may want
>>> calling
>>> out here. Plus, in non-EFI, non-XEN_ALIGN_2M builds isn't this going to
>>> shatter a 2M page at the tail of .rodata?
>>
>> cpu0_stack has already shattered down to 4k, which is likely in the same
>> superpage as rodata in a non-2M build.
>>
>> But at the end of the day, it is a security/performance tradeoff.
>>
>> memcpy(__va(__pa(divide_error)), "\x0f\x0b", 2);
>> asm ("div %ecx" :: "c" (0));
>>
>> is an especially low barrier for an attacker who has a partial write
>> gadget.
>>
>> The security benefits are substantial, and the perf downsides are a
>> handful of extra pagetables, and a handful of pagewalks taking extra
>> steps, in non-fast paths (i.e. distinctly marginal).
>
> How do you easily know what paths there are accessing data on the same
> (potential) superpage? However, thinking about it, with the directmap
> mapping presumably not getting used at all, how the mapping is arranged
> doesn't really matter (except for the extra memory needed, but as you
> say that's probably marginal).

Indeed.  Any path which requires Xen to reach into guest state via the
directmap isn't a fastpath in the first place.

~Andrew

Re: [PATCH] x86/boot: Restrict directmap permissions for .text/.rodata

[Jan Beulich]

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

Re: [PATCH] x86/boot: Restrict directmap permissions for .text/.rodata

[Andrew Cooper]

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

Re: [PATCH] x86/boot: Restrict directmap permissions for .text/.rodata

[Jan Beulich]

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

[PATCH] x86/boot: Restrict directmap permissions for .text/.rodata

[Andrew Cooper]

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