Re: [PATCH 03/22] x86/dom0: only disable SMAP for the PV dom0 build

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/07/2024 5:18 pm, Roger Pau Monné wrote:
> On Mon, Jul 29, 2024 at 04:52:22PM +0100, Andrew Cooper wrote:
>> On 29/07/2024 12:53 pm, Jan Beulich wrote:
>>> On 26.07.2024 17:21, Roger Pau Monne wrote:
>>>> The PVH dom0 builder doesn't switch page tables and has no need to run with
>>>> SMAP disabled.
>>>>
>>>> Put the SMAP disabling close to the code region where it's necessary, as it
>>>> then becomes obvious why switch_cr3_cr4() is required instead of
>>>> write_ptbase().
>>>>
>>>> Note removing SMAP from cr4_pv32_mask is not required, as we never jump 
>>>> into
>>>> guest context, and hence updating the value of cr4_pv32_mask is not 
>>>> relevant.
>>> I'm okay-ish with that being dropped, but iirc the goal was to keep the
>>> variable in sync with CPU state.
>> Removing SMAP from cr4_pv32_mask is necessary.
>>
>> Otherwise IST vectors will reactive SMAP behind the back of the dombuilder.
>>
>> This will probably only manifest in practice in a CONFIG_PV32=y build,
> Sorry, I'm possibly missing some context here.  When running the dom0
> builder we switch to the guest page-tables, but not to the guest vCPU,
> (iow: current == idle) and hence the context is always the Xen
> context.

Correct.

> Why would the return path of the IST use cr4_pv32_mask when the
> context in which the IST happened was the Xen one, and the current
> vCPU is the idle one (a 64bit PV guest from Xen's PoV).
>
> My understanding is that cr4_pv32_mask should only be used when the
> current context is running a 32bit PV vCPU.

This logic is evil to follow, because you need to look at both
cr4_pv32_mask and XEN_CR4_PV32_BITS to see both halves of it.

Notice how cr4_pv32_restore() only ever OR's cr4_pv32_mask into %cr4?

CR4_PV32_RESTORE is called from every entry path which *might* have come
from a 32bit PV guest, and it always results in Xen having SMEP/SMAP
active (as applicable).  This includes NMI.

The change is only undone in compat_restore_all_guest(), where
XEN_CR4_PV32_BITS is cleared from %cr4 iff returning to Ring1/2.  This
is logic cunningly disguised in the use of the Parity flag.


Because the NMI handler does reactive SMEP/SMAP (based on the value in
cr4_pv32_mask), and returning to Xen does not pass through
compat_restore_all_guest(), taking an NMI in the middle of of the
dombuilder will reactive SMAP behind your back.

~Andrew