Re: [PATCH v2 2/2] x86/dom0: only disable SMAP for the PV dom0 build

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

On Wed, Jul 31, 2024 at 03:39:35PM +0200, Jan Beulich wrote:
> On 31.07.2024 15:25, Roger Pau Monné wrote:
> > On Wed, Jul 31, 2024 at 08:44:46AM +0200, Jan Beulich wrote:
> >> On 30.07.2024 17:28, Roger Pau Monne wrote:
> >>> The PVH dom0 builder doesn't switch page tables and has no need to run 
> >>> with
> >>> SMAP disabled.
> >>>
> >>> Use stac() and clac(), as that's safe to use even if events would hit in 
> >>> the
> >>> middle of the region with SMAP disabled.  Nesting stac() and clac() calls 
> >>> is
> >>> not safe, so the stac() call is done strictly after elf_load_binary() 
> >>> because
> >>> that uses raw_{copy_to,clear}_guest() accessors which toggle SMAP.
> >>
> >> And that was the main concern causing the CR4.SMAP override to be used 
> >> instead,
> >> iirc. While I'm sure you've properly audited all code paths, how can we be 
> >> sure
> >> there's not going to be another stac() or clac() added somewhere? Or 
> >> setting of
> >> EFLAGS as a whole, clearing EFLAGS.AC without that being explicit? I think 
> >> we'd
> >> be better off sticking to the fiddling with CR4.
> > 
> > On approach I didn't test would be to add ASSERTs in stac/clac
> > functions to ensure that the state is as intended.  IOW: for stac we
> > would assert that the AC flag is not set, while for clac we would do
> > the opposite and assert that it's set before clearing it.
> > 
> > That should detect nesting.
> 
> Yet it would also refuse non-paired uses which are in principle okay.

While such non-paired uses could be fine, it would seem to point to
other issues, as I would expect stac/clac to always be paired unless
it's a non-return path (a panic or similar).

> Plus is requires respective code paths to be taken for such assertions
> to trigger.

It does.  It seems more reliable to me to use stac/clac, rather than
fiddling with %cr4, however there's the nesting issue.  I think we
need to reach consensus as to which approach is to be used.

> >>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>
> >> Considering the bug Andrew pointed out on the code you remove from setup.c,
> >> don't we want a Fixes: tag as well?
> > 
> > No, I think the current code is correct, it was my change that was
> > incorrect.
> 
> Hmm, no I think there was an issue also before, from the cpu_has_smap
> use in the restore-CR4 conditional: We'd enable SMAP there even if on
> the command line there was "smap=hvm". While we clear FEATURE_SMAP
> when "smap=off", we keep the feature available when "smap=hvm". Thus
> we'd pointlessly write CR4 in the first if() and then enable SMAP in
> the second one, even though it wasn't enabled earlier on.

Oh yes, that one.  I was thinking about the one related to IST and
cr4_pv32_mask.  I will add the fixes tag.

Thanks, Roger.