Unlike for XEN_DOMCTL_getdomaininfo, where the XSM check is intended to
cause the operation to fail, in the loop here it ought to merely
determine whether information for the domain at hand may be reported
back. Therefore if on the last iteration the hook results in denial,
this should not affect the sub-op's return value.

Fixes: d046f361dc93 ("Xen Security Modules: XSM")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The hook being able to deny access to data for certain domains means
that no caller can assume to have a system-wide picture when holding the
results.

Wouldn't it make sense to permit the function to merely "count" domains?
While racy in general (including in its present, "normal" mode of
operation), within a tool stack this could be used as long as creation
of new domains is suppressed between obtaining the count and then using
it.

In XEN_DOMCTL_getpageframeinfo2 said commit had introduced a 2nd such
issue, but luckily that sub-op and xsm_getpageframeinfo() are long gone.

--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
             if ( num_domains == op->u.getdomaininfolist.max_domains )
                 break;
 
-            ret = xsm_getdomaininfo(XSM_HOOK, d);
-            if ( ret )
+            if ( xsm_getdomaininfo(XSM_HOOK, d) )
                 continue;
 
             getdomaininfo(d, &info);

On 5/2/23 03:17, Jan Beulich wrote:
> Unlike for XEN_DOMCTL_getdomaininfo, where the XSM check is intended to
> cause the operation to fail, in the loop here it ought to merely
> determine whether information for the domain at hand may be reported
> back. Therefore if on the last iteration the hook results in denial,
> this should not affect the sub-op's return value.
> 
> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The hook being able to deny access to data for certain domains means
> that no caller can assume to have a system-wide picture when holding the
> results.
> 
> Wouldn't it make sense to permit the function to merely "count" domains?
> While racy in general (including in its present, "normal" mode of
> operation), within a tool stack this could be used as long as creation
> of new domains is suppressed between obtaining the count and then using
> it.
> 
> In XEN_DOMCTL_getpageframeinfo2 said commit had introduced a 2nd such
> issue, but luckily that sub-op and xsm_getpageframeinfo() are long gone.
> 

I understand there is a larger issue at play here but neutering the 
security control/XSM check is not the answer. This literally changes the 
way a FLASK policy that people currently have would be enforced, as well 
as contrary to how they understand the access control that it provides. 
Even though the code path does not fall under XSM maintainer, I would 
NACK this patch. IMHO, it is better to find a solution that does not 
abuse, misuse, or invalidate the purpose of the XSM calls.

On a side note, I am a little concern that only one person thought to 
include the XSM maintainer, or any of the XSM reviewers, onto a patch 
and the discussion around a patch that clearly relates to XSM for us to 
gauge the consequences of the patch. I am not assuming intentions here, 
only wanting to raise the concern.

So for what it is worth, NACK.

V/r,
Daniel P. Smith

On Tue, May 02, 2023 at 06:43:33AM -0400, Daniel P. Smith wrote:
> 
> 
> On 5/2/23 03:17, Jan Beulich wrote:
> > Unlike for XEN_DOMCTL_getdomaininfo, where the XSM check is intended to
> > cause the operation to fail, in the loop here it ought to merely
> > determine whether information for the domain at hand may be reported
> > back. Therefore if on the last iteration the hook results in denial,
> > this should not affect the sub-op's return value.
> > 
> > Fixes: d046f361dc93 ("Xen Security Modules: XSM")
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > The hook being able to deny access to data for certain domains means
> > that no caller can assume to have a system-wide picture when holding the
> > results.
> > 
> > Wouldn't it make sense to permit the function to merely "count" domains?
> > While racy in general (including in its present, "normal" mode of
> > operation), within a tool stack this could be used as long as creation
> > of new domains is suppressed between obtaining the count and then using
> > it.
> > 
> > In XEN_DOMCTL_getpageframeinfo2 said commit had introduced a 2nd such
> > issue, but luckily that sub-op and xsm_getpageframeinfo() are long gone.
> > 
> 
> I understand there is a larger issue at play here but neutering the security
> control/XSM check is not the answer. This literally changes the way a FLASK
> policy that people currently have would be enforced, as well as contrary to
> how they understand the access control that it provides. Even though the
> code path does not fall under XSM maintainer, I would NACK this patch. IMHO,
> it is better to find a solution that does not abuse, misuse, or invalidate
> the purpose of the XSM calls.
> 
> On a side note, I am a little concern that only one person thought to
> include the XSM maintainer, or any of the XSM reviewers, onto a patch and
> the discussion around a patch that clearly relates to XSM for us to gauge
> the consequences of the patch. I am not assuming intentions here, only
> wanting to raise the concern.
> 
> So for what it is worth, NACK.

