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

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

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?

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

Thanks, Roger.