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

[Jan Beulich <jbeulich@xxxxxxxx>]

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);
>>> +
>>> +    XSM_ASSERT_ACTION(XSM_HOOK);
>>> +    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.

Jan