I assume the NACK is to the remarks after the '---'?

The patch itself doesn't change the enforcement of the XSM checks,
just prevents returning an error when the information from the last
domain in the loop can not be fetched.

Am I missing something?

Roger.

On 5/2/23 07:00, Roger Pau Monné wrote:
> On Tue, May 02, 2023 at 06:43:33AM -0400, Daniel P. Smith wrote:
>>
>>
>> On 5/2/23 03:17, Jan Beulich wrote:
>>> Unlike for XEN_DOMCTL_getdomaininfo, where the XSM check is intended to
>>> cause the operation to fail, in the loop here it ought to merely
>>> determine whether information for the domain at hand may be reported
>>> back. Therefore if on the last iteration the hook results in denial,
>>> this should not affect the sub-op's return value.
>>>
>>> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> The hook being able to deny access to data for certain domains means
>>> that no caller can assume to have a system-wide picture when holding the
>>> results.
>>>
>>> Wouldn't it make sense to permit the function to merely "count" domains?
>>> While racy in general (including in its present, "normal" mode of
>>> operation), within a tool stack this could be used as long as creation
>>> of new domains is suppressed between obtaining the count and then using
>>> it.
>>>
>>> In XEN_DOMCTL_getpageframeinfo2 said commit had introduced a 2nd such
>>> issue, but luckily that sub-op and xsm_getpageframeinfo() are long gone.
>>>
>>
>> I understand there is a larger issue at play here but neutering the security
>> control/XSM check is not the answer. This literally changes the way a FLASK
>> policy that people currently have would be enforced, as well as contrary to
>> how they understand the access control that it provides. Even though the
>> code path does not fall under XSM maintainer, I would NACK this patch. IMHO,
>> it is better to find a solution that does not abuse, misuse, or invalidate
>> the purpose of the XSM calls.
>>
>> On a side note, I am a little concern that only one person thought to
>> include the XSM maintainer, or any of the XSM reviewers, onto a patch and
>> the discussion around a patch that clearly relates to XSM for us to gauge
>> the consequences of the patch. I am not assuming intentions here, only
>> wanting to raise the concern.
>>
>> So for what it is worth, NACK.
> 
> I assume the NACK is to the remarks after the '---'?
> 
> The patch itself doesn't change the enforcement of the XSM checks,
> just prevents returning an error when the information from the last
> domain in the loop can not be fetched.
> 
> Am I missing something?

Actually, I should have finished my first cup of tea and looked closer 
at the patch in the larger context instead of the description, as the 
two do not align. You are correct, and provided I am not wrong here, the 
change is a no-op formatting change that removes an intermediate 
variable. I do not see how directly checking the return in an if versus 
checking the return stored in a variable. Additionally, the claim is 
that this occurs when XSM is enabled, which is also incorrect. The only 
difference at this location in code between not having XSM enabled and 
having it enabled is that for the latter, xsm_getdomaininfo() is an 
in-lined version versus a function call. In either case, both will 
return 0 unless you are using FLASK and have a policy blocking the 
domain from making the call.

V/r,
Daniel P. Smith

