Re: [PATCH v2 08/10] xsm: remove xsm_default_t from hook definitions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.07.2021 16:15, Andrew Cooper wrote:
> On 16/07/2021 08:23, Jan Beulich wrote:
>> On 12.07.2021 22:32, Daniel P. Smith wrote:
>>> The passing of an xsm_default_t at each of the xsm hook call sites
>>> served different functions depending on whether XSM was enabled or not.
>>> When XSM was not enabled it attempted to function as a link-time check
>>> that declared default action at the call site matched the default
>>> declared action for that hook in the dummy policy. When XSM was enabled,
>>> it would just drop the  parameter.
>>>
>>> The removal of these values is two fold. They are a redundancy that
>>> provides little context, especially when the value is XSM_OTHER.
>> For XSM_OTHER I may agree, but in general I find the call-site uses
>> helpful to know at least the rough level of intended restriction.
>> E.g. ...
>>
>>> --- a/xen/arch/x86/cpu/mcheck/mce.c
>>> +++ b/xen/arch/x86/cpu/mcheck/mce.c
>>> @@ -1376,7 +1376,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc)
>>>      struct xen_mc_msrinject *mc_msrinject;
>>>      struct xen_mc_mceinject *mc_mceinject;
>>>  
>>> -    ret = xsm_do_mca(XSM_PRIV);
>>> +    ret = xsm_do_mca();
>> ... to now understand what this enforces (or not) I have to go to
>> the actual implementation, even if I only want to know the trivial
>> dummy incarnation of it. This effectively extends the "provides
>> little context" from XSM_OTHER to all hooks.
> 
> The old scheme was even worse because it was only a plausible
> approximation for the !XSM case while being actively wrong for SILO and
> FLASK.

"Actively wrong" is perhaps going a little far? I don't think SILO
relaxes things anywhere over the !XSM case, and I also wouldn't
expect a Flask policy to commonly relax things. Of course these
simplistic categories can't reflect the full possible range with
an actual policy, but seeing XSM_PRIV in a call used to be a fair
hint that subsequent code isn't expected to be reachable for
unprivileged entities. No such hints would exist anymore following
this change, and its description didn't sound to me like this
aspect was taken into consideration. Yet it is a difference to me
whether such removal of information (no matter how precise it is)
is deliberate or an unintended / unnoticed / unexpected side effect.

> Furthermore, someone reading the code could be forgiven for thinking
> that XSM_HOOK was something other than dead code.

I certainly agree with this part.

Jan