All of lore.kernel.org
 help / color / mirror / Atom feed
* XSM and the idle domain
@ 2020-10-21 14:34 Hongyan Xia
  2020-10-21 16:21 ` Jason Andryuk
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Hongyan Xia @ 2020-10-21 14:34 UTC (permalink / raw)
  To: xen-devel, jbeulich, andrew.cooper3, jandryuk, dgdegra

Hi,

A while ago there was a quick chat on IRC about how XSM interacts with
the idle domain. The conversation did not reach any clear conclusions
so it might be a good idea to summarise the questions in an email.

Basically there were two questions in that conversation:

1. In its current state, are security modules able to limit what the
idle domain can do?
2. Should security modules be able to restrict the idle domain?

The first question came up during ongoing work in LiveUpdate. After an
LU, the next Xen needs to restore all domains. To do that, some
hypercalls need to be issued from the idle domain context and
apparently XSM does not like it. We need to introduce hacks in the
dummy module to leave the idle domain alone. Our work is not compiled
with CONFIG_XSM at all, but with CONFIG_XSM, are we able to enforce
security policies against the idle domain? Of course, without any LU
work this does not make any difference because the idle domain does not
do any useful work to be restricted anyway.

Also, should idle domain be restricted? IMO the idle domain is Xen
itself which mostly bootstraps the system and performs limited work
when switched to, and is not something a user (either dom0 or domU)
directly interacts with. I doubt XSM was designed to include the idle
domain (although there is an ID allocated for it in the code), so I
would say just exclude idle in all security policy checks.

I may have missed some points in that discussion, so please feel free
to add.

Hongyan



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: XSM and the idle domain
  2020-10-21 14:34 XSM and the idle domain Hongyan Xia
@ 2020-10-21 16:21 ` Jason Andryuk
  2020-10-22 11:29   ` Hongyan Xia
  2020-10-22  1:23 ` Daniel P. Smith
  2020-10-22 12:51 ` Andrew Cooper
  2 siblings, 1 reply; 13+ messages in thread
From: Jason Andryuk @ 2020-10-21 16:21 UTC (permalink / raw)
  To: Hongyan Xia; +Cc: xen-devel, Jan Beulich, Andrew Cooper, Daniel De Graaf

On Wed, Oct 21, 2020 at 10:35 AM Hongyan Xia <hx242@xen.org> wrote:
>
> Hi,

Hi, Hongyan.

I'm familiar with Flask but not particularly familiar with other XSMs
or CONFIG_XSM=n.

> A while ago there was a quick chat on IRC about how XSM interacts with
> the idle domain. The conversation did not reach any clear conclusions
> so it might be a good idea to summarise the questions in an email.
>
> Basically there were two questions in that conversation:
>
> 1. In its current state, are security modules able to limit what the
> idle domain can do?
> 2. Should security modules be able to restrict the idle domain?
>
> The first question came up during ongoing work in LiveUpdate. After an
> LU, the next Xen needs to restore all domains. To do that, some
> hypercalls need to be issued from the idle domain context and
> apparently XSM does not like it. We need to introduce hacks in the
> dummy module to leave the idle domain alone.

Is this modifying xsm_default_action() to add an is_idle_domain()
check which always succeeds?

>Our work is not compiled
> with CONFIG_XSM at all, but with CONFIG_XSM, are we able to enforce
> security policies against the idle domain?

It's not clear to me if you want to use CONFIG_XSM, or just don't want
to break it.

> Of course, without any LU
> work this does not make any difference because the idle domain does not
> do any useful work to be restricted anyway.

I think this last sentence is the main point.  It's always been
labeled xen_t, but since it doesn't go through any of the hook points,
it hasn't needed any restrictions.  Actually, reviewing the Flask
policy there is:
# Domain destruction can result in some access checks for actions performed by
# the hypervisor.  These should always be allowed.
allow xen_t resource_type : resource { remove_irq remove_ioport remove_iomem };

> Also, should idle domain be restricted? IMO the idle domain is Xen
> itself which mostly bootstraps the system and performs limited work
> when switched to, and is not something a user (either dom0 or domU)
> directly interacts with. I doubt XSM was designed to include the idle
> domain (although there is an ID allocated for it in the code), so I
> would say just exclude idle in all security policy checks.

I think it makes sense to label xen_t, even if it doesn't do anything.
As you say, it is a distinct entity from dom0 and domU.  Yes, it can
circumvent the policy, but it's not actively hurting anything.  And it
can be good to catch when it does start doing something, as you found.

Might it make sense to create a LU domain instead of using the idle
domain for Live Update?  Another approach could be to run the
idle_domain as "dom0" during Live Update, and then transition to the
regular idle_domain when it completes?  You are re-creating dom0, but
you could flip is_privileged on during live update and then remove it
once complete.

Regards,
Jason


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: XSM and the idle domain
  2020-10-21 14:34 XSM and the idle domain Hongyan Xia
  2020-10-21 16:21 ` Jason Andryuk
@ 2020-10-22  1:23 ` Daniel P. Smith
  2020-10-22  8:13   ` Jan Beulich
  2020-10-22 12:51 ` Andrew Cooper
  2 siblings, 1 reply; 13+ messages in thread
From: Daniel P. Smith @ 2020-10-22  1:23 UTC (permalink / raw)
  To: Hongyan Xia, xen-devel, jbeulich, andrew.cooper3, jandryuk, dgdegra

On 10/21/20 10:34 AM, Hongyan Xia wrote:
> Hi,
> 
> A while ago there was a quick chat on IRC about how XSM interacts with
> the idle domain. The conversation did not reach any clear conclusions
> so it might be a good idea to summarise the questions in an email.
> 
> Basically there were two questions in that conversation:
> 
> 1. In its current state, are security modules able to limit what the
> idle domain can do?

Yes in the fact that the idle domain has a type and you can constrain 
what actions the type is allowed. Now in reality the idle domain is 
given the same type as the hypervisor itself thus must have the ability 
to make certain actions.

> 2. Should security modules be able to restrict the idle domain?

IMHO I think this question should be reversed to ask whether the actions 
the idle domain is being used for is appropriate from a security point 
of view. AIUI the idle domain is a mechanism for the scheduler to use as 
a place to schedule an idle vcpu. And yes I understand that some limited 
work is done there, e.g. memory scrubbing, but 1.) there is a difference 
between light/limited work that can be done within the confines of a 
domain and work requiring hypercalls, and 2.) this precedence may have 
been due to limitations vs being the necessarily correct approach.

> The first question came up during ongoing work in LiveUpdate. After an
> LU, the next Xen needs to restore all domains. To do that, some
> hypercalls need to be issued from the idle domain context and
> apparently XSM does not like it. We need to introduce hacks in the
> dummy module to leave the idle domain alone. Our work is not compiled
> with CONFIG_XSM at all, but with CONFIG_XSM, are we able to enforce
> security policies against the idle domain? Of course, without any LU
> work this does not make any difference because the idle domain does not
> do any useful work to be restricted anyway.

Why do they "need to be issued from the idle domain"? As was suggested 
by Jason, why isn't this done from a construction domain context? I will 
interject here that with DomB that is what we will be doing and it 
sounds like LiveUpdate is very similar to the relaunch concept that DomB 
is being constructed to support.

Yes XSM did not like it because an analogy of what is being done is like 
trying to do a system call from inside an OS kernel. Again AIUI the idle 
domain is not a real domain but an internal construct for the scheduler 
to manage idle vcpu and attempting to make hypercalls from it is in fact 
attempting to turn into a full fledged domain.

 From a security perspective, if hacks to the XSM hooks are necessary to 
make something work then it is highly recommended to take a step back 
and ask why and whether you are doing something that is not safe from a 
security perspective.

> Also, should idle domain be restricted? IMO the idle domain is Xen
> itself which mostly bootstraps the system and performs limited work
> when switched to, and is not something a user (either dom0 or domU)
> directly interacts with. I doubt XSM was designed to include the idle
> domain (although there is an ID allocated for it in the code), so I
> would say just exclude idle in all security policy checks.

The idle domain is a limited, internal construct within the hypervisor 
and should be constrained as part of the hypervisor, which is why its 
domain id gets labeled with the same label as the hypervisor. For this 
reason I would wholeheartedly disagree with exempting the idle domain id 
from XSM hooks as that would effectively be saying the core hypervisor 
should not be constrained. The purpose of the XSM hooks is to control 
the flow of information in the system in a non-bypassable way. Codifying 
bypasses completely subverts the security model behind XSM for which the 
flask security server is dependent upon.

> I may have missed some points in that discussion, so please feel free
> to add.
> 
> Hongyan
> 
> 

V/r,
DPS


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: XSM and the idle domain
  2020-10-22  1:23 ` Daniel P. Smith