On 02.05.2023 15:13, Daniel P. Smith wrote:
> On 5/2/23 07:00, Roger Pau Monné wrote:
>> On Tue, May 02, 2023 at 06:43:33AM -0400, Daniel P. Smith wrote:
>>> On 5/2/23 03:17, Jan Beulich wrote:
>>>> Unlike for XEN_DOMCTL_getdomaininfo, where the XSM check is intended to
>>>> cause the operation to fail, in the loop here it ought to merely
>>>> determine whether information for the domain at hand may be reported
>>>> back. Therefore if on the last iteration the hook results in denial,
>>>> this should not affect the sub-op's return value.
>>>>
>>>> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> The hook being able to deny access to data for certain domains means
>>>> that no caller can assume to have a system-wide picture when holding the
>>>> results.
>>>>
>>>> Wouldn't it make sense to permit the function to merely "count" domains?
>>>> While racy in general (including in its present, "normal" mode of
>>>> operation), within a tool stack this could be used as long as creation
>>>> of new domains is suppressed between obtaining the count and then using
>>>> it.
>>>>
>>>> In XEN_DOMCTL_getpageframeinfo2 said commit had introduced a 2nd such
>>>> issue, but luckily that sub-op and xsm_getpageframeinfo() are long gone.
>>>>
>>>
>>> I understand there is a larger issue at play here but neutering the security
>>> control/XSM check is not the answer. This literally changes the way a FLASK
>>> policy that people currently have would be enforced, as well as contrary to
>>> how they understand the access control that it provides. Even though the
>>> code path does not fall under XSM maintainer, I would NACK this patch. IMHO,
>>> it is better to find a solution that does not abuse, misuse, or invalidate
>>> the purpose of the XSM calls.
>>>
>>> On a side note, I am a little concern that only one person thought to
>>> include the XSM maintainer, or any of the XSM reviewers, onto a patch and
>>> the discussion around a patch that clearly relates to XSM for us to gauge
>>> the consequences of the patch. I am not assuming intentions here, only
>>> wanting to raise the concern.
>>>
>>> So for what it is worth, NACK.
>>
>> I assume the NACK is to the remarks after the '---'?
>>
>> The patch itself doesn't change the enforcement of the XSM checks,
>> just prevents returning an error when the information from the last
>> domain in the loop can not be fetched.
>>
>> Am I missing something?
> 
> Actually, I should have finished my first cup of tea and looked closer 
> at the patch in the larger context instead of the description, as the 
> two do not align. You are correct, and provided I am not wrong here, the 
> change is a no-op formatting change that removes an intermediate 
> variable. I do not see how directly checking the return in an if versus 
> checking the return stored in a variable. Additionally, the claim is 
> that this occurs when XSM is enabled, which is also incorrect. The only 
> difference at this location in code between not having XSM enabled and 
> having it enabled is that for the latter, xsm_getdomaininfo() is an 
> in-lined version versus a function call. In either case, both will 
> return 0 unless you are using FLASK and have a policy blocking the 
> domain from making the call.

While perhaps imprecise, "XSM enabled" typically is taken for "Flask
is in use". Then again, looking back, neither title nor description
say "XSM enabled". And it truly was the XSM hook which might have
caused the sub-op to wrongly be reported as failed (given, as you say,
a policy is in place which actually can cause failure from that hook).

Jan

On 02.05.2023 12:43, Daniel P. Smith wrote:
> On 5/2/23 03:17, Jan Beulich wrote:
>> Unlike for XEN_DOMCTL_getdomaininfo, where the XSM check is intended to
>> cause the operation to fail, in the loop here it ought to merely
>> determine whether information for the domain at hand may be reported
>> back. Therefore if on the last iteration the hook results in denial,
>> this should not affect the sub-op's return value.
>>
>> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> The hook being able to deny access to data for certain domains means
>> that no caller can assume to have a system-wide picture when holding the
>> results.
>>
>> Wouldn't it make sense to permit the function to merely "count" domains?
>> While racy in general (including in its present, "normal" mode of
>> operation), within a tool stack this could be used as long as creation
>> of new domains is suppressed between obtaining the count and then using
>> it.
>>
>> In XEN_DOMCTL_getpageframeinfo2 said commit had introduced a 2nd such
>> issue, but luckily that sub-op and xsm_getpageframeinfo() are long gone.
>>
> 
> I understand there is a larger issue at play here but neutering the 
> security control/XSM check is not the answer. This literally changes the 
> way a FLASK policy that people currently have would be enforced, as well 
> as contrary to how they understand the access control that it provides. 
> Even though the code path does not fall under XSM maintainer, I would 
> NACK this patch. IMHO, it is better to find a solution that does not 
> abuse, misuse, or invalidate the purpose of the XSM calls.
> 
> On a side note, I am a little concern that only one person thought to 
> include the XSM maintainer, or any of the XSM reviewers, onto a patch 
> and the discussion around a patch that clearly relates to XSM for us to 
> gauge the consequences of the patch. I am not assuming intentions here, 
> only wanting to raise the concern.

