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 <jbeul...@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