@ 2020-10-22  8:13   ` Jan Beulich
  2020-10-22 13:03     ` Daniel Smith
  0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2020-10-22  8:13 UTC (permalink / raw)
  To: Daniel P. Smith; +Cc: Hongyan Xia, xen-devel, andrew.cooper3, jandryuk, dgdegra

On 22.10.2020 03:23, Daniel P. Smith wrote:
> On 10/21/20 10:34 AM, Hongyan Xia wrote:
>> Also, should idle domain be restricted? IMO the idle domain is Xen
>> itself which mostly bootstraps the system and performs limited work
>> when switched to, and is not something a user (either dom0 or domU)
>> directly interacts with. I doubt XSM was designed to include the idle
>> domain (although there is an ID allocated for it in the code), so I
>> would say just exclude idle in all security policy checks.
> 
> The idle domain is a limited, internal construct within the hypervisor 
> and should be constrained as part of the hypervisor, which is why its 
> domain id gets labeled with the same label as the hypervisor. For this 
> reason I would wholeheartedly disagree with exempting the idle domain id 
> from XSM hooks as that would effectively be saying the core hypervisor 
> should not be constrained. The purpose of the XSM hooks is to control 
> the flow of information in the system in a non-bypassable way. Codifying 
> bypasses completely subverts the security model behind XSM for which the 
> flask security server is dependent upon.

While what you say may in general make sense, I have two questions:
1) When the idle domain is purely an internal construct of Xen, why
   does it need limiting in any way? In fact, if restricting it in a
   bad way, aren't you risking to prevent the system from functioning
   correctly?
2) LU is merely restoring the prior state of the system. This prior
   state was reached with security auditing as per the system's
   policy at the time. Why should there be anything denind in the
   process of re-establishing this same state? IOW can't XSM checking
   be globally disabled until the system is ready be run normally
   again?
Please forgive if this sounds like rubbish to you - I may not have a
good enough understanding of the abstract constraints involved here.

Jan


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: XSM and the idle domain
  2020-10-21 16:21 ` Jason Andryuk
@ 2020-10-22 11:29   ` Hongyan Xia
  0 siblings, 0 replies; 13+ messages in thread
From: Hongyan Xia @ 2020-10-22 11:29 UTC (permalink / raw)
  To: Jason Andryuk; +Cc: xen-devel, Jan Beulich, Andrew Cooper, Daniel De Graaf

(also replying to others in this thread.)

On Wed, 2020-10-21 at 12:21 -0400, Jason Andryuk wrote:
> On Wed, Oct 21, 2020 at 10:35 AM Hongyan Xia <hx242@xen.org> wrote:
> > 
> > Hi,
> 
> ...
> > 
> > The first question came up during ongoing work in LiveUpdate. After
> > an
> > LU, the next Xen needs to restore all domains. To do that, some
> > hypercalls need to be issued from the idle domain context and
> > apparently XSM does not like it. We need to introduce hacks in the
> > dummy module to leave the idle domain alone.
> 
> Is this modifying xsm_default_action() to add an is_idle_domain()
> check which always succeeds?

Yes. We had to do exactly that to avoid LU actions being denied by XSM.

> > Our work is not compiled
> > with CONFIG_XSM at all, but with CONFIG_XSM, are we able to enforce
> > security policies against the idle domain?
> 
> It's not clear to me if you want to use CONFIG_XSM, or just don't
> want
> to break it.

We don't (and won't) enable XSM in our build, but still we need a hack
to work around it, so I am just curious about what happens when people
use both LU and XSM at the same time.

> > Of course, without any LU
> > work this does not make any difference because the idle domain does
> > not
> > do any useful work to be restricted anyway.
> 
> I think this last sentence is the main point.  It's always been
> labeled xen_t, but since it doesn't go through any of the hook
> points,
> it hasn't needed any restrictions.  Actually, reviewing the Flask
> policy there is:
> # Domain destruction can result in some access checks for actions
> performed by
> # the hypervisor.  These should always be allowed.
> allow xen_t resource_type : resource { remove_irq remove_ioport
> remove_iomem };
> 
> > Also, should idle domain be restricted? IMO the idle domain is Xen
> > itself which mostly bootstraps the system and performs limited work
> > when switched to, and is not something a user (either dom0 or domU)
> > directly interacts with. I doubt XSM was designed to include the
> > idle
> > domain (although there is an ID allocated for it in the code), so I
> > would say just exclude idle in all security policy checks.
> 
> I think it makes sense to label xen_t, even if it doesn't do
> anything.
> As you say, it is a distinct entity from dom0 and domU.  Yes, it can
> circumvent the policy, but it's not actively hurting anything.  And
> it
> can be good to catch when it does start doing something, as you
> found.
> 
> Might it make sense to create a LU domain instead of using the idle
> domain for Live Update?  Another approach could be to run the
> idle_domain as "dom0" during Live Update, and then transition to the
> regular idle_domain when it completes?  You are re-creating dom0, but
> you could flip is_privileged on during live update and then remove it
> once complete.

