[PATCH v9 0/3] Adds starting the idle domain privileged

Daniel P. Smith posted 3 patches 1 year, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220630022110.31555-1-dpsmith@apertussolutions.com
Test gitlab-ci failed
There is a newer version of this series
tools/flask/policy/modules/dom0.te     |  3 ++
tools/flask/policy/modules/domU.te     |  3 ++
tools/flask/policy/modules/xen.if      |  7 +++
tools/flask/policy/modules/xen.te      |  1 +
tools/flask/policy/policy/initial_sids |  1 +
xen/arch/arm/setup.c                   |  3 ++
xen/arch/x86/setup.c                   |  4 ++
xen/common/sched/core.c                |  7 ++-
xen/include/xsm/dummy.h                | 17 +++++++
xen/include/xsm/xsm.h                  |  6 +++
xen/xsm/dummy.c                        |  1 +
xen/xsm/flask/hooks.c                  | 66 ++++++++++++++++++++------
xen/xsm/flask/policy/initial_sids      |  1 +
13 files changed, 104 insertions(+), 16 deletions(-)
[PATCH v9 0/3] Adds starting the idle domain privileged
Posted by Daniel P. Smith 1 year, 9 months ago
This series makes it so that the idle domain is started privileged under the
default policy, which the SILO policy inherits, and under the flask policy. It
then introduces a new one-way XSM hook, xsm_transition_running, that is hooked
by an XSM policy to transition the idle domain to its running privilege level.

Patch 3 is an important one, as first it addresses the issue raised under an
RFC late last year by Jason Andryuk regarding the awkward entanglement of
flask_domain_alloc_security() and flask_domain_create(). Second, it helps
articulate why it is that the hypervisor should go through the access control
checks, even when it is doing the action itself. The issue at hand is not that
the hypervisor could be influenced to go around these check. The issue is these
checks provides a configurable way to express the execution flow that the
hypervisor should enforce. Specifically with this change, it is now possible
for an owner of a dom0less or hyperlaunch system to express a policy where the
hypervisor will enforce that no dom0 will be constructed, regardless of what
boot construction details were provided to it. Likewise, an owner that does not
want to see dom0less or hyperlaunch to be used can enforce that the hypervisor
will only construct a dom0 domain. This can all be accomplished without the
need to rebuild the hypervisor with these features enabled or disabled.

Changes in v9:
- added missing Rb/Tb to patch 1
- corrected the flask policy macro in patch 2 to allow domain create
- added patch 3 to address allowing the hypervisor create more than 1 domain

Changes in v8:
- adjusted panic messages in arm and x86 setup.c to be less than 80cols
- fixed comment line that went over 80col
- added line in patch #1 commit message to clarify the need is for domain
  creation

Changes in v7:
- adjusted error message in default and flask xsm_set_system_active hooks
- merged panic messages in arm and x86 setup.c to a single line

Changes in v6:
- readded the setting of is_privileged in flask_set_system_active()
- clarified comment on is_privileged in flask_set_system_active()
- added ASSERT on is_privileged and self_sid in flask_set_system_active()
- fixed err code returned on Arm for xsm_set_system_active() panic message

Changes in v5:
- dropped setting is_privileged in flask_set_system_active()
- added err code returned by xsm_set_system_active() to panic message

Changes in v4:
- reworded patch 1 commit messaged
- fixed whitespace to coding style
- fixed comment to coding style

Changes in v3:
- renamed *_transition_running() to *_set_system_active()
- changed the XSM hook set_system_active() from void to int return
- added ASSERT check for the expected privilege level each XSM policy expected
- replaced a check against is_privileged in each arch with checking the return
  value from the call to xsm_set_system_active()

Changes in v2:
- renamed flask_domain_runtime_security() to flask_transition_running()
- added the missed assignment of self_sid

