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@apertussolutions.com>
---
 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();
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) ;
+
     system_state = SYS_STATE_active;
 
     domain_unpause_by_systemcontroller(dom0);
-- 
2.20.1

On 31.03.2022 01:05, Daniel P. Smith wrote:
> --- 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) ;

I think this should be stronger than ASSERT(); I'd recommend calling
panic(). Also please don't compare against "true" or "false" - use
ordinary boolean operations instead (here it would be
"!current->domain->is_privileged").

Jan

On 4/5/22 04:26, Jan Beulich wrote:
> On 31.03.2022 01:05, Daniel P. Smith wrote:
>> --- 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) ;
> 
> I think this should be stronger than ASSERT(); I'd recommend calling
> panic(). Also please don't compare against "true" or "false" - use
> ordinary boolean operations instead (here it would be
> "!current->domain->is_privileged").

Ack.

v/r,
dps

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@apertussolutions.com>
> ---
>  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.

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@apertussolutions.com>
>> ---
>>  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

Hi,

On 31/03/2022 13: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@apertussolutions.com>
>> ---
>>   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?
I think the code is based on an old version of Xen (looks like 4.14). In 
newer version create_domUs() is called before just before 
discard_initial_modules() (see XSA-372 for the rationale).

Daniel, can you please rebase this series to the latest staging?

Cheer,

-- 
Julien Grall

On Thu, 31 Mar 2022, Julien Grall wrote:
> On 31/03/2022 13: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@apertussolutions.com>
> > > ---
> > >   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?
> I think the code is based on an old version of Xen (looks like 4.14). In newer
> version create_domUs() is called before just before discard_initial_modules()
> (see XSA-372 for the rationale).
> 
> Daniel, can you please rebase this series to the latest staging?

Yeah they should be rebased. I have done it so that I could test this
approach as well, see attached.

I also added a patch that calls:

  xsm_elevate_priv(current->domain);

at the beginning of create_domUs, then calls:

  xsm_demote_priv(current->domain);

at the end of create_domUs.

With all that in place, dom0less+PV drivers works fine.

Note that I don't know if we want to do this within create_domUs of if
there is a better place, I was just trying to make sure everything works
as expected.