[PATCH 0/2] Introduce XSM ability for domain privilege escalation

[PATCH 0/2] Introduce XSM ability for domain privilege escalation

[Daniel P. Smith]

This series introduces a pair of functions that allow a domain to be escalated to
is_privileged or demoted. Internally the functions enforce the policy that this
is only allowed for system domains, the idle domain in particular.

As for the implementation, there is a desire that the logic does not persist after
__init code is jettison after setup. This has to be balanced with the fact there is no
.c unit files for XSM when only the default policy is in use, i.e. CONFIG_XSM is not
set. To balance this the functions were implemented as always_inline functions in xsm.h.
This should ensure that if the only usage of these functions are in __init code, there
should be no instances of this logic present after __init code is jettisoned. Since
this introduces the ability to elevate the idle domain to is_privileged, this should
not be left in place when transitioning into the running state. As such, a pair of
ASSERTs were introduced, one each, for x86 and Arm to ensure that the idle domain
isn't inadvertently left with is_privileged being true.

Daniel P. Smith (2):
  xsm: add ability to elevate a domain to privileged
  arch: ensure idle domain is not left privileged

 xen/arch/arm/setup.c  |  3 +++
 xen/arch/x86/setup.c  |  3 +++
 xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
 3 files changed, 28 insertions(+)

-- 
2.20.1

[PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Daniel P. Smith]

There are now instances where internal hypervisor logic needs to make resource
allocation calls that are protected by XSM checks. The internal hypervisor logic
is represented a number of system domains which by designed are represented by
non-privileged struct domain instances. To enable these logic blocks to
function correctly but in a controlled manner, this commit introduces a pair
of privilege escalation and demotion functions that will make a system domain
privileged and then remove that privilege.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index e22d6160b5..157e57151e 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -189,6 +189,28 @@ struct xsm_operations {
 #endif
 };
 
+static always_inline int xsm_elevate_priv(struct domain *d)
+{
+    if ( is_system_domain(d) )
+    {
+        d->is_privileged = true;
+        return 0;
+    }
+
+    return -EPERM;
+}
+
+static always_inline int xsm_demote_priv(struct domain *d)
+{
+    if ( is_system_domain(d) )
+    {
+        d->is_privileged = false;
+        return 0;
+    }
+
+    return -EPERM;
+}
+
 #ifdef CONFIG_XSM
 
 extern struct xsm_operations *xsm_ops;
-- 
2.20.1

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

[Daniel P. Smith]

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

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Roger Pau Monné]

On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
> There are now instances where internal hypervisor logic needs to make resource
> allocation calls that are protected by XSM checks. The internal hypervisor logic
> is represented a number of system domains which by designed are represented by
> non-privileged struct domain instances. To enable these logic blocks to
> function correctly but in a controlled manner, this commit introduces a pair
> of privilege escalation and demotion functions that will make a system domain
> privileged and then remove that privilege.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>  xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++

I'm not sure this needs to be in xsm code, AFAICT it could live in a
more generic file.

>  1 file changed, 22 insertions(+)
> 
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index e22d6160b5..157e57151e 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -189,6 +189,28 @@ struct xsm_operations {
>  #endif
>  };
>  
> +static always_inline int xsm_elevate_priv(struct domain *d)

I don't think it needs to be always_inline, using just inline would be
fine IMO.

Also this needs to be __init.

> +{
> +    if ( is_system_domain(d) )
> +    {
> +        d->is_privileged = true;
> +        return 0;
> +    }
> +

I would add an ASSERT_UNREACHABLE(); here, I don't think we have any
use case for trying to elevate the privileges of a non-system domain.

Thanks, Roger.

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

[Roger Pau Monné]

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.

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

[Julien Grall]

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

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Jason Andryuk]

On Wed, Mar 30, 2022 at 3:05 PM Daniel P. Smith
<dpsmith@apertussolutions.com> wrote:
>
> There are now instances where internal hypervisor logic needs to make resource
> allocation calls that are protected by XSM checks. The internal hypervisor logic
> is represented a number of system domains which by designed are represented by
> non-privileged struct domain instances. To enable these logic blocks to
> function correctly but in a controlled manner, this commit introduces a pair
> of privilege escalation and demotion functions that will make a system domain
> privileged and then remove that privilege.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>  xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index e22d6160b5..157e57151e 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -189,6 +189,28 @@ struct xsm_operations {
>  #endif
>  };
>
> +static always_inline int xsm_elevate_priv(struct domain *d)
> +{
> +    if ( is_system_domain(d) )
> +    {
> +        d->is_privileged = true;
> +        return 0;
> +    }
> +
> +    return -EPERM;
> +}

These look sufficient for the default policy, but they don't seem
sufficient for Flask.  I think you need to create a new XSM hook.  For
Flask, you would want the demote hook to transition xen_boot_t ->
xen_t.  That would start xen_boot_t with privileges that are dropped
in a one-way transition.  Does that require all policies to then have
xen_boot_t and xen_t?  I guess it does unless the hook code has some
logic to skip the transition.

For the default policy, you could start by creating the system domains
as privileged and just have a single hook to drop privs.  Then you
don't have to worry about the "elevate" hook existing.  The patch 2
asserts could instead become the location of xsm_drop_privs calls to
have a clear demarcation point.  That expands the window with
privileges though.  It's a little simpler, but maybe you don't want
that.  However, it seems like you can only depriv once for the Flask
case since you want it to be one-way.

Regards,
Jason

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2540 bytes --]

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0001-xsm-add-ability-to-elevate-a-domain-to-privileged.patch, Size: 1603 bytes --]

From a525c5a04a8483ee9217b0be6deb12d665e3fd72 Mon Sep 17 00:00:00 2001
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
Date: Thu, 31 Mar 2022 13:20:14 -0700
Subject: [PATCH 1/3] xsm: add ability to elevate a domain to privileged

There are now instances where internal hypervisor logic needs to make resource
allocation calls that are protected by XSM checks. The internal hypervisor logic
is represented a number of system domains which by designed are represented by
non-privileged struct domain instances. To enable these logic blocks to
function correctly but in a controlled manner, this commit introduces a pair
of privilege escalation and demotion functions that will make a system domain
privileged and then remove that privilege.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>

---
 xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 3e2b7fe3db..505dfd8308 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -202,6 +202,28 @@ struct xsm_ops {
 #endif
 };
 
+static always_inline int xsm_elevate_priv(struct domain *d)
+{
+    if ( is_system_domain(d) )
+    {
+        d->is_privileged = true;
+        return 0;
+    }
+
+    return -EPERM;
+}
+
+static always_inline int xsm_demote_priv(struct domain *d)
+{
+    if ( is_system_domain(d) )
+    {
+        d->is_privileged = false;
+        return 0;
+    }
+
+    return -EPERM;
+}
+
 #ifdef CONFIG_XSM
 
 extern struct xsm_ops xsm_ops;
-- 
2.25.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-diff; name=0002-arch-ensure-idle-domain-is-not-left-privileged.patch, Size: 1784 bytes --]

From 568a94a0b3fe8de8f69fda4a24d8856272172dfb Mon Sep 17 00:00:00 2001
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
Date: Thu, 31 Mar 2022 13:20:44 -0700
Subject: [PATCH 2/3] arch: ensure idle domain is not left privileged

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 d5d0792ed4..8b9edf5ff3 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -1048,6 +1048,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;
 
     for_each_domain( d )
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index a0ee8d206f..aab61e53a9 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -649,6 +649,9 @@ static void noreturn 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.25.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Type: text/x-diff; name=0003-xen-arm-temporarily-elevate-idle_domain-privileged-d.patch, Size: 1554 bytes --]

From 887a83d88d5dc6331c9296b1f513e3bda1ae75e8 Mon Sep 17 00:00:00 2001
From: Stefano Stabellini <stefano.stabellini@xilinx.com>
Date: Thu, 31 Mar 2022 13:23:17 -0700
Subject: [PATCH 3/3] xen/arm: temporarily elevate idle_domain privileged
 during create_domUs

