Re: [PATCH 2/4] xsm/silo: Support hwdom/control domains

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.06.2025 18:56, Jason Andryuk wrote:
> On 2025-06-12 03:52, Jan Beulich wrote:
>> On 11.06.2025 06:20, Jason Andryuk wrote:
>>> On 2025-06-11 09:17, Jan Beulich wrote:
>>>> On 11.06.2025 00:57, Jason Andryuk wrote:
>>>>> --- a/xen/xsm/silo.c
>>>>> +++ b/xen/xsm/silo.c
>>>>> @@ -20,6 +20,12 @@
>>>>>    #define XSM_NO_WRAPPERS
>>>>>    #include <xsm/dummy.h>
>>>>>    
>>>>> +static bool is_priv_domain(const struct domain *d)
>>>>> +{
>>>>> +    return is_xenstore_domain(d) || is_hardware_domain(d) ||
>>>>> +           is_control_domain(d);
>>>>> +}
>>>>
>>>> This construct expands to two evaluate_nospec(), which likely isn't
>>>> wanted. Some open-coding may be pretty much unavoidable here.
>>>
>>> Thanks, yes, good point.
>>>
>>>> (I'm
>>>> surprised it's not three, i.e. I find it odd that is_xenstore_domain()
>>>> doesn't also use that guard.)
>>>
>>> It looks okay to me.  There were only 2 uses until I added a 3rd in the
>>> dom0less code.  The XSM check has evaluate_nospec() and the other 2 uses
>>> aren't security critical - Setting a domain info flag, and __init code
>>> for dom0less.  Maybe moving the evaluate_nospec() would be safer in case
>>> use grows in the future, but it looks okay to me today.
>>
>> When some of the hardening was first introduced, actual use sites were
>> indeed taken into account. That wasn't quite right though, I think. Any
>> such construct ought to be safe to use anywhere. For uses with clearly
>> no concerns towards speculative abuse, a 2nd lightweight form of such
>> constructs should then exist, imo. As to your use of "security critical":
>> I'm not convinced you what mean is covering the potential of speculative
>> abuse of involved code paths.
> 
> I can't parse this last sentence, and I think it's your main point.

Oh, sorry - the "you" and "what" ought to have swapped places.

> XSM -> don't speculate around permission checks.  That's what I meant by 
> "security critical".
> 
> The __init code is inaccessible to users, so it doesn't matter.
> 
>          if ( is_xenstore_domain(d) )
>              continue;
> 
> getdomaininfo sets a flag, so I don't see this making a security 
> difference.  It's not controlling loads or code paths.

Right, but this is what I said should imo not have been done: Make a
predicate speculation-safe (or not) based on its present uses. It's
imo more likely than not that a new use being added won't result in
the predicate being looked at, re-considering its safety for the new
use.

And indeed there's a 3rd use, in xsm_default_action():

    case XSM_XS_PRIV:
        if ( action == XSM_XS_PRIV &&
             evaluate_nospec(is_xenstore_domain(src)) )
            return 0;
        fallthrough;

It should not have been necessary to open-code the speculation safety
here, just like such isn't required a few lines later:

    case XSM_PRIV:
        if ( is_control_domain(src) )
            return 0;
        return -EPERM;

I am, btw, also not convinced the uses of evaluate_nospec() are fully
correct here, in that they apply to only part of the if() conditions.
For "action == XSM_XS_PRIV" it's okay as long as
- the function is indeed inlined, and
- the function argument is compile-time constant.
For "target" the same applies, but there is more room there for the
latter of the constraints to not be met. The argument in favor of
the present arrangements likely was that our main concern here is
with the "success" paths. Yet such argumentation would again be
dependent upon all call sites fitting the assumption that on the
"failure" paths there would be nothing critical that follows.

Jan