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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.
Plus is requires respective code paths to be taken for such assertions
to trigger.

>>> 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.

Jan