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

Re: [PATCH v2 06/10] xsm: enable xsm to always be included


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
  • Date: Sun, 25 Jul 2021 16:47:10 -0400
  • Arc-authentication-results: i=1; mx.zohomail.com; dkim=pass header.i=apertussolutions.com; spf=pass smtp.mailfrom=dpsmith@xxxxxxxxxxxxxxxxxxxx; dmarc=pass header.from=<dpsmith@xxxxxxxxxxxxxxxxxxxx>
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1627246033; h=Content-Type:Content-Transfer-Encoding:Cc:Date:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:To; bh=CKrBufg6MB5+iFrVnARAdi22f0rXilfH/nP4BlPmyM8=; b=PQ2KEvCu48pHUfoyFXYJYL1H6e7iNOOshTK6TMm9bJ9/0Z8RFue1dXFcs+E9jbYvtqkGzabStlg73vIciDq/lbwLmIJDdYMadBStWAu1sewuMdzmVF7c7BE9KHsCR5mRGfk8fHd2OfSXSFYmeepzibTsBkSqMCXhyiG5HDNKdcc=
  • Arc-seal: i=1; a=rsa-sha256; t=1627246033; cv=none; d=zohomail.com; s=zohoarc; b=F6zz9fTE4sYUzMPwv1UTZhN2/DQbcLGTilYX6soEjmoMkYF30miX8GQtr6czJFDgkm/qjIMjAPmOVSjyDfEY7Efo+y/Md0q4D3Ht6B7ZNMFEgAhq6U6lqWw3MIgGUUhOIt9lQtujYIy5muAYZtjsrUo52JXyxh/fDYT1GOn2MDw=
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Sun, 25 Jul 2021 20:47:41 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 7/19/21 6:24 AM, Jan Beulich wrote:
> On 12.07.2021 22:32, Daniel P. Smith wrote:
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -200,23 +200,20 @@ config XENOPROF
>>  
>>        If unsure, say Y.
>>  
>> -config XSM
>> -    bool "Xen Security Modules support"
>> -    default ARM
>> -    ---help---
>> -      Enables the security framework known as Xen Security Modules which
>> -      allows administrators fine-grained control over a Xen domain and
>> -      its capabilities by defining permissible interactions between domains,
>> -      the hypervisor itself, and related resources such as memory and
>> -      devices.
>> +menu "Xen Security Modules"
> 
> I remain unconvinced of the removal of this top level option. I don't
> view my concern on code size / performance as "unreasonable" (as Andrew
> did call it) when comparing with the current !XSM configuration, and I
> also remain to be convinced of people who had to simply answer 'n' to
> the XSM kconfig prompt being happy to make an informed decision for all
> the (previously sub-)options that they will now be prompted for. As
> said before - it is one thing to re-work what exactly !XSM means
> internally (and the conversion away from inline functions may very well
> be a win; we simply don't know without you showing e.g. bloat-o-meter
> like data). It is another to require in-depth knowledge to configure
> Xen that previously wasn't required.

For me personally a concern about code size / performance itself is not
"unreasonable" but I would say it is a bit disingenuous to use it to
defend a position that the security framework should be special
conditioned and kept convoluted considering, 1) other subsystems, e.g.
iommu, do not appear to me to have the same subjugation requiring a
special case of one hook set over another, 2) the architecture (Arm)
which IMHO has the greatest concern over code size / performance is also
the architecture with the only security supported configuration that
requires an XSM enabled configuration, 3) this approach effectively
requires the maintaining of two sets of hook handlers which increases
work and risk for vulnerability introduction, and 4) based on the
discussions at the Developers Summit, no one seemed to be aware that the
only logical difference between !XSM and XSM was the invocation
mechanism, inline vs call-site, let alone that XSM_HOOK represented no
control check.

To bring context to the discussion, after applying the clean up patches
(everything up to the patch dropping !XSM) of the patch set, the
bloat-o-meter shows a 0.18% increase going from !XSM to XSM (without
SILO and Flask). IMHO this increase does not justify keeping the
convoluted gyrations being done to swap in an optimized security hook
when no other security module is enabled. In fact we should be working
to make the security framework clear and concise. I am all for
maximizing performance while doing so but the end goal is for it to be
clear and concise so that it can be reasoned about by everybody.