Actually I think your suggestion and what Daniel suggested make sense.
We could just have a domLU that does all the restore work which has its
own security policies. That sounds like a clean solution to me.
However, one top priority of LU is to minimise the down time so that
domains won't feel a thing and every millisecond counts. I don't know
how much overhead this adds (maybe negligible if we just let domLU sit
in idle domain's page tables so switching and passing the LU save
stream to it is painless), but is something we need to keep in mind.

But this still sidesteps the question of whether the idle domain should
be subject to security policies. From another reply it sounds like the
idle domain should not be exempt from XSM. Although, to me restrictions
on idle domain are more like a debugging feature than a security
policy, since it prevents, e.g., accidentally issuing hypercalls from
it, but if the idle domain really wants to do something then there is
nothing to stop it. This is different from enforcing policies on a real
domain which guarantees things won't happen and the domain simply has
no mechanism to circumvent it (hopefully).

My experience with XSM is only the idle domain hack for LU so what I
said about it here may not make sense.

Hongyan



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: XSM and the idle domain
  2020-10-21 14:34 XSM and the idle domain Hongyan Xia
  2020-10-21 16:21 ` Jason Andryuk
  2020-10-22  1:23 ` Daniel P. Smith
@ 2020-10-22 12:51 ` Andrew Cooper
  2020-10-22 17:01   ` Hongyan Xia
  2 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2020-10-22 12:51 UTC (permalink / raw)
  To: Hongyan Xia, xen-devel, jbeulich, jandryuk, dgdegra

On 21/10/2020 15:34, Hongyan Xia wrote:
> The first question came up during ongoing work in LiveUpdate. After an
> LU, the next Xen needs to restore all domains. To do that, some
> hypercalls need to be issued from the idle domain context and
> apparently XSM does not like it.

There is no such thing as issuing hypercalls from the idle domain
(context or otherwise), because the idle domain does not have enough
associated guest state for anything to make the requisite
SYSCALL/INT80/VMCALL/VMMCALL invocation.

I presume from this comment that what you mean is that you're calling
the plain hypercall functions, context checks and everything, from the
idle context?

If so, this is buggy for more reasons than just XSM objecting to its
calling context, and that XSM is merely the first thing to explode. 
Therefore, I don't think modifications to XSM are applicable to solving
the problem.

(Of course, this is all speculation because there's no concrete
implementation to look at.)

~Andrew


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: XSM and the idle domain
  2020-10-22  8:13   ` Jan Beulich
@ 2020-10-22 13:03     ` Daniel Smith
  0 siblings, 0 replies; 13+ messages in thread
From: Daniel Smith @ 2020-10-22 13:03 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Hongyan Xia, xen-devel, andrew.cooper3, jandryuk, dgdegra

---- On Thu, 22 Oct 2020 04:13:53 -0400 Jan Beulich <jbeulich@suse.com> wrote ----

 > On 22.10.2020 03:23, Daniel P. Smith wrote: 
 > > On 10/21/20 10:34 AM, Hongyan Xia wrote: 
 > >> Also, should idle domain be restricted? IMO the idle domain is Xen 
 > >> itself which mostly bootstraps the system and performs limited work 
 > >> when switched to, and is not something a user (either dom0 or domU) 
 > >> directly interacts with. I doubt XSM was designed to include the idle 
 > >> domain (although there is an ID allocated for it in the code), so I 
 > >> would say just exclude idle in all security policy checks. 
 > > 
 > > The idle domain is a limited, internal construct within the hypervisor 
 > > and should be constrained as part of the hypervisor, which is why its 
 > > domain id gets labeled with the same label as the hypervisor. For this 
 > > reason I would wholeheartedly disagree with exempting the idle domain id 
 > > from XSM hooks as that would effectively be saying the core hypervisor 
 > > should not be constrained. The purpose of the XSM hooks is to control 
 > > the flow of information in the system in a non-bypassable way. Codifying 
 > > bypasses completely subverts the security model behind XSM for which the 
 > > flask security server is dependent upon. 
 >  
 > While what you say may in general make sense, I have two questions: 

[Apologies for any poor formatting, responding from webmail interface ( ._.)]

Hey Jan, these are very legitimate questions.

 > 1) When the idle domain is purely an internal construct of Xen, why 
 >  does it need limiting in any way? In fact, if restricting it in a 
 >  bad way, aren't you risking to prevent the system from functioning 
 >  correctly? 

Think in terms of least privilege, do you want the idle domain and by extension the hypervisor to have the additional privilege of imposing state on to the system as opposed to processing the state changes. I am not saying it is wrong technical approach (though I do believe at a minimum the implementation approach is flawed), I am just asking is it wise from a privilege delegation aspect of whether it could be done differently from a technical stand point. The underlying concern here is once you grant the privilege the hypervisor will forever have the privilege which can be used for good (LU) and bad (corruption). Take for instance what is being attempted with DomB, in this approach the privilege to impose state (configure domains) is delegated to the Boot Domain but it is not delegated the privilege to create state (domain creation). As I mentioned before, this is what Jason was suggesting in having another domain type that is allowed to impose the state that is transitioned to from the idle domain to conduct the action.

Whether or not the idle domain is allowed to make hypercalls is not necessarily a concern of the XSM hooks. If it is decided that this is the desired path, then what is of concern is that the corrective action does not weaken/break the hooks. If this ends up being the desired approach, then IMHO the correct action is to update the dummy policy, flask policy, and SILO (if it applies) to allow the privilege/access to occur versus putting bypasses into the security hooks.

 > 2) LU is merely restoring the prior state of the system. This prior 
 >  state was reached with security auditing as per the system's 
 >  policy at the time. Why should there be anything denind in the 
 >  process of re-establishing this same state? IOW can't XSM checking 
 >  be globally disabled until the system is ready be run normally 
 >  again?

There is an assumption you made there that is being overlooked and that is you are assuming it is the same state. It is important to understand what assumptions are being made and when possible impose those assumptions through policy than with code. Not everyone will want to make the same assumptions and may want a better controlled path for that state to flow.

No you don't want to globally disable the XSM checking as that means you have lost all control over the system where any and all policy violations could occur without any auditing. This would open a huge hole for a malicious actor to take advantage of for an attack against the system. 

In the end to reiterate, if this is decided to be the desired approach then IMHO the correct implementation is to encode the access in policy not in bypasses to the XSM hooks.

 > Please forgive if this sounds like rubbish to you - I may not have a 
 > good enough understanding of the abstract constraints involved here. 