create_domUs might call functions that perform XSM checks on the current
domain, which is idle_domain at this time. Temporarily elevate
idle_domain privileges in create_domUs.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 xen/arch/arm/domain_build.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 03c023440d..9a49ee7dcb 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -28,6 +28,7 @@
 #include <asm/cpufeature.h>
 #include <asm/domain_build.h>
 #include <xen/event.h>
+#include <xsm/xsm.h>
 
 #include <xen/irq.h>
 #include <xen/grant_table.h>
@@ -3254,6 +3255,8 @@ void __init create_domUs(void)
     struct dt_device_node *node;
     const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
 
+    xsm_elevate_priv(current->domain);
+
     BUG_ON(chosen == NULL);
     dt_for_each_child_node(chosen, node)
     {
@@ -3335,6 +3338,8 @@ void __init create_domUs(void)
         if ( construct_domU(d, node) != 0 )
             panic("Could not set up domain %s\n", dt_node_name(node));
     }
+
+    xsm_demote_priv(current->domain);
 }
 
 static int __init construct_dom0(struct domain *d)
-- 
2.25.1

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Julien Grall]

Hi,

On 31/03/2022 13:36, Roger Pau Monné wrote:
> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
>> There are now instances where internal hypervisor logic needs to make resource
>> allocation calls that are protected by XSM checks. The internal hypervisor logic
>> is represented a number of system domains which by designed are represented by
>> non-privileged struct domain instances. To enable these logic blocks to
>> function correctly but in a controlled manner, this commit introduces a pair
>> of privilege escalation and demotion functions that will make a system domain
>> privileged and then remove that privilege.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>   xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
> 
> I'm not sure this needs to be in xsm code, AFAICT it could live in a
> more generic file.
> 
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>> index e22d6160b5..157e57151e 100644
>> --- a/xen/include/xsm/xsm.h
>> +++ b/xen/include/xsm/xsm.h
>> @@ -189,6 +189,28 @@ struct xsm_operations {
>>   #endif
>>   };
>>   
>> +static always_inline int xsm_elevate_priv(struct domain *d)
> 
> I don't think it needs to be always_inline, using just inline would be
> fine IMO.
> 
> Also this needs to be __init.

Hmmm.... I thought adding __init on function defined in header was 
pointless. In particular, if the compiler decides to inline it.

In any case, I think it would be good to check that the system_state < 
SYS_state_active (could potentially be an ASSERT()) to prevent any misuse.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Julien Grall]

Hi Daniel,

On 31/03/2022 00:05, Daniel P. Smith wrote:
> There are now instances where internal hypervisor logic needs to make resource
> allocation calls that are protected by XSM checks. The internal hypervisor logic
> is represented a number of system domains which by designed are represented by
> non-privileged struct domain instances. To enable these logic blocks to
> function correctly but in a controlled manner, this commit introduces a pair
> of privilege escalation and demotion functions that will make a system domain
> privileged and then remove that privilege.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>   xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index e22d6160b5..157e57151e 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -189,6 +189,28 @@ struct xsm_operations {
>   #endif
>   };
>   
> +static always_inline int xsm_elevate_priv(struct domain *d)
> +{
> +    if ( is_system_domain(d) )
> +    {
> +        d->is_privileged = true;

The call for xsm_elevate_priv() cannot be nested. So I would suggest to 
check if d->is_privileged is already true and return -EBUSY in this case.

> +        return 0;
> +    }
> +
> +    return -EPERM;
> +}
> +
> +static always_inline int xsm_demote_priv(struct domain *d)
> +{
> +    if ( is_system_domain(d) )
> +    {
> +        d->is_privileged = false;
> +        return 0;
> +    }
> +
> +    return -EPERM;
> +}
> +
>   #ifdef CONFIG_XSM
>   
>   extern struct xsm_operations *xsm_ops;

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Roger Pau Monné]

On Fri, Apr 01, 2022 at 06:52:46PM +0100, Julien Grall wrote:
> Hi,
> 
> On 31/03/2022 13:36, Roger Pau Monné wrote:
> > On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
> > > There are now instances where internal hypervisor logic needs to make resource
> > > allocation calls that are protected by XSM checks. The internal hypervisor logic
> > > is represented a number of system domains which by designed are represented by
> > > non-privileged struct domain instances. To enable these logic blocks to
> > > function correctly but in a controlled manner, this commit introduces a pair
> > > of privilege escalation and demotion functions that will make a system domain
> > > privileged and then remove that privilege.
> > > 
> > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> > > ---
> > >   xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
> > 
> > I'm not sure this needs to be in xsm code, AFAICT it could live in a
> > more generic file.
> > 
> > >   1 file changed, 22 insertions(+)
> > > 
> > > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> > > index e22d6160b5..157e57151e 100644
> > > --- a/xen/include/xsm/xsm.h
> > > +++ b/xen/include/xsm/xsm.h
> > > @@ -189,6 +189,28 @@ struct xsm_operations {
> > >   #endif
> > >   };
> > > +static always_inline int xsm_elevate_priv(struct domain *d)
> > 
> > I don't think it needs to be always_inline, using just inline would be
> > fine IMO.
> > 
> > Also this needs to be __init.
> 
> Hmmm.... I thought adding __init on function defined in header was
> pointless. In particular, if the compiler decides to inline it.

Indeed, I didn't realize, thanks for pointing this out.

> In any case, I think it would be good to check that the system_state <
> SYS_state_active (could potentially be an ASSERT()) to prevent any misuse.

My preference would be to make those non-inline then I think, we could
place them in common/domain.c maybe? There's no performance reason to
have those helpers as inline.

Another option would be what Jason suggested about creating the idle
domain as privileged and then dropping the privileges before starting
any domains (ie: before setting the system as ACTIVE).

This expands the duration the idle domain context is marked as
privileged, but OTOH we don't need to add a hook to set
is_privileged.

Thanks, Roger.

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Jan Beulich]

On 04.04.2022 10:08, Roger Pau Monné wrote:
> On Fri, Apr 01, 2022 at 06:52:46PM +0100, Julien Grall wrote:
>> Hi,
>>
>> On 31/03/2022 13:36, Roger Pau Monné wrote:
>>> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
>>>> There are now instances where internal hypervisor logic needs to make resource
>>>> allocation calls that are protected by XSM checks. The internal hypervisor logic
>>>> is represented a number of system domains which by designed are represented by
>>>> non-privileged struct domain instances. To enable these logic blocks to
>>>> function correctly but in a controlled manner, this commit introduces a pair
>>>> of privilege escalation and demotion functions that will make a system domain
>>>> privileged and then remove that privilege.
>>>>
>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>>> ---
>>>>   xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
>>>
>>> I'm not sure this needs to be in xsm code, AFAICT it could live in a
>>> more generic file.
>>>
>>>>   1 file changed, 22 insertions(+)
>>>>
>>>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>>>> index e22d6160b5..157e57151e 100644
>>>> --- a/xen/include/xsm/xsm.h
>>>> +++ b/xen/include/xsm/xsm.h
>>>> @@ -189,6 +189,28 @@ struct xsm_operations {
>>>>   #endif
>>>>   };
>>>> +static always_inline int xsm_elevate_priv(struct domain *d)
>>>
>>> I don't think it needs to be always_inline, using just inline would be
>>> fine IMO.
>>>
>>> Also this needs to be __init.
>>
>> Hmmm.... I thought adding __init on function defined in header was
>> pointless. In particular, if the compiler decides to inline it.
> 
> Indeed, I didn't realize, thanks for pointing this out.

The question isn't header or not, but declaration or definition.
Attributes like this one are meaningless on declarations (at least
on all the arches we care about; there may be subtleties), but
meaningful for definitions. Iirc even with always_inline the
compiler may find reasons why a function cannot be inlined, and
hence the intended section should be specified. Plus such an
annotation serves a documentation purpose.

Jan

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Daniel P. Smith]

On 3/31/22 08:36, Roger Pau Monné wrote:
> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
>> There are now instances where internal hypervisor logic needs to make resource
>> allocation calls that are protected by XSM checks. The internal hypervisor logic
>> is represented a number of system domains which by designed are represented by
>> non-privileged struct domain instances. To enable these logic blocks to
>> function correctly but in a controlled manner, this commit introduces a pair
>> of privilege escalation and demotion functions that will make a system domain
>> privileged and then remove that privilege.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>  xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
> 
> I'm not sure this needs to be in xsm code, AFAICT it could live in a
> more generic file.