Daniel P. Smith (3):
  xsm: create idle domain privileged and demote after setup
  flask: implement xsm_set_system_active
  xsm: refactor flask sid alloc and domain check

 tools/flask/policy/modules/dom0.te     |  3 ++
 tools/flask/policy/modules/domU.te     |  3 ++
 tools/flask/policy/modules/xen.if      |  7 +++
 tools/flask/policy/modules/xen.te      |  1 +
 tools/flask/policy/policy/initial_sids |  1 +
 xen/arch/arm/setup.c                   |  3 ++
 xen/arch/x86/setup.c                   |  4 ++
 xen/common/sched/core.c                |  7 ++-
 xen/include/xsm/dummy.h                | 17 +++++++
 xen/include/xsm/xsm.h                  |  6 +++
 xen/xsm/dummy.c                        |  1 +
 xen/xsm/flask/hooks.c                  | 66 ++++++++++++++++++++------
 xen/xsm/flask/policy/initial_sids      |  1 +
 13 files changed, 104 insertions(+), 16 deletions(-)

-- 
2.20.1
Re: [PATCH v9 0/3] Adds starting the idle domain privileged
Posted by Stefano Stabellini 1 year, 9 months ago
On Wed, 29 Jun 2022, Daniel P. Smith wrote:
> This series makes it so that the idle domain is started privileged under the
> default policy, which the SILO policy inherits, and under the flask policy. It
> then introduces a new one-way XSM hook, xsm_transition_running, that is hooked
> by an XSM policy to transition the idle domain to its running privilege level.
> 
> Patch 3 is an important one, as first it addresses the issue raised under an
> RFC late last year by Jason Andryuk regarding the awkward entanglement of
> flask_domain_alloc_security() and flask_domain_create(). Second, it helps
> articulate why it is that the hypervisor should go through the access control
> checks, even when it is doing the action itself. The issue at hand is not that
> the hypervisor could be influenced to go around these check. The issue is these
> checks provides a configurable way to express the execution flow that the
> hypervisor should enforce. Specifically with this change, it is now possible
> for an owner of a dom0less or hyperlaunch system to express a policy where the
> hypervisor will enforce that no dom0 will be constructed, regardless of what
> boot construction details were provided to it. Likewise, an owner that does not
> want to see dom0less or hyperlaunch to be used can enforce that the hypervisor
> will only construct a dom0 domain. This can all be accomplished without the
> need to rebuild the hypervisor with these features enabled or disabled.


It looks like this patch series is fully acked except:
- in theory we need an ack from Daniel for flask
- there is a very small change to sched that would need an ack from
  George/Dario


I think we should commit the series in a couple of days unless someone
spots an issue with it. Let me know if you have any concerns with this.

The minimal request to improve the in-code comment by Jan could be done
on commit.

Note that committing this series would also have the benefit of
unblocking the ARM gitlab-ci pipeline.
Re: [PATCH v9 0/3] Adds starting the idle domain privileged
Posted by Jan Beulich 1 year, 9 months ago
On 01.07.2022 00:35, Stefano Stabellini wrote:
> On Wed, 29 Jun 2022, Daniel P. Smith wrote:
>> This series makes it so that the idle domain is started privileged under the
>> default policy, which the SILO policy inherits, and under the flask policy. It
>> then introduces a new one-way XSM hook, xsm_transition_running, that is hooked
>> by an XSM policy to transition the idle domain to its running privilege level.
>>
>> Patch 3 is an important one, as first it addresses the issue raised under an
>> RFC late last year by Jason Andryuk regarding the awkward entanglement of
>> flask_domain_alloc_security() and flask_domain_create(). Second, it helps
>> articulate why it is that the hypervisor should go through the access control
>> checks, even when it is doing the action itself. The issue at hand is not that
>> the hypervisor could be influenced to go around these check. The issue is these
>> checks provides a configurable way to express the execution flow that the
>> hypervisor should enforce. Specifically with this change, it is now possible
>> for an owner of a dom0less or hyperlaunch system to express a policy where the
>> hypervisor will enforce that no dom0 will be constructed, regardless of what
>> boot construction details were provided to it. Likewise, an owner that does not
>> want to see dom0less or hyperlaunch to be used can enforce that the hypervisor
>> will only construct a dom0 domain. This can all be accomplished without the
>> need to rebuild the hypervisor with these features enabled or disabled.
> 
> 
> It looks like this patch series is fully acked except:
> - in theory we need an ack from Daniel for flask
> - there is a very small change to sched that would need an ack from
>   George/Dario

I don't think I've seen any R-b for the last patch.