No worries, it is always better to question when in doubt than making an assumption. Hopefully I helped in providing a better explanation.

 > Jan 
 > 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: XSM and the idle domain
  2020-10-22 12:51 ` Andrew Cooper
@ 2020-10-22 17:01   ` Hongyan Xia
  2020-10-26 13:37     ` Jason Andryuk
  0 siblings, 1 reply; 13+ messages in thread
From: Hongyan Xia @ 2020-10-22 17:01 UTC (permalink / raw)
  To: Andrew Cooper, xen-devel, jbeulich, jandryuk, dgdegra

On Thu, 2020-10-22 at 13:51 +0100, Andrew Cooper wrote:
> On 21/10/2020 15:34, Hongyan Xia wrote:
> > The first question came up during ongoing work in LiveUpdate. After
> > an
> > LU, the next Xen needs to restore all domains. To do that, some
> > hypercalls need to be issued from the idle domain context and
> > apparently XSM does not like it.
> 
> There is no such thing as issuing hypercalls from the idle domain
> (context or otherwise), because the idle domain does not have enough
> associated guest state for anything to make the requisite
> SYSCALL/INT80/VMCALL/VMMCALL invocation.
> 
> I presume from this comment that what you mean is that you're calling
> the plain hypercall functions, context checks and everything, from
> the
> idle context?

Yep, the restore code just calls the hypercall functions from idle
context.

> If so, this is buggy for more reasons than just XSM objecting to its
> calling context, and that XSM is merely the first thing to explode. 
> Therefore, I don't think modifications to XSM are applicable to
> solving
> the problem.
> 
> (Of course, this is all speculation because there's no concrete
> implementation to look at.)

Another explosion is the inability to create hypercall preemption,
which for now is disabled when the calling context is the idle domain. 
Apart from XSM and preemption, the LU prototype works fine. We only
reuse a limited number of hypercall functions and are not trying to be
able to call all possible hypercalls from idle.

Having a dedicated domLU just like domB (or reusing domB) sounds like a
viable option. If the overhead can be made low enough then we won't
need to work around XSM and hypercall preemption.

Although the question was whether XSM should interact with the idle
domain. With a good design LU should be able to sidestep this though.

Hongyan



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: XSM and the idle domain
  2020-10-22 17:01   ` Hongyan Xia
@ 2020-10-26 13:37     ` Jason Andryuk
  2020-10-26 13:46       ` [RFC PATCH] xsm: Re-work domain_create and domain_alloc_security Jason Andryuk
  2020-10-26 16:43       ` XSM and the idle domain Andrew Cooper
  0 siblings, 2 replies; 13+ messages in thread
From: Jason Andryuk @ 2020-10-26 13:37 UTC (permalink / raw)
  To: Hongyan Xia
  Cc: Andrew Cooper, xen-devel, Jan Beulich, Daniel De Graaf, Daniel Smith

On Thu, Oct 22, 2020 at 1:01 PM Hongyan Xia <hx242@xen.org> wrote:
>
> On Thu, 2020-10-22 at 13:51 +0100, Andrew Cooper wrote:
> > On 21/10/2020 15:34, Hongyan Xia wrote:
> > > The first question came up during ongoing work in LiveUpdate. After
> > > an
> > > LU, the next Xen needs to restore all domains. To do that, some
> > > hypercalls need to be issued from the idle domain context and
> > > apparently XSM does not like it.
> >
> > There is no such thing as issuing hypercalls from the idle domain
> > (context or otherwise), because the idle domain does not have enough
> > associated guest state for anything to make the requisite
> > SYSCALL/INT80/VMCALL/VMMCALL invocation.
> >
> > I presume from this comment that what you mean is that you're calling
> > the plain hypercall functions, context checks and everything, from
> > the
> > idle context?
>
> Yep, the restore code just calls the hypercall functions from idle
> context.
>
> > If so, this is buggy for more reasons than just XSM objecting to its
> > calling context, and that XSM is merely the first thing to explode.
> > Therefore, I don't think modifications to XSM are applicable to
> > solving
> > the problem.
> >
> > (Of course, this is all speculation because there's no concrete
> > implementation to look at.)
>
> Another explosion is the inability to create hypercall preemption,
> which for now is disabled when the calling context is the idle domain.
> Apart from XSM and preemption, the LU prototype works fine. We only
> reuse a limited number of hypercall functions and are not trying to be
> able to call all possible hypercalls from idle.

I wonder if for domain_create, it wouldn't be better to move
xsm_domain_create() out to the domctl (hypercall entry) and check it
there.  That would side-step xsm in domain_create.  Flask would need
to be modified for that.  I've an untested patch doing the
rearranging, which I'll send as a follow up.

What other hypercalls are you having issues with?  Those could also be
refactored so the hypercall entry checks permissions, and the actual
work is done in a directly callable function.

> Having a dedicated domLU just like domB (or reusing domB) sounds like a
> viable option. If the overhead can be made low enough then we won't
> need to work around XSM and hypercall preemption.
>
> Although the question was whether XSM should interact with the idle
> domain. With a good design LU should be able to sidestep this though.

Circling back to the main topic, is the idle domain Xen, or is it
distinct?  It runs in the context of Xen, so Xen isn't really in a
place to enforce policy on itself.  Hongyan, as you said earlier,
applying XSM is more of a debugging feature at that point than a
security feature.  And as Jan pointed out, you can have problems if
XSM prevents the hypervisor from performing an action it doesn't
expect to fail.

Regards,
Jason


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [RFC PATCH] xsm: Re-work domain_create and domain_alloc_security
  2020-10-26 13:37     ` Jason Andryuk