From my perspective this is access control logic, thus why I advocate
for it to be under XSM. A personal goal is to try to get all security,
i.e. access control, centralized to the extent that it doing so does not
make the code base unnecessarily complicated.

>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>> index e22d6160b5..157e57151e 100644
>> --- a/xen/include/xsm/xsm.h
>> +++ b/xen/include/xsm/xsm.h
>> @@ -189,6 +189,28 @@ struct xsm_operations {
>>  #endif
>>  };
>>  
>> +static always_inline int xsm_elevate_priv(struct domain *d)
> 
> I don't think it needs to be always_inline, using just inline would be
> fine IMO.
> 
> Also this needs to be __init.

AIUI always_inline is likely the best way to preserve the speculation
safety brought in by the call to is_system_domain().

>> +{
>> +    if ( is_system_domain(d) )
>> +    {
>> +        d->is_privileged = true;
>> +        return 0;
>> +    }
>> +
> 
> I would add an ASSERT_UNREACHABLE(); here, I don't think we have any
> use case for trying to elevate the privileges of a non-system domain.

Ack.

v/r
dps

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

[Daniel P. Smith]

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

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Roger Pau Monné]

On Mon, Apr 04, 2022 at 10:21:18AM -0400, Daniel P. Smith wrote:
> On 3/31/22 08:36, Roger Pau Monné wrote:
> > On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
> >> There are now instances where internal hypervisor logic needs to make resource
> >> allocation calls that are protected by XSM checks. The internal hypervisor logic
> >> is represented a number of system domains which by designed are represented by
> >> non-privileged struct domain instances. To enable these logic blocks to
> >> function correctly but in a controlled manner, this commit introduces a pair
> >> of privilege escalation and demotion functions that will make a system domain
> >> privileged and then remove that privilege.
> >>
> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> >> ---
> >>  xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
> > 
> > I'm not sure this needs to be in xsm code, AFAICT it could live in a
> > more generic file.
> 
> From my perspective this is access control logic, thus why I advocate
> for it to be under XSM. A personal goal is to try to get all security,
> i.e. access control, centralized to the extent that it doing so does not
> make the code base unnecessarily complicated.

Maybe others have a different opinion, but IMO setting is_privileged
is not XSM specific. It happens to solve an XSM issue, but that's no
reason to place it in the xsm code base.

> >>  1 file changed, 22 insertions(+)
> >>
> >> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> >> index e22d6160b5..157e57151e 100644
> >> --- a/xen/include/xsm/xsm.h
> >> +++ b/xen/include/xsm/xsm.h
> >> @@ -189,6 +189,28 @@ struct xsm_operations {
> >>  #endif
> >>  };
> >>  
> >> +static always_inline int xsm_elevate_priv(struct domain *d)
> > 
> > I don't think it needs to be always_inline, using just inline would be
> > fine IMO.
> > 
> > Also this needs to be __init.
> 
> AIUI always_inline is likely the best way to preserve the speculation
> safety brought in by the call to is_system_domain().

There's nothing related to speculation safety in is_system_domain()
AFAICT. It's just a plain check against d->domain_id. It's my
understanding there's no need for any speculation barrier there
because d->domain_id is not an external input.

In any case this function should be __init only, at which point there
are no untrusted inputs to Xen.

Thanks, Roger.

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Jan Beulich]

On 04.04.2022 17:12, Roger Pau Monné wrote:
> On Mon, Apr 04, 2022 at 10:21:18AM -0400, Daniel P. Smith wrote:
>> On 3/31/22 08:36, Roger Pau Monné wrote:
>>> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
>>>> There are now instances where internal hypervisor logic needs to make resource
>>>> allocation calls that are protected by XSM checks. The internal hypervisor logic
>>>> is represented a number of system domains which by designed are represented by
>>>> non-privileged struct domain instances. To enable these logic blocks to
>>>> function correctly but in a controlled manner, this commit introduces a pair
>>>> of privilege escalation and demotion functions that will make a system domain
>>>> privileged and then remove that privilege.
>>>>
>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>>> ---
>>>>  xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
>>>
>>> I'm not sure this needs to be in xsm code, AFAICT it could live in a
>>> more generic file.
>>
>> From my perspective this is access control logic, thus why I advocate
>> for it to be under XSM. A personal goal is to try to get all security,
>> i.e. access control, centralized to the extent that it doing so does not
>> make the code base unnecessarily complicated.
> 
> Maybe others have a different opinion, but IMO setting is_privileged
> is not XSM specific. It happens to solve an XSM issue, but that's no
> reason to place it in the xsm code base.

To be honest I can see justification for either placement.

>>>>  1 file changed, 22 insertions(+)
>>>>
>>>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>>>> index e22d6160b5..157e57151e 100644
>>>> --- a/xen/include/xsm/xsm.h
>>>> +++ b/xen/include/xsm/xsm.h
>>>> @@ -189,6 +189,28 @@ struct xsm_operations {
>>>>  #endif
>>>>  };
>>>>  
>>>> +static always_inline int xsm_elevate_priv(struct domain *d)
>>>
>>> I don't think it needs to be always_inline, using just inline would be
>>> fine IMO.
>>>
>>> Also this needs to be __init.
>>
>> AIUI always_inline is likely the best way to preserve the speculation
>> safety brought in by the call to is_system_domain().
> 
> There's nothing related to speculation safety in is_system_domain()
> AFAICT. It's just a plain check against d->domain_id. It's my
> understanding there's no need for any speculation barrier there
> because d->domain_id is not an external input.

Whether is_system_domain() wants hardening really depends on where
it's used. It doesn't matter as much what specific check an is_...()
function performs, but what code paths it sits on where taking the
wrong branch of a conditional could reveal data which shouldn't be
visible.

Jan

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Daniel P. Smith]

On 3/31/22 09:16, Jason Andryuk wrote:
> On Wed, Mar 30, 2022 at 3:05 PM Daniel P. Smith
> <dpsmith@apertussolutions.com> wrote:
>>
>> There are now instances where internal hypervisor logic needs to make resource
>> allocation calls that are protected by XSM checks. The internal hypervisor logic
>> is represented a number of system domains which by designed are represented by
>> non-privileged struct domain instances. To enable these logic blocks to
>> function correctly but in a controlled manner, this commit introduces a pair
>> of privilege escalation and demotion functions that will make a system domain
>> privileged and then remove that privilege.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>  xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>> index e22d6160b5..157e57151e 100644
>> --- a/xen/include/xsm/xsm.h
>> +++ b/xen/include/xsm/xsm.h
>> @@ -189,6 +189,28 @@ struct xsm_operations {
>>  #endif
>>  };
>>
>> +static always_inline int xsm_elevate_priv(struct domain *d)
>> +{
>> +    if ( is_system_domain(d) )
>> +    {
>> +        d->is_privileged = true;
>> +        return 0;
>> +    }
>> +
>> +    return -EPERM;
>> +}
> 
> These look sufficient for the default policy, but they don't seem
> sufficient for Flask.  I think you need to create a new XSM hook.  For
> Flask, you would want the demote hook to transition xen_boot_t ->
> xen_t.  That would start xen_boot_t with privileges that are dropped
> in a one-way transition.  Does that require all policies to then have
> xen_boot_t and xen_t?  I guess it does unless the hook code has some
> logic to skip the transition.

I am still thinking this through but my initial concern for Flask is
that I don't think we want dedicated domain transitions directly in
code. My current thinking would be to use a Kconfig to use xen_boot_t
type as the initial sid for the idle domain which would then require the
default policy to include an allowed transition from xen_boot_t to
xen_t. Then rely upon a boot domain to issue an xsm_op to do a relabel
transition for the idle domain with an assertion that the idle domain is
no longer labeled with its initial sid before Xen transitions its state
to SYS_STATE_active. The one wrinkle to this is whether I will be able
to schedule the boot domain before allowing Xen to transition into
SYS_STATE_active.

