Re: [PATCH] Revert "evtchn: refuse EVTCHNOP_status for Xen-bound event channels"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.04.2024 15:27, Daniel P. Smith wrote:
> On 4/3/24 08:05, Jan Beulich wrote:
>> On 03.04.2024 13:10, Daniel P. Smith wrote:
>>> On 4/3/24 02:16, Jan Beulich wrote:
>>>> On 02.04.2024 19:06, Andrew Cooper wrote:
>>>>> The commit makes a claim without any kind of justification.
>>>>
>>>> Well, what does "have no business" leave open?
>>>
>>> Why does it not have any business? Why should a domain that creates an
>>> event channel not be able to inquire about its status?
>>
>> Event channels we talk about here are created via
>> alloc_unbound_xen_event_channel(). IOW it's not any domain creating them.
>> Once connected, the respective domain is of course fine to query its end
>> of the channel.
> 
> I would disagree, for instance alloc_unbound_xen_event_channel() is used 
> in response to XEN_DOMCTL_vuart_op:XEN_DOMCTL_VUART_OP_INIT and 
> XEN_DOMCTL_VM_EVENT_OP_PAGING:XEN_VM_EVENT_ENABLE, which are hypercalls 
> by a domain and not something initiated by the hypervisor.

Those ports, aiui, aren't supposed to be used by the caller for other
than connecting an inter-domain port to the other side.

>>>>> The claim is false, and the commit broke lsevtchn in dom0.
>>>>
>>>> Or alternatively lsevtchn was doing something that was never meant to work
>>>> (from Xen's perspective).
>>>
>>> Again, you have not said why this is a problem. What concern does it
>>> create? Does it open the door for access elevation, resource
>>> deprivation, or some other malicious behaviors?
>>
>> It exposes information that perhaps better wouldn't be exposed. Imo if
>> Xen owned resource state is of interest, it would want exposing via
>> hypfs.
> 
> You didn't answer why, just again expressed your opinion that it is not 
> better exposed.

I'm sorry, but "better wouldn't be exposed" includes the "why" part
already imo: Information should simply not be exposed unduly. For
every bit of exposed information, there ought to be a reason (and
then the right vehicle used for exposure).

>>>>>    It is also quite
>>>>> obvious from XSM_TARGET that it has broken device model stubdoms too.
>>>>
>>>> Why would that be "obvious"? What business would a stubdom have to look at
>>>> Xen's side of an evtchn?
>>>
>>> Again, you have not expressed why it shouldn't be able to do so.
>>
>> See above - not its resource, nor its guest's.
> 
> It is a resource provided to a domain that the domain can send/raise an 
> event to and a backing domain that can bind to it, ie. the two 
> parameters that must be passed to the allocation call.

I don't think so: As per above (my understanding may be wrong), it's only
the other side of the connection which is available for use by a domain.
Over night I was pretty close to admitting a mistake there, but upon re-
checking of the sources I could only find this view of mine supported.
Which doesn't mean I'm viewing things correctly; please point out my
mistake if there is any.

>>>>> Whether to return information about a xen-owned evtchn is a matter of 
>>>>> policy,
>>>>> and it's not acceptable to short circuit the XSM on the matter.
>>>>
>>>> I can certainly accept this as one possible view point. As in so many cases
>>>> I'm afraid I dislike you putting it as if it was the only possible one.
>>>
>>> In fact, this commit is in violation of the XSM. It hard-codes a
>>> resource access check outside XSM, thus breaking the fine-grained access
>>> control of FLASK.
>>
>> Perhaps; see below and see the question raised in the subsequent reply
>> to the patch.
>>
>>>> In summary: The supposed justification you claim is missing in the original
>>>> change is imo also missing here then: What business would any entity in the
>>>> system have to look at Xen's side of an event channel? Back at the time, 3
>>>> people agreed that it's "none".
>>>
>>> As stated, you provided no reason or justification for "has no business"
>>> and by face value is an opinion that a few people agreed with. As for
>>> why, there could be a myriad number of reasons a domain may want to
>>> check the status of an interface it has with the hypervisor. From just
>>> logging its state for debug to throttling attempts at sending an event.
>>> So why, from a security/access control decision, does this access have
>>> to absolutely blocked, even from FLASK?
>>
>> I didn't say it absolutely needs to be blocked. I'm okay to become
>> convinced otherwise. But in the description complaining about lack of
>> reasons in the 3-4 year old change, just to then again not provide any
>> reasons looks "interesting" to me. (And no, just to take that example,
>> lsevtchn not working anymore on such channels is not on its own a
>> reason. As indicated, it may well be that conceptually it was never
>> supposed to be able to have access to this information. The latest not
>> anymore when hypfs was introduced.)
> 
> This broke an existing behavior, whether that behavior is correct can 
> always be questioned, does not justify leaving an incorrect 
> implementation. And it is incorrect because as again you have not 
> articulated why the lsevtchn behavior is wrong and thus whether this is 
> the valid corrective action.

Again - if lsevtchn is supposed to be able to access Xen-internal
resources, _that_ is what needs justifying. Otherwise my take is that
it is supposed to only access domain resources.

Jan