@ 2020-10-26 13:46       ` Jason Andryuk
  2020-10-26 16:23         ` Daniel Smith
  2020-10-26 16:43       ` XSM and the idle domain Andrew Cooper
  1 sibling, 1 reply; 13+ messages in thread
From: Jason Andryuk @ 2020-10-26 13:46 UTC (permalink / raw)
  To: xen-devel, hx242
  Cc: dpsmith, Jason Andryuk, Andrew Cooper, Jan Beulich, Daniel De Graaf

Untested!

This only really matters for flask, but all of xsm is updated.

flask_domain_create() and flask_domain_alloc_security() are a strange
pair.

flask_domain_create() serves double duty.  It both assigns sid and
self_sid values and checks if the calling domain has permission to
create the target domain.  It also has special casing for handling dom0.
Meanwhile flask_domain_alloc_security() assigns some special sids, but
waits for others to be assigned in flask_domain_create.  This split
seems to have come about so that the structures are allocated before
calling flask_domain_create().  It also means flask_domain_create is
called in the middle of domain_create.

Re-arrange the two calls.  Let flask_domain_create just check if current
has permission to create ssidref.  Then it can be moved out to do_domctl
and gate entry into domain_create.  This avoids doing partial domain
creation before the permission check.

Have flask_domain_alloc_security() take a ssidref argument.  The ssidref
was already permission checked earlier, so it can just be assigned.
Then the self_sid can be calculated here as well rather than in
flask_domain_create().

The dom0 special casing is moved into flask_domain_alloc_security().
Maybe this should be just a fall-through for the dom0 already created
case.  This code may not be needed any longer.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 xen/common/domain.c     |  6 ++----
 xen/common/domctl.c     |  4 ++++
 xen/include/xsm/dummy.h |  6 +++---
 xen/include/xsm/xsm.h   | 12 +++++------
 xen/xsm/flask/hooks.c   | 48 ++++++++++++++++-------------------------
 5 files changed, 34 insertions(+), 42 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index f748806a45..6b1f5ed59d 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -407,7 +407,8 @@ struct domain *domain_create(domid_t domid,
 
     lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid);
 
-    if ( (err = xsm_alloc_security_domain(d)) != 0 )
+    if ( (err = xsm_alloc_security_domain(d, config ? config->ssidref :
+                                                      0)) != 0 )
         goto fail;
 
     atomic_set(&d->refcnt, 1);
@@ -470,9 +471,6 @@ struct domain *domain_create(domid_t domid,
         if ( !d->iomem_caps || !d->irq_caps )
             goto fail;
 
-        if ( (err = xsm_domain_create(XSM_HOOK, d, config->ssidref)) != 0 )
-            goto fail;
-
         d->controller_pause_count = 1;
         atomic_inc(&d->pause_count);
 
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index af044e2eda..ffdc1a41cd 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -406,6 +406,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         domid_t        dom;
         static domid_t rover = 0;
 
+        ret = xsm_domain_create(XSM_HOOK, op->u.createdomain.ssidref);
+        if (ret)
+            break;
+
         dom = op->domain;
         if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) )
         {
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 7ae3c40eb5..29c4ca9951 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -104,10 +104,10 @@ static XSM_INLINE void xsm_security_domaininfo(struct domain *d,
     return;
 }
 
-static XSM_INLINE int xsm_domain_create(XSM_DEFAULT_ARG struct domain *d, u32 ssidref)
+static XSM_INLINE int xsm_domain_create(XSM_DEFAULT_ARG u32 ssidref)
 {
     XSM_ASSERT_ACTION(XSM_HOOK);
-    return xsm_default_action(action, current->domain, d);
+    return xsm_default_action(action, current->domain, NULL);
 }
 
 static XSM_INLINE int xsm_getdomaininfo(XSM_DEFAULT_ARG struct domain *d)
@@ -163,7 +163,7 @@ static XSM_INLINE int xsm_readconsole(XSM_DEFAULT_ARG uint32_t clear)
     return xsm_default_action(action, current->domain, NULL);
 }
 
-static XSM_INLINE int xsm_alloc_security_domain(struct domain *d)
+static XSM_INLINE int xsm_alloc_security_domain(struct domain *d, uint32_t ssidref)
 {
     return 0;
 }
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 358ec13ba8..c1d2ef5832 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -46,7 +46,7 @@ typedef enum xsm_default xsm_default_t;
 struct xsm_operations {
     void (*security_domaininfo) (struct domain *d,
                                         struct xen_domctl_getdomaininfo *info);
-    int (*domain_create) (struct domain *d, u32 ssidref);
+    int (*domain_create) (u32 ssidref);
     int (*getdomaininfo) (struct domain *d);
     int (*domctl_scheduler_op) (struct domain *d, int op);
     int (*sysctl_scheduler_op) (int op);
@@ -71,7 +71,7 @@ struct xsm_operations {
     int (*grant_copy) (struct domain *d1, struct domain *d2);
     int (*grant_query_size) (struct domain *d1, struct domain *d2);
 
-    int (*alloc_security_domain) (struct domain *d);
+    int (*alloc_security_domain) (struct domain *d, uint32_t ssidref);
     void (*free_security_domain) (struct domain *d);
     int (*alloc_security_evtchn) (struct evtchn *chn);
     void (*free_security_evtchn) (struct evtchn *chn);
@@ -202,9 +202,9 @@ static inline void xsm_security_domaininfo (struct domain *d,
     xsm_ops->security_domaininfo(d, info);
 }
 
-static inline int xsm_domain_create (xsm_default_t def, struct domain *d, u32 ssidref)
+static inline int xsm_domain_create (xsm_default_t def, u32 ssidref)
 {
-    return xsm_ops->domain_create(d, ssidref);
+    return xsm_ops->domain_create(ssidref);
 }
 
 static inline int xsm_getdomaininfo (xsm_default_t def, struct domain *d)
@@ -305,9 +305,9 @@ static inline int xsm_grant_query_size (xsm_default_t def, struct domain *d1, st
     return xsm_ops->grant_query_size(d1, d2);
 }
 
-static inline int xsm_alloc_security_domain (struct domain *d)
+static inline int xsm_alloc_security_domain (struct domain *d, uint32_t ssidref)
 {
-    return xsm_ops->alloc_security_domain(d);
+    return xsm_ops->alloc_security_domain(d, ssidref);
 }
 
 static inline void xsm_free_security_domain (struct domain *d)
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index de050cc9fe..719fe90f22 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -156,9 +156,11 @@ static int avc_unknown_permission(const char *name, int id)
     return rc;
 }
 
-static int flask_domain_alloc_security(struct domain *d)
+static int flask_domain_alloc_security(struct domain *d, u32 ssidref)
 {
     struct domain_security_struct *dsec;
+    static int dom0_created = 0;
+    int rc;
 
     dsec = xzalloc(struct domain_security_struct);
     if ( !dsec )
@@ -175,14 +177,24 @@ static int flask_domain_alloc_security(struct domain *d)
     case DOMID_IO:
         dsec->sid = SECINITSID_DOMIO;
         break;
+    case 0:
+        if ( !dom0_created ) {
+            dsec->sid = SECINITSID_DOM0;
+            dom0_created = 1;
+        } else {
+            dsec->sid = SECINITSID_UNLABELED;
+        }
+        break;
     default:
-        dsec->sid = SECINITSID_UNLABELED;
+        dsec->sid = ssidref;
     }
 
     dsec->self_sid = dsec->sid;
-    d->ssid = dsec;
 
-    return 0;
+    rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
+                                 &dsec->self_sid);
+
+    return rc;
 }
 
 static void flask_domain_free_security(struct domain *d)
@@ -507,32 +519,10 @@ static void flask_security_domaininfo(struct domain *d,
     info->ssidref = domain_sid(d);
 }
 
-static int flask_domain_create(struct domain *d, u32 ssidref)
+static int flask_domain_create(u32 ssidref)
 {
-    int rc;
-    struct domain_security_struct *dsec = d->ssid;
-    static int dom0_created = 0;
-
-    if ( is_idle_domain(current->domain) && !dom0_created )
-    {
-        dsec->sid = SECINITSID_DOM0;
-        dom0_created = 1;
-    }
-    else
-    {
-        rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN,
-                          DOMAIN__CREATE, NULL);
-        if ( rc )
-            return rc;
-
-        dsec->sid = ssidref;
-    }
-    dsec->self_sid = dsec->sid;
-
-    rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN,
-                                 &dsec->self_sid);
-
-    return rc;
+    return avc_current_has_perm(ssidref, SECCLASS_DOMAIN, DOMAIN__CREATE,
+                                NULL);
 }
 
 static int flask_getdomaininfo(struct domain *d)
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH] xsm: Re-work domain_create and domain_alloc_security
  2020-10-26 13:46       ` [RFC PATCH] xsm: Re-work domain_create and domain_alloc_security Jason Andryuk