> For the default policy, you could start by creating the system domains
> as privileged and just have a single hook to drop privs.  Then you
> don't have to worry about the "elevate" hook existing.  The patch 2
> asserts could instead become the location of xsm_drop_privs calls to
> have a clear demarcation point.  That expands the window with
> privileges though.  It's a little simpler, but maybe you don't want
> that.  However, it seems like you can only depriv once for the Flask
> case since you want it to be one-way.

This does simplify the solution and since today we cannot differentiate
between hypervisor setup and hypervisor initiated domain construction
contexts, it does not run counter to what I have proposed. As for flask,
again I do not believe codifying a domain transition bound to a new XSM
op is the appropriate approach.

v/r,
dps

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Daniel P. Smith]

On 4/4/22 11:12, Roger Pau Monné wrote:
> On Mon, Apr 04, 2022 at 10:21:18AM -0400, Daniel P. Smith wrote:
>> On 3/31/22 08:36, Roger Pau Monné wrote:
>>> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
>>>> There are now instances where internal hypervisor logic needs to make resource
>>>> allocation calls that are protected by XSM checks. The internal hypervisor logic
>>>> is represented a number of system domains which by designed are represented by
>>>> non-privileged struct domain instances. To enable these logic blocks to
>>>> function correctly but in a controlled manner, this commit introduces a pair
>>>> of privilege escalation and demotion functions that will make a system domain
>>>> privileged and then remove that privilege.
>>>>
>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>>> ---
>>>>  xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
>>>
>>> I'm not sure this needs to be in xsm code, AFAICT it could live in a
>>> more generic file.
>>
>> From my perspective this is access control logic, thus why I advocate
>> for it to be under XSM. A personal goal is to try to get all security,
>> i.e. access control, centralized to the extent that it doing so does not
>> make the code base unnecessarily complicated.
> 
> Maybe others have a different opinion, but IMO setting is_privileged
> is not XSM specific. It happens to solve an XSM issue, but that's no
> reason to place it in the xsm code base.

I think this deserves a separate more dedicated thread as I would take
the position that Xen privilege model is at best fractured because
is_control_domain() (and underlying is_privileged),
is_hardware_domain,() and is_xenstore_domain() are used independent of
XSM. Perhaps the discussion can be had when I get back to the XSM Roles
patches to enable disaggregating Xen with hyperlaunch (or at a Xen
Summit design session).

>>>>  1 file changed, 22 insertions(+)
>>>>
>>>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>>>> index e22d6160b5..157e57151e 100644
>>>> --- a/xen/include/xsm/xsm.h
>>>> +++ b/xen/include/xsm/xsm.h
>>>> @@ -189,6 +189,28 @@ struct xsm_operations {
>>>>  #endif
>>>>  };
>>>>  
>>>> +static always_inline int xsm_elevate_priv(struct domain *d)
>>>
>>> I don't think it needs to be always_inline, using just inline would be
>>> fine IMO.
>>>
>>> Also this needs to be __init.
>>
>> AIUI always_inline is likely the best way to preserve the speculation
>> safety brought in by the call to is_system_domain().
> 
> There's nothing related to speculation safety in is_system_domain()
> AFAICT. It's just a plain check against d->domain_id. It's my
> understanding there's no need for any speculation barrier there
> because d->domain_id is not an external input.

Hmmm, this actually raises a good question. Why is is_control_domain(),
is_hardware_domain, and others all have evaluate_nospec() wrapping the
check of a struct domain element while is_system_domain() does not?

> In any case this function should be __init only, at which point there
> are no untrusted inputs to Xen.

I thought it was agreed that __init on inline functions in headers had
no meaning?

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Roger Pau Monné]

On Mon, Apr 04, 2022 at 12:08:25PM -0400, Daniel P. Smith wrote:
> On 4/4/22 11:12, Roger Pau Monné wrote:
> > On Mon, Apr 04, 2022 at 10:21:18AM -0400, Daniel P. Smith wrote:
> >> On 3/31/22 08:36, Roger Pau Monné wrote:
> >>> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
> >>>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> >>>> index e22d6160b5..157e57151e 100644
> >>>> --- a/xen/include/xsm/xsm.h
> >>>> +++ b/xen/include/xsm/xsm.h
> >>>> @@ -189,6 +189,28 @@ struct xsm_operations {
> >>>>  #endif
> >>>>  };
> >>>>  
> >>>> +static always_inline int xsm_elevate_priv(struct domain *d)
> >>>
> >>> I don't think it needs to be always_inline, using just inline would be
> >>> fine IMO.
> >>>
> >>> Also this needs to be __init.
> >>
> >> AIUI always_inline is likely the best way to preserve the speculation
> >> safety brought in by the call to is_system_domain().
> > 
> > There's nothing related to speculation safety in is_system_domain()
> > AFAICT. It's just a plain check against d->domain_id. It's my
> > understanding there's no need for any speculation barrier there
> > because d->domain_id is not an external input.
> 
> Hmmm, this actually raises a good question. Why is is_control_domain(),
> is_hardware_domain, and others all have evaluate_nospec() wrapping the
> check of a struct domain element while is_system_domain() does not?

Jan replied to this regard, see:

https://lore.kernel.org/xen-devel/54272d08-7ce1-b162-c8e9-1955b780ca11@suse.com/

> > In any case this function should be __init only, at which point there
> > are no untrusted inputs to Xen.
> 
> I thought it was agreed that __init on inline functions in headers had
> no meaning?

In a different reply I already noted my preference would be for the
function to not reside in a header and not be inline, simply because
it would be gone after initialization and we won't have to worry about
any stray calls when the system is active.

Thanks, Roger.

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

[Jan Beulich]

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

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Daniel P. Smith]

On 4/5/22 03:42, Roger Pau Monné wrote:
> On Mon, Apr 04, 2022 at 12:08:25PM -0400, Daniel P. Smith wrote:
>> On 4/4/22 11:12, Roger Pau Monné wrote:
>>> On Mon, Apr 04, 2022 at 10:21:18AM -0400, Daniel P. Smith wrote:
>>>> On 3/31/22 08:36, Roger Pau Monné wrote:
>>>>> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
>>>>>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>>>>>> index e22d6160b5..157e57151e 100644
>>>>>> --- a/xen/include/xsm/xsm.h
>>>>>> +++ b/xen/include/xsm/xsm.h
>>>>>> @@ -189,6 +189,28 @@ struct xsm_operations {
>>>>>>  #endif
>>>>>>  };
>>>>>>  
>>>>>> +static always_inline int xsm_elevate_priv(struct domain *d)
>>>>>
>>>>> I don't think it needs to be always_inline, using just inline would be
>>>>> fine IMO.
>>>>>
>>>>> Also this needs to be __init.
>>>>
>>>> AIUI always_inline is likely the best way to preserve the speculation
>>>> safety brought in by the call to is_system_domain().
>>>
>>> There's nothing related to speculation safety in is_system_domain()
>>> AFAICT. It's just a plain check against d->domain_id. It's my
>>> understanding there's no need for any speculation barrier there
>>> because d->domain_id is not an external input.
>>
>> Hmmm, this actually raises a good question. Why is is_control_domain(),
>> is_hardware_domain, and others all have evaluate_nospec() wrapping the
>> check of a struct domain element while is_system_domain() does not?
> 
> Jan replied to this regard, see:
> 
> https://lore.kernel.org/xen-devel/54272d08-7ce1-b162-c8e9-1955b780ca11@suse.com/

Jan can correct me if I misunderstood, but his point is with respect to
where the inline function will be expanded into and I would think you
would want to ensure that if anyone were to use is_system_domain(), then
the inline expansion of this new location could create a potential
speculation-able branch. Basically my concern is not putting the guards
in place today just because there is not currently any location where
is_system_domain() is expanded to create a speculation opportunity does
not mean there is not an opening for the opportunity down the road for a
future unprotected use.

>>> In any case this function should be __init only, at which point there
>>> are no untrusted inputs to Xen.
>>
>> I thought it was agreed that __init on inline functions in headers had
>> no meaning?
> 
> In a different reply I already noted my preference would be for the
> function to not reside in a header and not be inline, simply because
> it would be gone after initialization and we won't have to worry about
> any stray calls when the system is active.

