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é <[email protected]>
>>>>> Reviewed-by: Jan Beulich <[email protected]>
>>>>> 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
