Re: [PATCH v6] x86/dom0: disable SMAP for PV domain building only

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/08/2024 8:31 am, Roger Pau Monné wrote:
> On Wed, Aug 28, 2024 at 07:57:39PM +0100, Andrew Cooper wrote:
>> On 28/08/2024 2:02 pm, Roger Pau Monné wrote:
>>> On Wed, Aug 28, 2024 at 12:51:23PM +0100, Andrew Cooper wrote:
>>>> On 28/08/2024 12:50 pm, Jan Beulich wrote:
>>>>> On 28.08.2024 13:30, Roger Pau Monne wrote:
>>>>>> Move the logic that disables SMAP so it's only performed when building a 
>>>>>> PV
>>>>>> dom0, PVH dom0 builder doesn't require disabling SMAP.
>>>>>>
>>>>>> The fixes tag is to account for the wrong usage of cpu_has_smap in
>>>>>> create_dom0(), it should instead have used
>>>>>> boot_cpu_has(X86_FEATURE_XEN_SMAP).  Fix while moving the logic to apply 
>>>>>> to PV
>>>>>> only.
>>>>>>
>>>>>> While there also make cr4_pv32_mask __ro_after_init.
>>>>>>
>>>>>> Fixes: 493ab190e5b1 ('xen/sm{e, a}p: allow disabling sm{e, a}p for Xen 
>>>>>> itself')
>>>>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> preferably with ...
>>>>>
>>>>>> @@ -1051,6 +1051,34 @@ out:
>>>>>>      return rc;
>>>>>>  }
>>>>>>  
>>>>>> +int __init dom0_construct_pv(struct domain *d,
>>>>>> +                             const module_t *image,
>>>>>> +                             unsigned long image_headroom,
>>>>>> +                             module_t *initrd,
>>>>>> +                             const char *cmdline)
>>>>>> +{
>>>>>> +    int rc;
>>>>>> +
>>>>>> +    /*
>>>>>> +     * Temporarily clear SMAP in CR4 to allow user-accesses in
>>>>>> +     * construct_dom0().  This saves a large number of corner cases
>>>>> ... the final 's' dropped here and ...
>>>>>
>>>>>> +     * interactions with copy_from_user().
>>
>> Actually, even with this adjustment the comment is still wonky.
>>
>> The point is that we're clearing SMAP so we *don't* need to rewrite
>> construct_dom0() in terms of copy_{to,from}_user().
>>
>> I've adjusted it.
> It did seem weird to me, I've assumed the wording was meaning to imply
> that SMAP was disabled so that construct_dom0() didn't need to use
> copy_from_user().
>
> The comment you added seems fine to me, however there's a typo I
> think:
>
>     /*
>      * Clear SMAP in CR4 to allow user-accesses in construct_dom0().  This
>      * prevents us needing to write rewrite construct_dom0() in terms of
>                               ^ extra?
>      * copy_{to,from}_user().
>      */
>
> We can fix at some later point when modifying this.

Urgh, sorry.

Luckily, I've got just the patch to do this in...

~Andrew