If an inline function is only used by __init code, how would be
available for stray calls when the system is active? I would concede
that it is possible for someone to explicitly use in not __init code but
I would like to believe any usage in a submitted code change would be
questioned by the reviewers.

With that said, if we consider Jason's suggestion would this remove your
concern since that would only introduce a de-privilege function and
there would be no piv escalation that could be erroneously called at
anytime?

v/r
dps

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

[Daniel P. Smith]

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

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Roger Pau Monné]

On Tue, Apr 05, 2022 at 08:06:31AM -0400, Daniel P. Smith wrote:
> On 4/5/22 03:42, Roger Pau Monné wrote:
> > On Mon, Apr 04, 2022 at 12:08:25PM -0400, Daniel P. Smith wrote:
> >> On 4/4/22 11:12, Roger Pau Monné wrote:
> >>> On Mon, Apr 04, 2022 at 10:21:18AM -0400, Daniel P. Smith wrote:
> >>>> On 3/31/22 08:36, Roger Pau Monné wrote:
> >>>>> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
> >>>>>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> >>>>>> index e22d6160b5..157e57151e 100644
> >>>>>> --- a/xen/include/xsm/xsm.h
> >>>>>> +++ b/xen/include/xsm/xsm.h
> >>>>>> @@ -189,6 +189,28 @@ struct xsm_operations {
> >>>>>>  #endif
> >>>>>>  };
> >>>>>>  
> >>>>>> +static always_inline int xsm_elevate_priv(struct domain *d)
> >>>>>
> >>>>> I don't think it needs to be always_inline, using just inline would be
> >>>>> fine IMO.
> >>>>>
> >>>>> Also this needs to be __init.
> >>>>
> >>>> AIUI always_inline is likely the best way to preserve the speculation
> >>>> safety brought in by the call to is_system_domain().
> >>>
> >>> There's nothing related to speculation safety in is_system_domain()
> >>> AFAICT. It's just a plain check against d->domain_id. It's my
> >>> understanding there's no need for any speculation barrier there
> >>> because d->domain_id is not an external input.
> >>
> >> Hmmm, this actually raises a good question. Why is is_control_domain(),
> >> is_hardware_domain, and others all have evaluate_nospec() wrapping the
> >> check of a struct domain element while is_system_domain() does not?
> > 
> > Jan replied to this regard, see:
> > 
> > https://lore.kernel.org/xen-devel/54272d08-7ce1-b162-c8e9-1955b780ca11@suse.com/
> 
> Jan can correct me if I misunderstood, but his point is with respect to
> where the inline function will be expanded into and I would think you
> would want to ensure that if anyone were to use is_system_domain(), then
> the inline expansion of this new location could create a potential
> speculation-able branch. Basically my concern is not putting the guards
> in place today just because there is not currently any location where
> is_system_domain() is expanded to create a speculation opportunity does
> not mean there is not an opening for the opportunity down the road for a
> future unprotected use.
> 
> >>> In any case this function should be __init only, at which point there
> >>> are no untrusted inputs to Xen.
> >>
> >> I thought it was agreed that __init on inline functions in headers had
> >> no meaning?
> > 
> > In a different reply I already noted my preference would be for the
> > function to not reside in a header and not be inline, simply because
> > it would be gone after initialization and we won't have to worry about
> > any stray calls when the system is active.
> 
> If an inline function is only used by __init code, how would be
> available for stray calls when the system is active? I would concede
> that it is possible for someone to explicitly use in not __init code but
> I would like to believe any usage in a submitted code change would be
> questioned by the reviewers.

Right, it's IMO easier when things just explode when not used
correctly, hence my suggestion to make it __init.

> With that said, if we consider Jason's suggestion would this remove your
> concern since that would only introduce a de-privilege function and
> there would be no piv escalation that could be erroneously called at
> anytime?

Indeed.  IMO everything that happens before the system switches to the
active state should be considered to be running in a privileged
context anyway.  Maybe others have different opinions.  Or maybe there
are use-cases I'm not aware of where this is not true.

Thanks, Roger.

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Jason Andryuk]

On Mon, Apr 4, 2022 at 11:34 AM Daniel P. Smith
<dpsmith@apertussolutions.com> wrote:
>
> On 3/31/22 09:16, Jason Andryuk wrote:
> > On Wed, Mar 30, 2022 at 3:05 PM Daniel P. Smith
> > <dpsmith@apertussolutions.com> wrote:
> >>
> >> There are now instances where internal hypervisor logic needs to make resource
> >> allocation calls that are protected by XSM checks. The internal hypervisor logic
> >> is represented a number of system domains which by designed are represented by
> >> non-privileged struct domain instances. To enable these logic blocks to
> >> function correctly but in a controlled manner, this commit introduces a pair
> >> of privilege escalation and demotion functions that will make a system domain
> >> privileged and then remove that privilege.
> >>
> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> >> ---
> >>  xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >>
> >> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> >> index e22d6160b5..157e57151e 100644
> >> --- a/xen/include/xsm/xsm.h
> >> +++ b/xen/include/xsm/xsm.h
> >> @@ -189,6 +189,28 @@ struct xsm_operations {
> >>  #endif
> >>  };
> >>
> >> +static always_inline int xsm_elevate_priv(struct domain *d)
> >> +{
> >> +    if ( is_system_domain(d) )
> >> +    {
> >> +        d->is_privileged = true;
> >> +        return 0;
> >> +    }
> >> +
> >> +    return -EPERM;
> >> +}
> >
> > These look sufficient for the default policy, but they don't seem
> > sufficient for Flask.  I think you need to create a new XSM hook.  For
> > Flask, you would want the demote hook to transition xen_boot_t ->
> > xen_t.  That would start xen_boot_t with privileges that are dropped
> > in a one-way transition.  Does that require all policies to then have
> > xen_boot_t and xen_t?  I guess it does unless the hook code has some
> > logic to skip the transition.
>
> I am still thinking this through but my initial concern for Flask is
> that I don't think we want dedicated domain transitions directly in
> code. My current thinking would be to use a Kconfig to use xen_boot_t
> type as the initial sid for the idle domain which would then require the
> default policy to include an allowed transition from xen_boot_t to
> xen_t. Then rely upon a boot domain to issue an xsm_op to do a relabel
> transition for the idle domain with an assertion that the idle domain is
> no longer labeled with its initial sid before Xen transitions its state
> to SYS_STATE_active. The one wrinkle to this is whether I will be able
> to schedule the boot domain before allowing Xen to transition into
> SYS_STATE_active.

That is an interesting approach.  While it would work, I find it
unusual that a domain would relabel Xen.  I think Xen should be
responsible for itself and not rely on a domain for this operation.

> > For the default policy, you could start by creating the system domains
> > as privileged and just have a single hook to drop privs.  Then you
> > don't have to worry about the "elevate" hook existing.  The patch 2
> > asserts could instead become the location of xsm_drop_privs calls to
> > have a clear demarcation point.  That expands the window with
> > privileges though.  It's a little simpler, but maybe you don't want
> > that.  However, it seems like you can only depriv once for the Flask
> > case since you want it to be one-way.
>
> This does simplify the solution and since today we cannot differentiate
> between hypervisor setup and hypervisor initiated domain construction
> contexts, it does not run counter to what I have proposed. As for flask,
> again I do not believe codifying a domain transition bound to a new XSM
> op is the appropriate approach.

This hard coded domain transition does feel a little weird.  But it
seems like a natural consequence of trying to use Flask to
deprivilege.  I guess the transition could be behind a
dom0less/hyperlaunch Kconfig option.  I just don't see a way around it
in some fashion with Flask enforcing.

Another idea: Flask could start in permissive and only transition to
enforcing at the deprivilege point.  Kinda gross, but it works without
needing a transition.

To reiterate, XSM isn't really appropriate to enforce anything
internal to Xen.  We are working around the need to go through hook
points during correct operation.  Code exec in Xen means all bets are
off.  Memory writes to Xen data mean the XSM checks can be disabled
(flip Flask to permissive) or bypassed (set d->is_privileged or change
d->ssid).  We shouldn't lose sight of this when we talk about
deprivileging the idle domain.