@ 2020-10-26 16:23         ` Daniel Smith
  2020-10-26 16:39           ` Jason Andryuk
  0 siblings, 1 reply; 13+ messages in thread
From: Daniel Smith @ 2020-10-26 16:23 UTC (permalink / raw)
  To: Jason Andryuk
  Cc: xen-devel, hx242, Andrew Cooper, Jan Beulich, Daniel De Graaf

---- On Mon, 26 Oct 2020 09:46:51 -0400 Jason Andryuk <jandryuk@gmail.com> wrote ----

 > Untested! 
 >  
 > This only really matters for flask, but all of xsm is updated. 
 >  
 > flask_domain_create() and flask_domain_alloc_security() are a strange 
 > pair. 
 >  
 > flask_domain_create() serves double duty.  It both assigns sid and 
 > self_sid values and checks if the calling domain has permission to 
 > create the target domain.  It also has special casing for handling dom0. 
 > Meanwhile flask_domain_alloc_security() assigns some special sids, but 
 > waits for others to be assigned in flask_domain_create.  This split 
 > seems to have come about so that the structures are allocated before 
 > calling flask_domain_create().  It also means flask_domain_create is 
 > called in the middle of domain_create. 
 >  
 > Re-arrange the two calls.  Let flask_domain_create just check if current 
 > has permission to create ssidref.  Then it can be moved out to do_domctl 
 > and gate entry into domain_create.  This avoids doing partial domain 
 > creation before the permission check. 
 >  
 > Have flask_domain_alloc_security() take a ssidref argument.  The ssidref 
 > was already permission checked earlier, so it can just be assigned. 
 > Then the self_sid can be calculated here as well rather than in 
 > flask_domain_create(). 
 >  
 > The dom0 special casing is moved into flask_domain_alloc_security(). 
 > Maybe this should be just a fall-through for the dom0 already created 
 > case.  This code may not be needed any longer. 
 >  
 > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> 
 > --- 
 >  xen/common/domain.c     |  6 ++---- 
 >  xen/common/domctl.c     |  4 ++++ 
 >  xen/include/xsm/dummy.h |  6 +++--- 
 >  xen/include/xsm/xsm.h   | 12 +++++------ 
 >  xen/xsm/flask/hooks.c   | 48 ++++++++++++++++------------------------- 
 >  5 files changed, 34 insertions(+), 42 deletions(-) 
 >  
 > diff --git a/xen/common/domain.c b/xen/common/domain.c 
 > index f748806a45..6b1f5ed59d 100644 
 > --- a/xen/common/domain.c 
 > +++ b/xen/common/domain.c 
 > @@ -407,7 +407,8 @@ struct domain *domain_create(domid_t domid, 
 >  
 >  lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid); 
 >  
 > -    if ( (err = xsm_alloc_security_domain(d)) != 0 ) 
 > +    if ( (err = xsm_alloc_security_domain(d, config ? config->ssidref : 
 > +                                                      0)) != 0 ) 
 >  goto fail; 
 >  
 >  atomic_set(&d->refcnt, 1); 
 > @@ -470,9 +471,6 @@ struct domain *domain_create(domid_t domid, 
 >  if ( !d->iomem_caps || !d->irq_caps ) 
 >  goto fail; 
 >  
 > -        if ( (err = xsm_domain_create(XSM_HOOK, d, config->ssidref)) != 0 ) 
 > -            goto fail; 
 > - 
 >  d->controller_pause_count = 1; 
 >  atomic_inc(&d->pause_count); 
 >  
 > diff --git a/xen/common/domctl.c b/xen/common/domctl.c 
 > index af044e2eda..ffdc1a41cd 100644 
 > --- a/xen/common/domctl.c 
 > +++ b/xen/common/domctl.c 
 > @@ -406,6 +406,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) 
 >  domid_t        dom; 
 >  static domid_t rover = 0; 
 >  
 > +        ret = xsm_domain_create(XSM_HOOK, op->u.createdomain.ssidref); 
 > +        if (ret) 
 > +            break; 
 > + 
 >  dom = op->domain; 
 >  if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) ) 
 >  { 
 > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h 
 > index 7ae3c40eb5..29c4ca9951 100644 
 > --- a/xen/include/xsm/dummy.h 
 > +++ b/xen/include/xsm/dummy.h 
 > @@ -104,10 +104,10 @@ static XSM_INLINE void xsm_security_domaininfo(struct domain *d, 
 >  return; 
 >  } 
 >  
 > -static XSM_INLINE int xsm_domain_create(XSM_DEFAULT_ARG struct domain *d, u32 ssidref) 
 > +static XSM_INLINE int xsm_domain_create(XSM_DEFAULT_ARG u32 ssidref) 
 >  { 
 >  XSM_ASSERT_ACTION(XSM_HOOK); 
 > -    return xsm_default_action(action, current->domain, d); 
 > +    return xsm_default_action(action, current->domain, NULL); 
 >  } 
 >  
 >  static XSM_INLINE int xsm_getdomaininfo(XSM_DEFAULT_ARG struct domain *d) 
 > @@ -163,7 +163,7 @@ static XSM_INLINE int xsm_readconsole(XSM_DEFAULT_ARG uint32_t clear) 
 >  return xsm_default_action(action, current->domain, NULL); 
 >  } 
 >  
 > -static XSM_INLINE int xsm_alloc_security_domain(struct domain *d) 
 > +static XSM_INLINE int xsm_alloc_security_domain(struct domain *d, uint32_t ssidref) 
 >  { 
 >  return 0; 
 >  } 
 > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h 
 > index 358ec13ba8..c1d2ef5832 100644 
 > --- a/xen/include/xsm/xsm.h 
 > +++ b/xen/include/xsm/xsm.h 
 > @@ -46,7 +46,7 @@ typedef enum xsm_default xsm_default_t; 
 >  struct xsm_operations { 
 >  void (*security_domaininfo) (struct domain *d, 
 >  struct xen_domctl_getdomaininfo *info); 
 > -    int (*domain_create) (struct domain *d, u32 ssidref); 
 > +    int (*domain_create) (u32 ssidref); 
 >  int (*getdomaininfo) (struct domain *d); 
 >  int (*domctl_scheduler_op) (struct domain *d, int op); 
 >  int (*sysctl_scheduler_op) (int op); 
 > @@ -71,7 +71,7 @@ struct xsm_operations { 
 >  int (*grant_copy) (struct domain *d1, struct domain *d2); 
 >  int (*grant_query_size) (struct domain *d1, struct domain *d2); 
 >  
 > -    int (*alloc_security_domain) (struct domain *d); 
 > +    int (*alloc_security_domain) (struct domain *d, uint32_t ssidref); 
 >  void (*free_security_domain) (struct domain *d); 
 >  int (*alloc_security_evtchn) (struct evtchn *chn); 
 >  void (*free_security_evtchn) (struct evtchn *chn); 
 > @@ -202,9 +202,9 @@ static inline void xsm_security_domaininfo (struct domain *d, 
 >  xsm_ops->security_domaininfo(d, info); 
 >  } 
 >  
 > -static inline int xsm_domain_create (xsm_default_t def, struct domain *d, u32 ssidref) 
 > +static inline int xsm_domain_create (xsm_default_t def, u32 ssidref) 
 >  { 
 > -    return xsm_ops->domain_create(d, ssidref); 
 > +    return xsm_ops->domain_create(ssidref); 
 >  } 
 >  
 >  static inline int xsm_getdomaininfo (xsm_default_t def, struct domain *d) 
 > @@ -305,9 +305,9 @@ static inline int xsm_grant_query_size (xsm_default_t def, struct domain *d1, st 
 >  return xsm_ops->grant_query_size(d1, d2); 
 >  } 
 >  
 > -static inline int xsm_alloc_security_domain (struct domain *d) 
 > +static inline int xsm_alloc_security_domain (struct domain *d, uint32_t ssidref) 
 >  { 
 > -    return xsm_ops->alloc_security_domain(d); 
 > +    return xsm_ops->alloc_security_domain(d, ssidref); 
 >  } 
 >  
 >  static inline void xsm_free_security_domain (struct domain *d) 
 > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c 
 > index de050cc9fe..719fe90f22 100644 
 > --- a/xen/xsm/flask/hooks.c 
 > +++ b/xen/xsm/flask/hooks.c 
 > @@ -156,9 +156,11 @@ static int avc_unknown_permission(const char *name, int id) 
 >  return rc; 
 >  } 
 >  
 > -static int flask_domain_alloc_security(struct domain *d) 
 > +static int flask_domain_alloc_security(struct domain *d, u32 ssidref) 
 >  { 
 >  struct domain_security_struct *dsec; 
 > +    static int dom0_created = 0; 
 > +    int rc; 
 >  
 >  dsec = xzalloc(struct domain_security_struct); 
 >  if ( !dsec ) 
 > @@ -175,14 +177,24 @@ static int flask_domain_alloc_security(struct domain *d) 
 >  case DOMID_IO: 
 >  dsec->sid = SECINITSID_DOMIO; 
 >  break; 
 > +    case 0: 
 > +        if ( !dom0_created ) { 
 > +            dsec->sid = SECINITSID_DOM0; 
 > +            dom0_created = 1; 
 > +        } else { 
 > +            dsec->sid = SECINITSID_UNLABELED; 
 > +        } 

While the handling of this case is not wrong, I have to wonder if there is a better way to handle the dom0 creation case.

 > +        break; 
 >  default: 
 > -        dsec->sid = SECINITSID_UNLABELED; 
 > +        dsec->sid = ssidref; 
 >  } 
 >  
 >  dsec->self_sid = dsec->sid; 
 > -    d->ssid = dsec; 

I don't think you meant to deleted that, without it domains will have no ssid assigned to them.

 > -    return 0; 
 > +    rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN, 
 > +                                 &dsec->self_sid); 
 > + 
 > +    return rc; 
 >  } 
 >  
 >  static void flask_domain_free_security(struct domain *d) 
 > @@ -507,32 +519,10 @@ static void flask_security_domaininfo(struct domain *d, 
 >  info->ssidref = domain_sid(d); 
 >  } 
 >  
 > -static int flask_domain_create(struct domain *d, u32 ssidref) 
 > +static int flask_domain_create(u32 ssidref) 
 >  { 
 > -    int rc; 
 > -    struct domain_security_struct *dsec = d->ssid; 
 > -    static int dom0_created = 0; 
 > - 
 > -    if ( is_idle_domain(current->domain) && !dom0_created ) 
 > -    { 
 > -        dsec->sid = SECINITSID_DOM0; 
 > -        dom0_created = 1; 
 > -    } 
 > -    else 
 > -    { 
 > -        rc = avc_current_has_perm(ssidref, SECCLASS_DOMAIN, 
 > -                          DOMAIN__CREATE, NULL); 
 > -        if ( rc ) 
 > -            return rc; 
 > - 
 > -        dsec->sid = ssidref; 
 > -    } 
 > -    dsec->self_sid = dsec->sid; 
 > - 
 > -    rc = security_transition_sid(dsec->sid, dsec->sid, SECCLASS_DOMAIN, 
 > -                                 &dsec->self_sid); 
 > - 
 > -    return rc; 
 > +    return avc_current_has_perm(ssidref, SECCLASS_DOMAIN, DOMAIN__CREATE, 
 > +                                NULL); 
 >  } 
 >  
 >  static int flask_getdomaininfo(struct domain *d) 
 > -- 
 > 2.26.2 
 >  
 