Jan
Re: [PATCH v9 0/3] Adds starting the idle domain privileged
Posted by Stefano Stabellini 1 year, 9 months ago
On Fri, 1 Jul 2022, Jan Beulich wrote:
> On 01.07.2022 00:35, Stefano Stabellini wrote:
> > On Wed, 29 Jun 2022, Daniel P. Smith wrote:
> >> This series makes it so that the idle domain is started privileged under the
> >> default policy, which the SILO policy inherits, and under the flask policy. It
> >> then introduces a new one-way XSM hook, xsm_transition_running, that is hooked
> >> by an XSM policy to transition the idle domain to its running privilege level.
> >>
> >> Patch 3 is an important one, as first it addresses the issue raised under an
> >> RFC late last year by Jason Andryuk regarding the awkward entanglement of
> >> flask_domain_alloc_security() and flask_domain_create(). Second, it helps
> >> articulate why it is that the hypervisor should go through the access control
> >> checks, even when it is doing the action itself. The issue at hand is not that
> >> the hypervisor could be influenced to go around these check. The issue is these
> >> checks provides a configurable way to express the execution flow that the
> >> hypervisor should enforce. Specifically with this change, it is now possible
> >> for an owner of a dom0less or hyperlaunch system to express a policy where the
> >> hypervisor will enforce that no dom0 will be constructed, regardless of what
> >> boot construction details were provided to it. Likewise, an owner that does not
> >> want to see dom0less or hyperlaunch to be used can enforce that the hypervisor
> >> will only construct a dom0 domain. This can all be accomplished without the
> >> need to rebuild the hypervisor with these features enabled or disabled.
> > 
> > 
> > It looks like this patch series is fully acked except:
> > - in theory we need an ack from Daniel for flask
> > - there is a very small change to sched that would need an ack from
> >   George/Dario
> 
> I don't think I've seen any R-b for the last patch.

Good point. I hope you'll be happy to give your Reviewed-by or Acked-by
to the next version with the minor comments fixed.
Re: [PATCH v9 0/3] Adds starting the idle domain privileged
Posted by Daniel P. Smith 1 year, 9 months ago
On 6/30/22 18:35, Stefano Stabellini wrote:
> On Wed, 29 Jun 2022, Daniel P. Smith wrote:
>> This series makes it so that the idle domain is started privileged under the
>> default policy, which the SILO policy inherits, and under the flask policy. It
>> then introduces a new one-way XSM hook, xsm_transition_running, that is hooked
>> by an XSM policy to transition the idle domain to its running privilege level.
>>
>> Patch 3 is an important one, as first it addresses the issue raised under an
>> RFC late last year by Jason Andryuk regarding the awkward entanglement of
>> flask_domain_alloc_security() and flask_domain_create(). Second, it helps
>> articulate why it is that the hypervisor should go through the access control
>> checks, even when it is doing the action itself. The issue at hand is not that
>> the hypervisor could be influenced to go around these check. The issue is these
>> checks provides a configurable way to express the execution flow that the
>> hypervisor should enforce. Specifically with this change, it is now possible
>> for an owner of a dom0less or hyperlaunch system to express a policy where the
>> hypervisor will enforce that no dom0 will be constructed, regardless of what
>> boot construction details were provided to it. Likewise, an owner that does not
>> want to see dom0less or hyperlaunch to be used can enforce that the hypervisor
>> will only construct a dom0 domain. This can all be accomplished without the
>> need to rebuild the hypervisor with these features enabled or disabled.
> 
> 
> It looks like this patch series is fully acked except:
> - in theory we need an ack from Daniel for flask
> - there is a very small change to sched that would need an ack from
>   George/Dario
> 
> 
> I think we should commit the series in a couple of days unless someone
> spots an issue with it. Let me know if you have any concerns with this.
> 
> The minimal request to improve the in-code comment by Jan could be done
> on commit.

I already have those changes ready to go. I was holding off with the
hope that Jason might find some time to read the last patch. I can try
pinging him directly tomorrow to see if he is even in town, as this is a
big holiday weekend here in the states.

> Note that committing this series would also have the benefit of
> unblocking the ARM gitlab-ci pipeline.

v/r,
dps