Regards,
Jason

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Daniel P. Smith]

On 4/5/22 13:17, Jason Andryuk wrote:
> On Mon, Apr 4, 2022 at 11:34 AM Daniel P. Smith
> <dpsmith@apertussolutions.com> wrote:
>>
>> On 3/31/22 09:16, Jason Andryuk wrote:
>>> On Wed, Mar 30, 2022 at 3:05 PM Daniel P. Smith
>>> <dpsmith@apertussolutions.com> wrote:
>>>>
>>>> There are now instances where internal hypervisor logic needs to make resource
>>>> allocation calls that are protected by XSM checks. The internal hypervisor logic
>>>> is represented a number of system domains which by designed are represented by
>>>> non-privileged struct domain instances. To enable these logic blocks to
>>>> function correctly but in a controlled manner, this commit introduces a pair
>>>> of privilege escalation and demotion functions that will make a system domain
>>>> privileged and then remove that privilege.
>>>>
>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>>> ---
>>>>  xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
>>>>  1 file changed, 22 insertions(+)
>>>>
>>>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>>>> index e22d6160b5..157e57151e 100644
>>>> --- a/xen/include/xsm/xsm.h
>>>> +++ b/xen/include/xsm/xsm.h
>>>> @@ -189,6 +189,28 @@ struct xsm_operations {
>>>>  #endif
>>>>  };
>>>>
>>>> +static always_inline int xsm_elevate_priv(struct domain *d)
>>>> +{
>>>> +    if ( is_system_domain(d) )
>>>> +    {
>>>> +        d->is_privileged = true;
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    return -EPERM;
>>>> +}
>>>
>>> These look sufficient for the default policy, but they don't seem
>>> sufficient for Flask.  I think you need to create a new XSM hook.  For
>>> Flask, you would want the demote hook to transition xen_boot_t ->
>>> xen_t.  That would start xen_boot_t with privileges that are dropped
>>> in a one-way transition.  Does that require all policies to then have
>>> xen_boot_t and xen_t?  I guess it does unless the hook code has some
>>> logic to skip the transition.
>>
>> I am still thinking this through but my initial concern for Flask is
>> that I don't think we want dedicated domain transitions directly in
>> code. My current thinking would be to use a Kconfig to use xen_boot_t
>> type as the initial sid for the idle domain which would then require the
>> default policy to include an allowed transition from xen_boot_t to
>> xen_t. Then rely upon a boot domain to issue an xsm_op to do a relabel
>> transition for the idle domain with an assertion that the idle domain is
>> no longer labeled with its initial sid before Xen transitions its state
>> to SYS_STATE_active. The one wrinkle to this is whether I will be able
>> to schedule the boot domain before allowing Xen to transition into
>> SYS_STATE_active.
> 
> That is an interesting approach.  While it would work, I find it
> unusual that a domain would relabel Xen.  I think Xen should be
> responsible for itself and not rely on a domain for this operation.

The boot domain is not a general domain as no domain can/should be
created with its domid or flask label post transition to
SYS_STATE_active. Its purpose was specifically meant to be a natural way
to push out complicated pre-execution domain configuration from having
to be in they hypervisor code. Therefore in a way it can be considered a
user provided de-privileged part of the hypervisor.

With that said, I just realized a flaw in the basis of my position. What
is the difference between codifying a check that the idle domain is not
the boot label versus codifying a transition from the boot label to the
running label? None really, both will require some knowledge that there
is a boot label and some running label. Combine with the fact that the
idle domain really shouldn't have any other label than xen_t. I will
work out how to incorporate the domain transition.

>>> For the default policy, you could start by creating the system domains
>>> as privileged and just have a single hook to drop privs.  Then you
>>> don't have to worry about the "elevate" hook existing.  The patch 2
>>> asserts could instead become the location of xsm_drop_privs calls to
>>> have a clear demarcation point.  That expands the window with
>>> privileges though.  It's a little simpler, but maybe you don't want
>>> that.  However, it seems like you can only depriv once for the Flask
>>> case since you want it to be one-way.
>>
>> This does simplify the solution and since today we cannot differentiate
>> between hypervisor setup and hypervisor initiated domain construction
>> contexts, it does not run counter to what I have proposed. As for flask,
>> again I do not believe codifying a domain transition bound to a new XSM
>> op is the appropriate approach.
> 
> This hard coded domain transition does feel a little weird.  But it
> seems like a natural consequence of trying to use Flask to
> deprivilege.  I guess the transition could be behind a
> dom0less/hyperlaunch Kconfig option.  I just don't see a way around it
> in some fashion with Flask enforcing.
> 
> Another idea: Flask could start in permissive and only transition to
> enforcing at the deprivilege point.  Kinda gross, but it works without
> needing a transition.
> 
> To reiterate, XSM isn't really appropriate to enforce anything
> internal to Xen.  We are working around the need to go through hook
> points during correct operation.  Code exec in Xen means all bets are
> off.  Memory writes to Xen data mean the XSM checks can be disabled
> (flip Flask to permissive) or bypassed (set d->is_privileged or change
> d->ssid).  We shouldn't lose sight of this when we talk about
> deprivileging the idle domain.

I would far prefer a transition than trying to reason about whether
flask should be in permissive or enforcing based on this state change in
combination of command line setting.

v/r
dps

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Jan Beulich]

On 05.04.2022 19:17, Jason Andryuk wrote:
> On Mon, Apr 4, 2022 at 11:34 AM Daniel P. Smith <dpsmith@apertussolutions.com> wrote:
>> On 3/31/22 09:16, Jason Andryuk wrote:
>>> For the default policy, you could start by creating the system domains
>>> as privileged and just have a single hook to drop privs.  Then you
>>> don't have to worry about the "elevate" hook existing.  The patch 2
>>> asserts could instead become the location of xsm_drop_privs calls to
>>> have a clear demarcation point.  That expands the window with
>>> privileges though.  It's a little simpler, but maybe you don't want
>>> that.  However, it seems like you can only depriv once for the Flask
>>> case since you want it to be one-way.
>>
>> This does simplify the solution and since today we cannot differentiate
>> between hypervisor setup and hypervisor initiated domain construction
>> contexts, it does not run counter to what I have proposed. As for flask,
>> again I do not believe codifying a domain transition bound to a new XSM
>> op is the appropriate approach.
> 
> This hard coded domain transition does feel a little weird.  But it
> seems like a natural consequence of trying to use Flask to
> deprivilege.  I guess the transition could be behind a
> dom0less/hyperlaunch Kconfig option.  I just don't see a way around it
> in some fashion with Flask enforcing.
> 
> Another idea: Flask could start in permissive and only transition to
> enforcing at the deprivilege point.  Kinda gross, but it works without
> needing a transition.

I don't think that would be right. Logically such behavior ought to be
mirrored to SILO, and I'll take that for the example for being the
simpler model: Suppose an admin wants to disallow communication
between DomU-s created by Xen. Such would want enforcing when creating
those DomU-s, despite the creator (Xen) being all powerful. If the
device tree information said something different (e.g. directing for
an event channel to be established between two such DomU-s), this
should be flagged as an error, not be silently permitted.

Jan

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Roger Pau Monné]

