Re: [PATCH 2/2] arch: ensure idle domain is not left privileged

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 3/31/22 08:46, Roger Pau Monné wrote:
> On Wed, Mar 30, 2022 at 07:05:49PM -0400, Daniel P. Smith wrote:
>> It is now possible to promote the idle domain to privileged during setup.  It
>> is not desirable for the idle domain to still be privileged when moving into 
>> a
>> running state. If the idle domain was elevated and not properly demoted, it 
>> is
>> desirable to fail at this point. This commit adds an assert for both x86 and
>> Arm just before transitioning to a running state that ensures the idle is not
>> privileged.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>> ---
>>  xen/arch/arm/setup.c | 3 +++
>>  xen/arch/x86/setup.c | 3 +++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 7968cee47d..3de394e946 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -973,6 +973,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      /* Hide UART from DOM0 if we're using it */
>>      serial_endboot();
>>  
>> +    /* Ensure idle domain was not left privileged */
>> +    ASSERT(current->domain->is_privileged == false) ;
>> +
>>      system_state = SYS_STATE_active;
>>  
>>      create_domUs();
> 
> Hm, I think you want to use the permission promotion of the idle
> domain in create_domUs() likely?

Apologies, I cherry-picked this onto a branch of staging of what I
thought was an up to date remote, but as Julien pointed out I was not.

> At which point the check should be after create_domUs, and it would
> seem that logically SYS_STATE_active should be set after creating the
> domUs.
> 
> Also, FWIW, I'm not seeing this create_domUs() call in my context,
> maybe you have other patches on your queue?
> 
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 885919d5c3..b868463f83 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -589,6 +589,9 @@ static void noinline init_done(void)
>>      void *va;
>>      unsigned long start, end;
>>  
>> +    /* Ensure idle domain was not left privileged */
>> +    ASSERT(current->domain->is_privileged == false) ;
>                                                       ^ extra space.
> 
> I think you could squash this patch with the previous one and also
> squash it with a patch that actually makes use of the introduced
> permission promotion functions (or at least in a patch series where
> further patches make use the introduced functions).

Ack, I can squash them together.

v/r,
dps