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 <dpsm...@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.
From a525c5a04a8483ee9217b0be6deb12d665e3fd72 Mon Sep 17 00:00:00 2001
From: "Daniel P. Smith" <dpsm...@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 <dpsm...@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
From 568a94a0b3fe8de8f69fda4a24d8856272172dfb Mon Sep 17 00:00:00 2001
From: "Daniel P. Smith" <dpsm...@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 <dpsm...@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
From 887a83d88d5dc6331c9296b1f513e3bda1ae75e8 Mon Sep 17 00:00:00 2001
From: Stefano Stabellini <stefano.stabell...@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.stabell...@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