Well, yes, for the discussion items I could have remembered to include
you. The code change itself, otoh, doesn't require your ack, even if it
is the return value of an XSM function which was used wrongly here.

> So for what it is worth, NACK.

I'm puzzled: I hope you don't mean NACK to the patch (or effectively
Jürgen's identical one, which I had noticed only after sending mine).
Yet beyond that I don't see anything here which could be NACKed. I've
merely raised a couple of points for discussion.

Jan

On 5/2/23 06:59, Jan Beulich wrote:
> On 02.05.2023 12:43, Daniel P. Smith wrote:
>> On 5/2/23 03:17, Jan Beulich wrote:
>>> Unlike for XEN_DOMCTL_getdomaininfo, where the XSM check is intended to
>>> cause the operation to fail, in the loop here it ought to merely
>>> determine whether information for the domain at hand may be reported
>>> back. Therefore if on the last iteration the hook results in denial,
>>> this should not affect the sub-op's return value.
>>>
>>> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> The hook being able to deny access to data for certain domains means
>>> that no caller can assume to have a system-wide picture when holding the
>>> results.
>>>
>>> Wouldn't it make sense to permit the function to merely "count" domains?
>>> While racy in general (including in its present, "normal" mode of
>>> operation), within a tool stack this could be used as long as creation
>>> of new domains is suppressed between obtaining the count and then using
>>> it.
>>>
>>> In XEN_DOMCTL_getpageframeinfo2 said commit had introduced a 2nd such
>>> issue, but luckily that sub-op and xsm_getpageframeinfo() are long gone.
>>>
>>
>> I understand there is a larger issue at play here but neutering the
>> security control/XSM check is not the answer. This literally changes the
>> way a FLASK policy that people currently have would be enforced, as well
>> as contrary to how they understand the access control that it provides.
>> Even though the code path does not fall under XSM maintainer, I would
>> NACK this patch. IMHO, it is better to find a solution that does not
>> abuse, misuse, or invalidate the purpose of the XSM calls.
>>
>> On a side note, I am a little concern that only one person thought to
>> include the XSM maintainer, or any of the XSM reviewers, onto a patch
>> and the discussion around a patch that clearly relates to XSM for us to
>> gauge the consequences of the patch. I am not assuming intentions here,
>> only wanting to raise the concern.
> 
> Well, yes, for the discussion items I could have remembered to include
> you. The code change itself, otoh, doesn't require your ack, even if it
> is the return value of an XSM function which was used wrongly here.

I beg to disagree, not that you could have, but that you should have. 
This is now the second XSM issue, that I am aware of at least, that 
myself and the XSM reviewers have been left out of. How and where the 
XSM hooks are deployed are critical to how XSM function, regardless of 
how mundane the change may be. By your logic, as the XSM maintainer I 
can make changes to the XSM code that changes how the system behaves for 
x86 and claim you have no Ack/Nack authority since it is XSM code. These 
subsystems are symbiotic, and we owe each other the due respect to 
include to the other when these systems touch or influence each other.

>> So for what it is worth, NACK.
> 
> I'm puzzled: I hope you don't mean NACK to the patch (or effectively
> Jürgen's identical one, which I had noticed only after sending mine).
> Yet beyond that I don't see anything here which could be NACKed. I've
> merely raised a couple of points for discussion.

I will comment on Jurgen's patch.

On 02.05.2023 14:54, Daniel P. Smith wrote:
> On 5/2/23 06:59, Jan Beulich wrote:
>> On 02.05.2023 12:43, Daniel P. Smith wrote:
>>> On 5/2/23 03:17, Jan Beulich wrote:
>>>> Unlike for XEN_DOMCTL_getdomaininfo, where the XSM check is intended to
>>>> cause the operation to fail, in the loop here it ought to merely
>>>> determine whether information for the domain at hand may be reported
>>>> back. Therefore if on the last iteration the hook results in denial,
>>>> this should not affect the sub-op's return value.
>>>>
>>>> Fixes: d046f361dc93 ("Xen Security Modules: XSM")
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> The hook being able to deny access to data for certain domains means
>>>> that no caller can assume to have a system-wide picture when holding the
>>>> results.
>>>>
>>>> Wouldn't it make sense to permit the function to merely "count" domains?
>>>> While racy in general (including in its present, "normal" mode of
>>>> operation), within a tool stack this could be used as long as creation
>>>> of new domains is suppressed between obtaining the count and then using
>>>> it.
>>>>
>>>> In XEN_DOMCTL_getpageframeinfo2 said commit had introduced a 2nd such
>>>> issue, but luckily that sub-op and xsm_getpageframeinfo() are long gone.
>>>>
>>>
>>> I understand there is a larger issue at play here but neutering the
>>> security control/XSM check is not the answer. This literally changes the
>>> way a FLASK policy that people currently have would be enforced, as well
>>> as contrary to how they understand the access control that it provides.
>>> Even though the code path does not fall under XSM maintainer, I would
>>> NACK this patch. IMHO, it is better to find a solution that does not
>>> abuse, misuse, or invalidate the purpose of the XSM calls.
>>>
>>> On a side note, I am a little concern that only one person thought to
>>> include the XSM maintainer, or any of the XSM reviewers, onto a patch
>>> and the discussion around a patch that clearly relates to XSM for us to
>>> gauge the consequences of the patch. I am not assuming intentions here,
>>> only wanting to raise the concern.
>>
>> Well, yes, for the discussion items I could have remembered to include
>> you. The code change itself, otoh, doesn't require your ack, even if it
>> is the return value of an XSM function which was used wrongly here.
> 
> I beg to disagree, not that you could have, but that you should have. 
> This is now the second XSM issue, that I am aware of at least, that 
> myself and the XSM reviewers have been left out of. How and where the 
> XSM hooks are deployed are critical to how XSM function, regardless of 
> how mundane the change may be. By your logic, as the XSM maintainer I 
> can make changes to the XSM code that changes how the system behaves for 
> x86 and claim you have no Ack/Nack authority since it is XSM code. These 
> subsystems are symbiotic, and we owe each other the due respect to 
> include to the other when these systems touch or influence each other.

No, that's not a proper representation of "my logic". Everyone can comment
on any patch, and pending objections will prevent it from going in. Still
not everyone needs to be Cc-ed on every patch. If you want to get to see
ones you're not Cc-ed on, you'll need to be subscribed to the list, to
look at (and perhaps comment on) all the ones of interest to you.

Jan

On 02/05/2023 8:17 am, Jan Beulich wrote:
> The hook being able to deny access to data for certain domains means
> that no caller can assume to have a system-wide picture when holding the
> results.
>
> Wouldn't it make sense to permit the function to merely "count" domains?
> While racy in general (including in its present, "normal" mode of
> operation), within a tool stack this could be used as long as creation
> of new domains is suppressed between obtaining the count and then using
> it.

This would not be the first example of the XSM hooks being tantamount to
useless.  I doubt it will be the last either.

With the rest of Alejandro's series in place, all requests for a single
domid's worth of info use the domctl, and all requests for all domains
use the systctl.


As a result, we can retrofit some sanity and change the meaning of the
XSM hook here for the sysctl, to mean "can see a systemwide view" (or
not).  This moves the check out of the loop, and fixes the behaviour.

~Andrew

On Tue, May 02, 2023 at 10:27:39AM +0100, Andrew Cooper wrote:
> On 02/05/2023 8:17 am, Jan Beulich wrote:
> > The hook being able to deny access to data for certain domains means
> > that no caller can assume to have a system-wide picture when holding the
> > results.
> >
> > Wouldn't it make sense to permit the function to merely "count" domains?
> > While racy in general (including in its present, "normal" mode of
> > operation), within a tool stack this could be used as long as creation
> > of new domains is suppressed between obtaining the count and then using
> > it.
> 
> This would not be the first example of the XSM hooks being tantamount to
> useless.  I doubt it will be the last either.
> 
> With the rest of Alejandro's series in place, all requests for a single
> domid's worth of info use the domctl, and all requests for all domains
> use the systctl.
> 
> 
> As a result, we can retrofit some sanity and change the meaning of the
> XSM hook here for the sysctl, to mean "can see a systemwide view" (or
> not).  This moves the check out of the loop, and fixes the behaviour.

Don't we still need some kind of loop, as the current getdomaininfo()
XSM hook expects a domain parameter in order to check whether the
caller has permissions over it?

Or we plan to introduce a new hook that reports whether a caller has
permissions over all domains?

Thanks, Roger.

On 02.05.2023 11:33, Roger Pau Monné wrote:
> On Tue, May 02, 2023 at 10:27:39AM +0100, Andrew Cooper wrote:
>> On 02/05/2023 8:17 am, Jan Beulich wrote:
>>> The hook being able to deny access to data for certain domains means
>>> that no caller can assume to have a system-wide picture when holding the
>>> results.
>>>
>>> Wouldn't it make sense to permit the function to merely "count" domains?
>>> While racy in general (including in its present, "normal" mode of
>>> operation), within a tool stack this could be used as long as creation
>>> of new domains is suppressed between obtaining the count and then using
>>> it.
>>
>> This would not be the first example of the XSM hooks being tantamount to
>> useless.  I doubt it will be the last either.
>>
>> With the rest of Alejandro's series in place, all requests for a single
>> domid's worth of info use the domctl, and all requests for all domains
>> use the systctl.
>>
>>
>> As a result, we can retrofit some sanity and change the meaning of the
>> XSM hook here for the sysctl, to mean "can see a systemwide view" (or
>> not).  This moves the check out of the loop, and fixes the behaviour.
> 
> Don't we still need some kind of loop, as the current getdomaininfo()
> XSM hook expects a domain parameter in order to check whether the
> caller has permissions over it?
> 
> Or we plan to introduce a new hook that reports whether a caller has
> permissions over all domains?

I'd be inclined to make the existing hook recognize NULL as "global view".

Jan

On 02/05/2023 10:33 am, Roger Pau Monné wrote:
> On Tue, May 02, 2023 at 10:27:39AM +0100, Andrew Cooper wrote:
>> On 02/05/2023 8:17 am, Jan Beulich wrote:
>>> The hook being able to deny access to data for certain domains means
>>> that no caller can assume to have a system-wide picture when holding the
>>> results.
>>>
>>> Wouldn't it make sense to permit the function to merely "count" domains?
>>> While racy in general (including in its present, "normal" mode of
>>> operation), within a tool stack this could be used as long as creation
>>> of new domains is suppressed between obtaining the count and then using
>>> it.
>> This would not be the first example of the XSM hooks being tantamount to
>> useless.  I doubt it will be the last either.
>>
>> With the rest of Alejandro's series in place, all requests for a single
>> domid's worth of info use the domctl, and all requests for all domains
>> use the systctl.
>>
>>
>> As a result, we can retrofit some sanity and change the meaning of the
>> XSM hook here for the sysctl, to mean "can see a systemwide view" (or
>> not).  This moves the check out of the loop, and fixes the behaviour.
> Don't we still need some kind of loop, as the current getdomaininfo()
> XSM hook expects a domain parameter in order to check whether the
> caller has permissions over it?
>
> Or we plan to introduce a new hook that reports whether a caller has
> permissions over all domains?

New hook.

The current behaviour of skipping certain entries is fundamentally
broken, and needs not to stay.

~Andrew