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

Re: [PATCH v3 7/7] xsm: removing facade that XSM can be enabled/disabled

  • To: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 30 Aug 2021 15:34:40 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.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-SenderADCheck; bh=OydfQp5+YXHTSznbrPHkkqn3D7DR6PRQpVd4MLB/D6Y=; b=Fq9lK7yxF2jK8ewDMc1aqA+f47tHG5jFMu/bBOgfCfhxoOZ+6H8By0kIiFGKbVqyPqUGOLxvGUcBWFRkMs0b0eJp3Vb0MKpajAQKWHKZp2Xb79hZj0aSWhzSDNmwUw+Lvi0MzVJyVIyHsaOoXqGSX1K/dKz4lp/dpHvoOAfW99c6kXwsFvvPXlMmH5K6lcSz+dVzd/e3vcTN9x6/x9eTTW4beQrrDdQKHb6PbM2/hOsWLRdqMptfgswehyYVVJPYM17GCJ8HMyon1n933Tw9ympJKzPjpWZOKwVyjRV6CahOv1h9N3DMfjl+S1PU21Vx/tswABtblCO3w74nMG+RoA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=SIFWHAIylqMf6VFuBLrcN8QHCMdefUwBy5G/h9wTYCOu8smVO6zPHzk+u3S3Vu1E2AIBAVHHR1fOXMxIzfdimenQrkcpBa42FfgYMyCWZjILMm0Z1BmllvCcmO97TFZu+X7rVMhShcfgQhIm4rSeMnNlzlin4lKRJIWKByzUQOFBd4MwSdkuakPGGm207Nj8x5/T3fqx5FFv51XSv0WWUEteZlkIOsIO6php4O/nIeqSRFZ2e4qGWZY8OlwJycBkXCwmaH3ew1GGJCjeI+9a2628/eeULVQXR0ddmRsNc3QgvNGBjTmAOuIZogYMfwqxYE9AfDSEWPQqFWl0aBR/RQ==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 30 Aug 2021 13:34:56 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 30.08.2021 14:11, Daniel P. Smith wrote:
> On 8/26/21 5:37 AM, Jan Beulich wrote:
>> On 05.08.2021 16:06, Daniel P. Smith wrote:
>>>  config XSM_FLASK
>>> -   def_bool y
>>> -   prompt "FLux Advanced Security Kernel support"
>>> -   depends on XSM
>>> -   ---help---
>>> +   bool "FLux Advanced Security Kernel support"
>>> +   default n
>> I don't understand this change in default (and as an aside, a default
>> of "n" doesn't need spelling out): In the description you say "adjusted
>> CONFIG_XSM_SILO, CONFIG_XSM_FLASK, and the default module selection to
>> sensible defaults". If that's to describe this change, then I'm afraid
>> I don't see why defaulting to "n" is more sensible once the person
>> configuring Xen has chosen the configure XSM's (or XSM_CONFIGURABLE's)
>> sub-options. If that's unrelated to the change here, then I'm afraid
>> I'm missing justification altogether. (Same for SILO then.)
> When an individual selects to be able to be to configure/select
> alternative policy modules, they should be the ones to decide which
> one(s) should be enabled and not have presumptuous selections provided
> to them. IOW, the sensible default is to have no modules selected and
> allow the user to enable the one(s) they want.

Just FTR - I disagree. Defaults should be such that a majority would
not need to alter the respective settings.

>>> @@ -282,7 +275,7 @@ endchoice
>>>  config LATE_HWDOM
>>>     bool "Dedicated hardware domain"
>>>     default n
>>> -   depends on XSM && X86
>>> +   depends on XSM_FLASK && X86
>> This change is not mentioned or justified in the description. In fact
>> I think it is unrelated to the change here and hence would want breaking
>> out.
> Actually, if you read the help just below it specifically says to use
> this feature requires having an XSM policy (legacy wording for XSM Flask
> policy) to be able to do the proper fine grained delegation of the
> permissions/accesses necessary for a hardware domain to work. This
> should have been made XSM_FLASK when that KConfig option was added.
> Dropping the XSM KConfig option just exposed this oversight. Ack that I
> should have mentioned this in the commit message.

I'm getting the impression that you mistook "breaking out" for "dropping".
I can see the point of the change; it merely shouldn't be hidden in this
otherwise unrelated much larger change.

>>> -static inline int xsm_set_target (xsm_default_t def, struct domain *d, 
>>> struct domain *e)
>>> +static inline int xsm_set_target(xsm_default_t action, struct domain *d,
>>> +                                 struct domain *e)
>>>  {
>>> -    return alternative_call(xsm_ops.set_target, d, e);
>>> +    if ( xsm_ops.set_target )
>>> +        return alternative_call(xsm_ops.set_target, d, e);
>>> +
>>> +    return xsm_default_action(action, current->domain, NULL);
>>>  }
>> While benign because xsm_default_action() does nothing for XSM_HOOK, I
>> think there's an inconsistency here which rather wants correcting (in
>> a prereq patch): The default hook should have been passed consistent
>> arguments, no matter whether used because of !XSM or because of the
>> module in use left the hook unset.
>> Of course such anomalies are much easier to notice (outside of review
>> of patches introducing such) with you now placing both invocations
>> next to each other.
> This series assumes the logic is correct and is only focused on trying
> to make XSM more maintainable. I would be glad to consider looking at
> what the right security decisions should be in a subsequent patch set
> but will be consider out of scope for this patch set.

Well, I came to make these comments because these anomalies look to
stand in the way of producing a sufficiently small set of wrapper /
helper macros. But since you want to go back to an older version's
approach, the need to correct these as a (series of) prereq(s) may
indeed vanish.




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