V/r,
Daniel P. Smith
Apertus Solutions, LLC





^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH] xsm: Re-work domain_create and domain_alloc_security
  2020-10-26 16:23         ` Daniel Smith
@ 2020-10-26 16:39           ` Jason Andryuk
  0 siblings, 0 replies; 13+ messages in thread
From: Jason Andryuk @ 2020-10-26 16:39 UTC (permalink / raw)
  To: Daniel Smith
  Cc: xen-devel, hx242, Andrew Cooper, Jan Beulich, Daniel De Graaf

On Mon, Oct 26, 2020 at 12:23 PM Daniel Smith
<dpsmith@apertussolutions.com> wrote:
>
> ---- On Mon, 26 Oct 2020 09:46:51 -0400 Jason Andryuk <jandryuk@gmail.com> wrote ----
>
>  > Untested!
>  >
>  > This only really matters for flask, but all of xsm is updated.
>  >
>  > flask_domain_create() and flask_domain_alloc_security() are a strange
>  > pair.
>  >
>  > flask_domain_create() serves double duty.  It both assigns sid and
>  > self_sid values and checks if the calling domain has permission to
>  > create the target domain.  It also has special casing for handling dom0.
>  > Meanwhile flask_domain_alloc_security() assigns some special sids, but
>  > waits for others to be assigned in flask_domain_create.  This split
>  > seems to have come about so that the structures are allocated before
>  > calling flask_domain_create().  It also means flask_domain_create is
>  > called in the middle of domain_create.
>  >
>  > Re-arrange the two calls.  Let flask_domain_create just check if current
>  > has permission to create ssidref.  Then it can be moved out to do_domctl
>  > and gate entry into domain_create.  This avoids doing partial domain
>  > creation before the permission check.
>  >
>  > Have flask_domain_alloc_security() take a ssidref argument.  The ssidref
>  > was already permission checked earlier, so it can just be assigned.
>  > Then the self_sid can be calculated here as well rather than in
>  > flask_domain_create().
>  >
>  > The dom0 special casing is moved into flask_domain_alloc_security().
>  > Maybe this should be just a fall-through for the dom0 already created
>  > case.  This code may not be needed any longer.
>  >
>  > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
>  > ---

