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 <[email protected]> > --- > CC: Jan Beulich <[email protected]> > CC: Roger Pau Monné <[email protected]> > CC: Wei Liu <[email protected]> > > 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
