[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 1/2] xsm: create idle domain privieged and demote after setup


  • To: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Thu, 21 Apr 2022 16:56:47 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=u1pxBJommfTjdXLPGqXVOjDmAqIo4pL05sOyBNfBBUA=; b=IjxFn5oTKgVIQTMAlwO7HQ3WPwpDHX7v74eFuAkWza3fwJAspiDEkdMegr1r0PAvTAL6Y8Ppp5WzwAiZ4joTWSaL3neQ66/TeapdoBdL7Brw0MploxLLc3tSYjVr/WOWMTRkFfqztETwaCqFujA+AolOmIF1lfYO2UFXxYx7f8Tn2dWiwW3z7nV+1EOxQOV83L72XJU8i+Fxi6e1Y07cju0aliNeoBuc9dLMm3KcZXNNhjMISfP9Lv7QLATisDEiLGRDWme22Q7v6CItxC+YCRlfvc8/JcqfLdUrIParBYhedog6Fwhno84VhPx+9L2MqubwaUVrlPO65sFdTYQlGg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=fpYNDA8loocN6V9sXs9GFFoWleQq8ZLvF9nu3mHx+yxNPrK4VW/W9RJ2wveReIvwhxgl0oHUnnDf42LvfjxdOwbI1eawEDsQU6lbe+kxSrdqafQ/15t2kbFaBnsRNptDBgyUX2eRI4XxQ1ooSNTYl1cGlniOG+/T9tbwiwU3tKllARYyveupgVvmoMRJ3s7SHoTVb3VCFUOtOpnZNsN+ECjab5AV68iiNP1We9AeVpy0SETIP/AbVKyQmIOfuD/luU2ev4gecWZ6CNpMFW3h571+G6QWlQWAZGRd+lFovWH9AQNOdeNtn3eBzSllyNp7R+pRW7hFr5Pm4gkXy48jAA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Wei Liu <wl@xxxxxxx>, scott.davis@xxxxxxxxxx, jandryuk@xxxxxxxxx, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Dario Faggioli <dfaggioli@xxxxxxxx>, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
  • Delivery-date: Thu, 21 Apr 2022 14:57:05 +0000
  • Ironport-data: A9a23:zOXOR63pul7P0X5m7/bD5ZJxkn2cJEfYwER7XKvMYLTBsI5bpzMHy TAdWWqCOfzZMTf9c9ogPYS1pkMFvpCGzNM2QANupC1hF35El5HIVI+TRqvS04J+DSFhoGZPt Zh2hgzodZhsJpPkjk7xdOCn9xGQ7InQLlbGILes1htZGEk1EE/NtTo5w7Rj2tIy2IDja++wk YiaT/P3aQfNNwFcagr424rbwP+4lK2v0N+wlgVWicFj5DcypVFMZH4sDfjZw0/DaptVBoaHq 9Prl9lVyI97EyAFUbtJmp6jGqEDryW70QKm0hK6UID66vROS7BbPg/W+5PwZG8O4whlkeydx /1iiqDgFER4Y5SRheM8XVp2AipGY5xvreqvzXiX6aR/zmXgWl61mbBCKR9zOocVvOFqHWtJ6 PoUbigXaQyOjP63x7T9TfRwgsMkL4/gO4Z3VnNIlGmFS6p5B82SBfyVu7e03x9p7ixKNezZa McDLyJmcTzLYgFVO0dRA5U79AutrieuLWYJ9w3PzUYxy3ri6wZpyLH0C8XYWPiFV/tfxEuah W2TqgwVBTlfbrRz0wGt/mq3g+7TnQvyQI8ICKCj7flunUGSwWoIThYRUDOTsfS/z0KzRd9bA 0gV4TY167g/8lSxSdvwVAH+p2SL1jYiXN5XH/w/+Ru64KPe6AaEBUAJVjdELtchsaceWjgCx lKP2dTzClRHq7aSVW7b+r6KrCiaIjQcN2sLb2kFSmMt4dDlrJsikxHnQdNqEarzhdrwcRnr2 CyDpiU6g7QVjOYI2r+98FSBhCijzrDLUwo06wP/Tm+jqARja+aNbYGy9ULS6/oGKY+DV0SAp 1ANgc3Y5+cLZbmPniGQROQGHJmy+u2IdjbbhDZHE5co+Dus/HqiVZtN+zw4L0BsWu4IdjPkb 1XakR9A759Uen2xZOl4ZJzZNigx5a3pFNCgWvWKaNNLO8J1bFXeo38oYlOM1WfwlkRqibs4J ZqQbcerCzAdFLhjyz21Aewa1NfH2xwD+I8afrijpzzP7FZUTCX9pWstWLdWUt0E0Q==
  • Ironport-hdrordr: A9a23:S72yt6poxKMCjo6CiKG9lBcaV5rbeYIsimQD101hICG9Evb0qy nOpoV96faQslwssR4b9uxoVJPvfZqYz+8X3WBzB8bHYOCFgguVxehZhOOP/9SjIVydygc078 xdmsNFebjN5DZB7PoT4GODYqodKNvsytHWuQ8JpU0dMz2DaMtbnnZE4h7wKDwReOHfb6BJbq Z14KB81kOdUEVSVOuXLF8fUdPOotXa/aiWHCLvV3YcmXGzZSrD0s+ALySl
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Thu, Apr 21, 2022 at 10:14:18AM -0400, Daniel P. Smith wrote:
> On 4/21/22 05:53, Roger Pau Monné wrote:
> > On Wed, Apr 20, 2022 at 06:28:33PM -0400, Daniel P. Smith wrote:
> >> There are now instances where internal hypervisor logic needs to make 
> >> resource
> >> allocation calls that are protectd 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 changes the idle
> >> domain to be created as a privileged domain under the default policy, 
> >> which is
> >> inherited by the SILO policy, and demoted before transitioning to running. 
> >> A
> >> new XSM hook, xsm_transition_running, is introduced to allow each XSM 
> >> policy
> >> type to demote the idle domain appropriately for that policy type.
> >>
> >> For flask a stub is added to ensure that flask policy system will function
> >> correctly with this patch until flask is extended with support for 
> >> starting the
> >> idle domain privileged and properly demoting it on the call to
> >> xsm_transtion_running.
> >>
> >> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> >> ---
> >>  xen/arch/arm/setup.c    |  6 ++++++
> >>  xen/arch/x86/setup.c    |  6 ++++++
> >>  xen/common/sched/core.c |  7 ++++++-
> >>  xen/include/xsm/dummy.h | 12 ++++++++++++
> >>  xen/include/xsm/xsm.h   |  6 ++++++
> >>  xen/xsm/dummy.c         |  1 +
> >>  xen/xsm/flask/hooks.c   | 15 +++++++++++++++
> >>  7 files changed, 52 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> >> index d5d0792ed4..763835aeb5 100644
> >> --- a/xen/arch/arm/setup.c
> >> +++ b/xen/arch/arm/setup.c
> >> @@ -1048,6 +1048,12 @@ void __init start_xen(unsigned long 
> >> boot_phys_offset,
> >>      /* Hide UART from DOM0 if we're using it */
> >>      serial_endboot();
> >>  
> >> +    xsm_transition_running();
> > 
> > Could we put depriv or dipriviledge somewhere here? 'transition' seem to
> > ambiguous IMO (but I'm not a native speaker).
> > 
> > xsm_{depriv,demote}_current();
> 
> Let me say this explanation is not to say no but to give context to the
> concerns. Forms of deprive/demote were considered though when
> considering the concept proposed was to change the security model where
> the hypervisor/idle domain were now explicitly being give a new security
> context, is_privileged and xenboot_t, under which setup is being run.
> This new xsm hook is to provide a transition point for the XSM policies
> to set what the running security context should be for the
> hypervisor/idle domain. The name xsm_transition_running() clearly
> denotes when/where this hook should be used, where as the name
> xsm_depriv_current() is more generic and another developer may attempt
> to use it in a manner it was not intended.

Hm, I see. I (wrongly) originally understood it was related to making
a transition in the running context, rather than the context being
changed to the running state.

Maybe xsm_{transition_,set_,}system_active() to better match the
system_state?

Albeit now that I understand it's purpose it doesn't feel so weird.

> >> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> >> index 19ab678181..22a619e260 100644
> >> --- a/xen/common/sched/core.c
> >> +++ b/xen/common/sched/core.c
> >> @@ -3021,7 +3021,12 @@ void __init scheduler_init(void)
> >>          sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
> >>      }
> >>  
> >> -    idle_domain = domain_create(DOMID_IDLE, NULL, 0);
> >> +    /*
> >> +     * idle dom is created privileged to ensure unrestricted access during
> >> +     * setup and will be demoted by xsm_transition_running when setup is
> >> +     * complete
> >> +     */
> >> +    idle_domain = domain_create(DOMID_IDLE, NULL, CDF_privileged);
> >>      BUG_ON(IS_ERR(idle_domain));
> >>      BUG_ON(nr_cpu_ids > ARRAY_SIZE(idle_vcpu));
> >>      idle_domain->vcpu = idle_vcpu;
> >> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> >> index 58afc1d589..b33f0ec672 100644
> >> --- a/xen/include/xsm/dummy.h
> >> +++ b/xen/include/xsm/dummy.h
> >> @@ -101,6 +101,18 @@ static always_inline int xsm_default_action(
> >>      }
> >>  }
> >>  
> >> +static XSM_INLINE void cf_check xsm_transition_running(void)
> >> +{
> >> +    struct domain *d = current->domain;
> >> +
> >> +    if ( d->domain_id != DOMID_IDLE )
> >> +        panic("xsm_transition_running should only be called by idle 
> >> domain\n");
> > 
> > Could you also add a check that d->is_privileged == true?
> 
> Are you thinking along the lines of,
> 
>     if ( (!d->is_privileged) || (d->domain_id != DOMID_IDLE)
>         panic("some message\n");
> 
> or is your concern more of,
> 
>     if ( !d->is_privileged )
>         return;
> 
> In my mind the former is legitimate because execution should only arrive
> here with current->domain being the idle domain and is_privileged set to
> true.

I was thinking about the former, maybe adding it as a separate
condition so you can print a specific panic message, or joined with
the other if the panic message can be adjusted to fit both conditions.

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.