<snip>

>  > -static int flask_domain_alloc_security(struct domain *d)
>  > +static int flask_domain_alloc_security(struct domain *d, u32 ssidref)
>  >  {
>  >  struct domain_security_struct *dsec;
>  > +    static int dom0_created = 0;
>  > +    int rc;
>  >
>  >  dsec = xzalloc(struct domain_security_struct);
>  >  if ( !dsec )
>  > @@ -175,14 +177,24 @@ static int flask_domain_alloc_security(struct domain *d)
>  >  case DOMID_IO:
>  >  dsec->sid = SECINITSID_DOMIO;
>  >  break;
>  > +    case 0:
>  > +        if ( !dom0_created ) {
>  > +            dsec->sid = SECINITSID_DOM0;
>  > +            dom0_created = 1;
>  > +        } else {
>  > +            dsec->sid = SECINITSID_UNLABELED;
>  > +        }
>
> While the handling of this case is not wrong, I have to wonder if there is a better way to handle the dom0 creation case.

dom0_cfg.ssidref could be set to SECINITSID_DOM0.  I guess that would
need some xsm_ssid_dom0 wrapper.  Then maybe this logic can go away
and the default case used.

pv-shim doesn't necessarily use domid 0, so this may be broken there.
dom0_cfg.ssidref would fix that, I think.  But I'm not familiar with
pv-shim.

>  > +        break;
>  >  default:
>  > -        dsec->sid = SECINITSID_UNLABELED;
>  > +        dsec->sid = ssidref;
>  >  }
>  >
>  >  dsec->self_sid = dsec->sid;
>  > -    d->ssid = dsec;
>
> I don't think you meant to deleted that, without it domains will have no ssid assigned to them.

Yes, this should be retained.

Thanks for looking.

-Jason


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: XSM and the idle domain
  2020-10-26 13:37     ` Jason Andryuk
  2020-10-26 13:46       ` [RFC PATCH] xsm: Re-work domain_create and domain_alloc_security Jason Andryuk
@ 2020-10-26 16:43       ` Andrew Cooper
  1 sibling, 0 replies; 13+ messages in thread
From: Andrew Cooper @ 2020-10-26 16:43 UTC (permalink / raw)
  To: Jason Andryuk, Hongyan Xia
  Cc: xen-devel, Jan Beulich, Daniel De Graaf, Daniel Smith

On 26/10/2020 13:37, Jason Andryuk wrote:
> On Thu, Oct 22, 2020 at 1:01 PM Hongyan Xia <hx242@xen.org> wrote:
>> On Thu, 2020-10-22 at 13:51 +0100, Andrew Cooper wrote:
>>> On 21/10/2020 15:34, Hongyan Xia wrote:
>>>> The first question came up during ongoing work in LiveUpdate. After
>>>> an
>>>> LU, the next Xen needs to restore all domains. To do that, some
>>>> hypercalls need to be issued from the idle domain context and
>>>> apparently XSM does not like it.
>>> There is no such thing as issuing hypercalls from the idle domain
>>> (context or otherwise), because the idle domain does not have enough
>>> associated guest state for anything to make the requisite
>>> SYSCALL/INT80/VMCALL/VMMCALL invocation.
>>>
>>> I presume from this comment that what you mean is that you're calling
>>> the plain hypercall functions, context checks and everything, from
>>> the
>>> idle context?
>> Yep, the restore code just calls the hypercall functions from idle
>> context.
>>
>>> If so, this is buggy for more reasons than just XSM objecting to its
>>> calling context, and that XSM is merely the first thing to explode.
>>> Therefore, I don't think modifications to XSM are applicable to
>>> solving
>>> the problem.
>>>
>>> (Of course, this is all speculation because there's no concrete
>>> implementation to look at.)
>> Another explosion is the inability to create hypercall preemption,
>> which for now is disabled when the calling context is the idle domain.
>> Apart from XSM and preemption, the LU prototype works fine. We only
>> reuse a limited number of hypercall functions and are not trying to be
>> able to call all possible hypercalls from idle.
> I wonder if for domain_create, it wouldn't be better to move
> xsm_domain_create() out to the domctl (hypercall entry) and check it
> there.  That would side-step xsm in domain_create.  Flask would need
> to be modified for that.  I've an untested patch doing the
> rearranging, which I'll send as a follow up.
>
> What other hypercalls are you having issues with?  Those could also be
> refactored so the hypercall entry checks permissions, and the actual
> work is done in a directly callable function.
>
>> Having a dedicated domLU just like domB (or reusing domB) sounds like a
>> viable option. If the overhead can be made low enough then we won't
>> need to work around XSM and hypercall preemption.
>>
>> Although the question was whether XSM should interact with the idle
>> domain. With a good design LU should be able to sidestep this though.
> Circling back to the main topic, is the idle domain Xen, or is it
> distinct?

It "is" Xen, IMO.

> It runs in the context of Xen, so Xen isn't really in a
> place to enforce policy on itself.  Hongyan, as you said earlier,
> applying XSM is more of a debugging feature at that point than a
> security feature.  And as Jan pointed out, you can have problems if
> XSM prevents the hypervisor from performing an action it doesn't
> expect to fail.

We have several system DOMID's which are SELF, IO, XEN, COW, INVALID and
IDLE.

SELF is a magic constant expected to be used in most hypercalls on
oneself, to simplify callers.  INVALID is also a magic constant.

The others all have struct domain's allocated for them, and are concrete
objects as far as Xen is concerned.  IO/XEN/COW all exist for the
purpose of fitting into the memory/device ownership models, while IDLE
exists for the purpose of encapsulating the idle vcpus in the scheduling
model.

None of them have any kind of outside-Xen state associated with them. 
"scheduling" an idle vCPU runs the idle loop, but it is all code within
the hypervisor.

The problem here is that idle context is also used in certain "normal"
cases in Xen (startup, shutdown, possibly also for softirq/tasklet
context), all of which we (currently) expect not to be making hypercalls
from.

~Andrew


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-10-26 16:44 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-21 14:34 XSM and the idle domain Hongyan Xia
2020-10-21 16:21 ` Jason Andryuk
2020-10-22 11:29   ` Hongyan Xia
2020-10-22  1:23 ` Daniel P. Smith
2020-10-22  8:13   ` Jan Beulich
2020-10-22 13:03     ` Daniel Smith
2020-10-22 12:51 ` Andrew Cooper
2020-10-22 17:01   ` Hongyan Xia
2020-10-26 13:37     ` Jason Andryuk
2020-10-26 13:46       ` [RFC PATCH] xsm: Re-work domain_create and domain_alloc_security Jason Andryuk
2020-10-26 16:23         ` Daniel Smith
2020-10-26 16:39           ` Jason Andryuk
2020-10-26 16:43       ` XSM and the idle domain Andrew Cooper

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.