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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jul 29, 2024 at 06:51:31PM +0100, Andrew Cooper wrote:
> 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.

After further conversations with Andrew we believe the current
disabling of X86_CR4_SMAP in %cr4 during dom0 build is not safe.

Regardless of whether cr4_pv32_mask is properly adjusted return to Xen
context from interrupt would be done with SMAP enabled if
X86_FEATURE_XEN_SMAP is set.

I will send a new patch that uses stac/clac in order to disable SMAP
(if required) around the dom0 builder code that switches to the guest
page-tables.

Thanks, Roger.