As to your position that this increases configuration complexity, I
would have to strongly disagree. I have worked to ensure no new
configuration steps are necessary. The default config will only have XSM
and the default/dummy policy enabled unless on Arm which will enable
SILO and make it the default policy. Prior to this if XSM was enabled,
the default policy was forced to Flask. While I am an advocate for
Flask, I do not agree this is a reasonable configuration. It now works
such that,
    - if you enable only SILO, it is set as the default
    - if you enable only Flask, it is set as the default
    - if you enable both SILO & Flask, it uses SILO as the default
Which basically works such that if one selects a policy, then it assumes
that policy is desired to be used, and when more than one is selected it
will default to the one that functions most like classic Dom0 model.
IMHO this is much more intuitive. Now one alternative I am thinking
about that might be a bit of a compromise is to move the default policy
selection up to the same level as XSM menu. That would make it so one
could immediately see what the default policy is but must go into the
XSM menu if they want to alter what policy modules are available.

> Keeping the top-level option would then also make it unnecessary to
> alter some of the (prior sub-)options' defaults.

I would beg to differ, it now makes the builder have to explicitly enter
that sub-option if they desire to enable an alternate security model,
e.g. have explicit intent instead of having the potential for accidental
selection.

> As to Andrew previously having said
> 
> "There is no such thing as !XSM even in staging right now.
> 
>  All over Xen, we have calls to xsm_*() functions which, even in the !XSM
>  case, contain a non-trivial security policy."
> 
> Yes, this is one possible viewpoint, which I simply do not share. I view
> the xsm_*() calls in the !XSM case as simple surrogates, not anything that
> deserves the name "module". This is actually supported by the help text
> of the XSM Kconfig option saying "which allows administrators fine-grained
> control": There's nothing fine-grained with what currently is !XSM.

IMHO that is a very unfortunate viewpoint. An xsm_*() call is an access
control check point where a decision needs to be made on whether an
access to a resource should be allowed. In a kernel with a monolithic
access control there would still be an implicit or an explicit access
check necessary at this location. What XSM does is makes all the access
check locations to be explicit checks where the decision can be handled
by different access control frameworks to be plugged in to implement the
security architecture that the implementer requires.

>> -      If unsure, say N.
>> +config XSM_EVTCHN_LABELING
>> +    bool "Enables security labeling of event channels"
> 
> Does this really need to have a prompt, instead of simply being
> selected by the module(s) needing it? IOW what do users gain when they
> answer y here but n for XSM_FLASK?
> 
> Furthermore, if the prompt is to remain, then I'll have to again
> raise the question of ordering of options: This way, via e.g. the
> "syncconfig" or "oldconfig" targets, I'd be asked for the setting here
> when, if I'd then also set XSM_FLASK=y, the question was in vein - the
> option will be set to y anyway.
> 
>> +    default n
> 
> May I ask to omit "default n" lines. I'm unaware of anything good they
> do.

It has been dropped

>> @@ -265,24 +262,26 @@ config XSM_SILO
>>        If unsure, say Y.
>>  
>>  choice
>> -    prompt "Default XSM implementation"
>> -    depends on XSM
>> +    prompt "Default XSM module"
>>      default XSM_SILO_DEFAULT if XSM_SILO && ARM
>>      default XSM_FLASK_DEFAULT if XSM_FLASK
>>      default XSM_SILO_DEFAULT if XSM_SILO
>>      default XSM_DUMMY_DEFAULT
>>      config XSM_DUMMY_DEFAULT
>> -            bool "Match non-XSM behavior"
>> +            bool "Classic Dom0 behavior"
>>      config XSM_FLASK_DEFAULT
>>              bool "FLux Advanced Security Kernel" if XSM_FLASK
>>      config XSM_SILO_DEFAULT
>>              bool "SILO" if XSM_SILO
>> +
>>  endchoice
> 
> Nit: I see only two consistent formatting options here: Either there is
> a blank line ahead of "endchoice" and after "choice", or there are none
> in both places. I don't mind which one it is, but I do mind
> inconsistencies getting introduced.

ack

v/r,
dps




 


Rackspace

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