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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> 
> 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