On Wed, Apr 06, 2022 at 09:06:59AM +0200, Jan Beulich wrote:
> On 05.04.2022 19:17, Jason Andryuk wrote:
> > On Mon, Apr 4, 2022 at 11:34 AM Daniel P. Smith <dpsmith@apertussolutions.com> wrote:
> >> On 3/31/22 09:16, Jason Andryuk wrote:
> >>> For the default policy, you could start by creating the system domains
> >>> as privileged and just have a single hook to drop privs.  Then you
> >>> don't have to worry about the "elevate" hook existing.  The patch 2
> >>> asserts could instead become the location of xsm_drop_privs calls to
> >>> have a clear demarcation point.  That expands the window with
> >>> privileges though.  It's a little simpler, but maybe you don't want
> >>> that.  However, it seems like you can only depriv once for the Flask
> >>> case since you want it to be one-way.
> >>
> >> This does simplify the solution and since today we cannot differentiate
> >> between hypervisor setup and hypervisor initiated domain construction
> >> contexts, it does not run counter to what I have proposed. As for flask,
> >> again I do not believe codifying a domain transition bound to a new XSM
> >> op is the appropriate approach.
> > 
> > This hard coded domain transition does feel a little weird.  But it
> > seems like a natural consequence of trying to use Flask to
> > deprivilege.  I guess the transition could be behind a
> > dom0less/hyperlaunch Kconfig option.  I just don't see a way around it
> > in some fashion with Flask enforcing.
> > 
> > Another idea: Flask could start in permissive and only transition to
> > enforcing at the deprivilege point.  Kinda gross, but it works without
> > needing a transition.
> 
> I don't think that would be right. Logically such behavior ought to be
> mirrored to SILO, and I'll take that for the example for being the
> simpler model: Suppose an admin wants to disallow communication
> between DomU-s created by Xen. Such would want enforcing when creating
> those DomU-s, despite the creator (Xen) being all powerful. If the
> device tree information said something different (e.g. directing for
> an event channel to be established between two such DomU-s), this
> should be flagged as an error, not be silently permitted.

I could also see this argument the other way around: what if an admin
wants to disallow domUs freely communicating between them, but would
still like some controlled domU communication to be possible by
setting up those channels at domain creation?

Thanks, Roger.

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Jan Beulich]

On 06.04.2022 10:46, Roger Pau Monné wrote:
> On Wed, Apr 06, 2022 at 09:06:59AM +0200, Jan Beulich wrote:
>> On 05.04.2022 19:17, Jason Andryuk wrote:
>>> On Mon, Apr 4, 2022 at 11:34 AM Daniel P. Smith <dpsmith@apertussolutions.com> wrote:
>>>> On 3/31/22 09:16, Jason Andryuk wrote:
>>>>> For the default policy, you could start by creating the system domains
>>>>> as privileged and just have a single hook to drop privs.  Then you
>>>>> don't have to worry about the "elevate" hook existing.  The patch 2
>>>>> asserts could instead become the location of xsm_drop_privs calls to
>>>>> have a clear demarcation point.  That expands the window with
>>>>> privileges though.  It's a little simpler, but maybe you don't want
>>>>> that.  However, it seems like you can only depriv once for the Flask
>>>>> case since you want it to be one-way.
>>>>
>>>> This does simplify the solution and since today we cannot differentiate
>>>> between hypervisor setup and hypervisor initiated domain construction
>>>> contexts, it does not run counter to what I have proposed. As for flask,
>>>> again I do not believe codifying a domain transition bound to a new XSM
>>>> op is the appropriate approach.
>>>
>>> This hard coded domain transition does feel a little weird.  But it
>>> seems like a natural consequence of trying to use Flask to
>>> deprivilege.  I guess the transition could be behind a
>>> dom0less/hyperlaunch Kconfig option.  I just don't see a way around it
>>> in some fashion with Flask enforcing.
>>>
>>> Another idea: Flask could start in permissive and only transition to
>>> enforcing at the deprivilege point.  Kinda gross, but it works without
>>> needing a transition.
>>
>> I don't think that would be right. Logically such behavior ought to be
>> mirrored to SILO, and I'll take that for the example for being the
>> simpler model: Suppose an admin wants to disallow communication
>> between DomU-s created by Xen. Such would want enforcing when creating
>> those DomU-s, despite the creator (Xen) being all powerful. If the
>> device tree information said something different (e.g. directing for
>> an event channel to be established between two such DomU-s), this
>> should be flagged as an error, not be silently permitted.
> 
> I could also see this argument the other way around: what if an admin
> wants to disallow domUs freely communicating between them, but would
> still like some controlled domU communication to be possible by
> setting up those channels at domain creation?

Well, imo that would require a proper (Flask) policy then, not SILO mode.

Jan

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Roger Pau Monné]

On Wed, Apr 06, 2022 at 10:48:23AM +0200, Jan Beulich wrote:
> On 06.04.2022 10:46, Roger Pau Monné wrote:
> > On Wed, Apr 06, 2022 at 09:06:59AM +0200, Jan Beulich wrote:
> >> On 05.04.2022 19:17, Jason Andryuk wrote:
> >>> On Mon, Apr 4, 2022 at 11:34 AM Daniel P. Smith <dpsmith@apertussolutions.com> wrote:
> >>>> On 3/31/22 09:16, Jason Andryuk wrote:
> >>>>> For the default policy, you could start by creating the system domains
> >>>>> as privileged and just have a single hook to drop privs.  Then you
> >>>>> don't have to worry about the "elevate" hook existing.  The patch 2
> >>>>> asserts could instead become the location of xsm_drop_privs calls to
> >>>>> have a clear demarcation point.  That expands the window with
> >>>>> privileges though.  It's a little simpler, but maybe you don't want
> >>>>> that.  However, it seems like you can only depriv once for the Flask
> >>>>> case since you want it to be one-way.
> >>>>
> >>>> This does simplify the solution and since today we cannot differentiate
> >>>> between hypervisor setup and hypervisor initiated domain construction
> >>>> contexts, it does not run counter to what I have proposed. As for flask,
> >>>> again I do not believe codifying a domain transition bound to a new XSM
> >>>> op is the appropriate approach.
> >>>
> >>> This hard coded domain transition does feel a little weird.  But it
> >>> seems like a natural consequence of trying to use Flask to
> >>> deprivilege.  I guess the transition could be behind a
> >>> dom0less/hyperlaunch Kconfig option.  I just don't see a way around it
> >>> in some fashion with Flask enforcing.
> >>>
> >>> Another idea: Flask could start in permissive and only transition to
> >>> enforcing at the deprivilege point.  Kinda gross, but it works without
> >>> needing a transition.
> >>
> >> I don't think that would be right. Logically such behavior ought to be
> >> mirrored to SILO, and I'll take that for the example for being the
> >> simpler model: Suppose an admin wants to disallow communication
> >> between DomU-s created by Xen. Such would want enforcing when creating
> >> those DomU-s, despite the creator (Xen) being all powerful. If the
> >> device tree information said something different (e.g. directing for
> >> an event channel to be established between two such DomU-s), this
> >> should be flagged as an error, not be silently permitted.
> > 
> > I could also see this argument the other way around: what if an admin
> > wants to disallow domUs freely communicating between them, but would
> > still like some controlled domU communication to be possible by
> > setting up those channels at domain creation?
> 
> Well, imo that would require a proper (Flask) policy then, not SILO mode.

But when creating such domains in SILO mode from dom0, dom0 would be
allowed to create and bind event channels to the created domains, even
if the domain themselves are not allowed the operation.

That's because the check in evtchn_bind_interdomain() is done against
'current' and not the domain where the event channel will be bound.
Maybe such check should instead take 3 parameters: current context
domain, domain to bind the event channel to and remote domain on the
other end of the event channel.

Thanks, Roger.

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Jan Beulich]

On 06.04.2022 11:09, Roger Pau Monné wrote:
> On Wed, Apr 06, 2022 at 10:48:23AM +0200, Jan Beulich wrote:
>> On 06.04.2022 10:46, Roger Pau Monné wrote:
>>> On Wed, Apr 06, 2022 at 09:06:59AM +0200, Jan Beulich wrote:
>>>> On 05.04.2022 19:17, Jason Andryuk wrote:
>>>>> On Mon, Apr 4, 2022 at 11:34 AM Daniel P. Smith <dpsmith@apertussolutions.com> wrote:
>>>>>> On 3/31/22 09:16, Jason Andryuk wrote:
>>>>>>> For the default policy, you could start by creating the system domains
>>>>>>> as privileged and just have a single hook to drop privs.  Then you
>>>>>>> don't have to worry about the "elevate" hook existing.  The patch 2
>>>>>>> asserts could instead become the location of xsm_drop_privs calls to
>>>>>>> have a clear demarcation point.  That expands the window with
>>>>>>> privileges though.  It's a little simpler, but maybe you don't want
>>>>>>> that.  However, it seems like you can only depriv once for the Flask
>>>>>>> case since you want it to be one-way.
>>>>>>
>>>>>> This does simplify the solution and since today we cannot differentiate
>>>>>> between hypervisor setup and hypervisor initiated domain construction
>>>>>> contexts, it does not run counter to what I have proposed. As for flask,
>>>>>> again I do not believe codifying a domain transition bound to a new XSM
>>>>>> op is the appropriate approach.
>>>>>
>>>>> This hard coded domain transition does feel a little weird.  But it
>>>>> seems like a natural consequence of trying to use Flask to
>>>>> deprivilege.  I guess the transition could be behind a
>>>>> dom0less/hyperlaunch Kconfig option.  I just don't see a way around it
>>>>> in some fashion with Flask enforcing.
>>>>>
>>>>> Another idea: Flask could start in permissive and only transition to
>>>>> enforcing at the deprivilege point.  Kinda gross, but it works without
>>>>> needing a transition.
>>>>
>>>> I don't think that would be right. Logically such behavior ought to be
>>>> mirrored to SILO, and I'll take that for the example for being the
>>>> simpler model: Suppose an admin wants to disallow communication
>>>> between DomU-s created by Xen. Such would want enforcing when creating
>>>> those DomU-s, despite the creator (Xen) being all powerful. If the
>>>> device tree information said something different (e.g. directing for
>>>> an event channel to be established between two such DomU-s), this
>>>> should be flagged as an error, not be silently permitted.
>>>
>>> I could also see this argument the other way around: what if an admin
>>> wants to disallow domUs freely communicating between them, but would
>>> still like some controlled domU communication to be possible by
>>> setting up those channels at domain creation?
>>
>> Well, imo that would require a proper (Flask) policy then, not SILO mode.
> 
> But when creating such domains in SILO mode from dom0, dom0 would be
> allowed to create and bind event channels to the created domains, even
> if the domain themselves are not allowed the operation.
> 
> That's because the check in evtchn_bind_interdomain() is done against
> 'current' and not the domain where the event channel will be bound.

Yes and no - the check is against current, but that's because
communication is established between current ( == ld) and rd. The
function in its present shape simply can't establish a channel
between two arbitrary domains.

Jan

> Maybe such check should instead take 3 parameters: current context
> domain, domain to bind the event channel to and remote domain on the
> other end of the event channel.
> 
> Thanks, Roger.
> 

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Roger Pau Monné]

On Wed, Apr 06, 2022 at 11:16:10AM +0200, Jan Beulich wrote:
> On 06.04.2022 11:09, Roger Pau Monné wrote:
> > On Wed, Apr 06, 2022 at 10:48:23AM +0200, Jan Beulich wrote:
> >> On 06.04.2022 10:46, Roger Pau Monné wrote:
> >>> On Wed, Apr 06, 2022 at 09:06:59AM +0200, Jan Beulich wrote:
> >>>> On 05.04.2022 19:17, Jason Andryuk wrote:
> >>>>> On Mon, Apr 4, 2022 at 11:34 AM Daniel P. Smith <dpsmith@apertussolutions.com> wrote:
> >>>>>> On 3/31/22 09:16, Jason Andryuk wrote:
> >>>>>>> For the default policy, you could start by creating the system domains
> >>>>>>> as privileged and just have a single hook to drop privs.  Then you
> >>>>>>> don't have to worry about the "elevate" hook existing.  The patch 2
> >>>>>>> asserts could instead become the location of xsm_drop_privs calls to
> >>>>>>> have a clear demarcation point.  That expands the window with
> >>>>>>> privileges though.  It's a little simpler, but maybe you don't want
> >>>>>>> that.  However, it seems like you can only depriv once for the Flask
> >>>>>>> case since you want it to be one-way.
> >>>>>>
> >>>>>> This does simplify the solution and since today we cannot differentiate
> >>>>>> between hypervisor setup and hypervisor initiated domain construction
> >>>>>> contexts, it does not run counter to what I have proposed. As for flask,
> >>>>>> again I do not believe codifying a domain transition bound to a new XSM
> >>>>>> op is the appropriate approach.
> >>>>>
> >>>>> This hard coded domain transition does feel a little weird.  But it
> >>>>> seems like a natural consequence of trying to use Flask to
> >>>>> deprivilege.  I guess the transition could be behind a
> >>>>> dom0less/hyperlaunch Kconfig option.  I just don't see a way around it
> >>>>> in some fashion with Flask enforcing.
> >>>>>
> >>>>> Another idea: Flask could start in permissive and only transition to
> >>>>> enforcing at the deprivilege point.  Kinda gross, but it works without
> >>>>> needing a transition.
> >>>>
> >>>> I don't think that would be right. Logically such behavior ought to be
> >>>> mirrored to SILO, and I'll take that for the example for being the
> >>>> simpler model: Suppose an admin wants to disallow communication
> >>>> between DomU-s created by Xen. Such would want enforcing when creating
> >>>> those DomU-s, despite the creator (Xen) being all powerful. If the
> >>>> device tree information said something different (e.g. directing for
> >>>> an event channel to be established between two such DomU-s), this
> >>>> should be flagged as an error, not be silently permitted.
> >>>
> >>> I could also see this argument the other way around: what if an admin
> >>> wants to disallow domUs freely communicating between them, but would
> >>> still like some controlled domU communication to be possible by
> >>> setting up those channels at domain creation?
> >>
> >> Well, imo that would require a proper (Flask) policy then, not SILO mode.
> > 
> > But when creating such domains in SILO mode from dom0, dom0 would be
> > allowed to create and bind event channels to the created domains, even
> > if the domain themselves are not allowed the operation.
> > 
> > That's because the check in evtchn_bind_interdomain() is done against
> > 'current' and not the domain where the event channel will be bound.
> 
> Yes and no - the check is against current, but that's because
> communication is established between current ( == ld) and rd. The
> function in its present shape simply can't establish a channel
> between two arbitrary domains.

I've got confused with evtchn_alloc_unbound() that does take a local
and remote domains as parameters, but in that case the xsm check is
done against ld (which might not be current) and rd.

Thanks, Roger.

Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Jason Andryuk]

On Wed, Apr 6, 2022 at 3:07 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 05.04.2022 19:17, Jason Andryuk wrote:
> > On Mon, Apr 4, 2022 at 11:34 AM Daniel P. Smith <dpsmith@apertussolutions.com> wrote:
> >> On 3/31/22 09:16, Jason Andryuk wrote:
> >>> For the default policy, you could start by creating the system domains
> >>> as privileged and just have a single hook to drop privs.  Then you
> >>> don't have to worry about the "elevate" hook existing.  The patch 2
> >>> asserts could instead become the location of xsm_drop_privs calls to
> >>> have a clear demarcation point.  That expands the window with
> >>> privileges though.  It's a little simpler, but maybe you don't want
> >>> that.  However, it seems like you can only depriv once for the Flask
> >>> case since you want it to be one-way.
> >>
> >> This does simplify the solution and since today we cannot differentiate
> >> between hypervisor setup and hypervisor initiated domain construction
> >> contexts, it does not run counter to what I have proposed. As for flask,
> >> again I do not believe codifying a domain transition bound to a new XSM
> >> op is the appropriate approach.
> >
> > This hard coded domain transition does feel a little weird.  But it
> > seems like a natural consequence of trying to use Flask to
> > deprivilege.  I guess the transition could be behind a
> > dom0less/hyperlaunch Kconfig option.  I just don't see a way around it
> > in some fashion with Flask enforcing.
> >
> > Another idea: Flask could start in permissive and only transition to
> > enforcing at the deprivilege point.  Kinda gross, but it works without
> > needing a transition.
>
> I don't think that would be right. Logically such behavior ought to be
> mirrored to SILO, and I'll take that for the example for being the
> simpler model: Suppose an admin wants to disallow communication
> between DomU-s created by Xen. Such would want enforcing when creating
> those DomU-s, despite the creator (Xen) being all powerful. If the
> device tree information said something different (e.g. directing for
> an event channel to be established between two such DomU-s), this
> should be flagged as an error, not be silently permitted.

Yes, you are right.  As you say, you want the policy enforced when
creating the DomU-s.

Regards,
Jason