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

["Oleksii K." <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2024-05-14 at 12:13 +0100, Julien Grall wrote:
> Hi,
> 
> (+ Oleksii as the release manager)
> 
> Chiming into the discussion as there seems there is disagreement.
> 
> On 14/05/2024 11:03, Jan Beulich wrote:
> > On 14.05.2024 11:51, Andrew Cooper wrote:
> > > On 14/05/2024 10:25 am, Jan Beulich wrote:
> > > > On 03.04.2024 08: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?
> > > > > 
> > > > > > 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).
> > > > > 
> > > > > >   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?
> > > > > 
> > > > > > 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 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".
> > > > You've never responded to this reply of mine, or its follow-up.
> > > > You also
> > > > didn't chime in on the discussion Daniel and I were having. I
> > > > consider my
> > > > objections unaddressed, and in fact I continue to consider the
> > > > change to
> > > > be wrong. Therefore it was inappropriate for you to commit it;
> > > > it needs
> > > > reverting asap. If you're not going to do so, I will.
> > > 
> > > You tried defending breaking a utility with "well it shouldn't
> > > exist then".
> > > 
> > > You don't have a leg to stand on, and two maintainers of relevant
> > > subsystems here just got tired of bullshit being presented in
> > > place of
> > > any credible argument for having done the change in the way you
> > > did.
> > 
> > Please can you finally get into the habit of not sending rude
> > replies?
> > 
> > > The correct response was "Sorry I broke things.  Lets revert this
> > > for
> > > now to unbreak, and I'll see about reworking it to not
> > > intentionally
> > > subvert Xen's security mechanism".
> > 
> > I'm sorry, but I didn't break things. I made things more consistent
> > with
> > the earlier change, as pointed out before: With your revert,
> > evtchn_status() is now (again) inconsistent with e.g.
> > evtchn_send(). If
> > you were serious about this being something that needs leaving to
> > XSM,
> > you'd have adjusted such further uses of consumer_is_xen() as well.
> > But
> > you aren't. You're merely insisting on lsevtchn needing to continue
> > to
> > work in a way it should never have worked, with a patch to improve
> > the
> > situation already pending.
> > 
> > Just to state a very basic principle here again: Xen-internal event
> > channels ought to either be fully under XSM control when it comes
> > to
> > domains attempting to access them (in whichever way), or they
> > should
> > truly be Xen-internal, with access uniformly prevented. To me the
> > former option simply makes very little sense.
> 
> I agree we need consistency on how we handle security policy event 
> channel. Although, I don't have a strong opinion on which way to go.
> 
> For the commit message, it is not entirely clear what "broke
> lseventch 
> in dom0" really mean. Is it lsevtchn would not stop or it will just
> not 
> display the event channel?
> 
> If the former, isn't a sign that the tool needs to be harden a bit
> more? 
> If the latter, then I would argue that consistency for the XSM policy
> is 
> more important than displaying the event channel for now (the patch
> was 
> also committed 3 years ago...).
> 
> So I would vote for a revert and, if desired, replacing with a patch 
> that would change the XSM policy consistently. Alternatively, the 
> consistency should be a blocker for Xen 4.19.
Sorry for the delayed response.

I am not deeply familiar with the technical details surrounding XSM,
but if I understand Daniel's point correctly, the original change
violates the access control framework. This suggests to me that the
revert should be merged.

However, I have a question: if we merge this revert, does it mean that
using channels a user ( domain ) will be able to get information about
certain events such as EVTCHNSTAT_unbound, EVTCHNSTAT_interdomain,
EVTCHNSTAT_pirq, EVTCHNSTAT_virq, and EVTCHNSTAT_IPI (based on the code
of lseventch.c)? Is this information really so critical that it cannot
be exposed for some time until a patch that changes the XSM policy
consistently is provided and merged?

If this information is indeed critical and should not be exposed, I
think we can consider Daniel's suggestion to add a check to the dummy
XSM policy as a solution.

~ Oleksii
> 
> > 
> > > As it stands, you're 2-1 outvoted, and wasted any sympathy I may
> > > have
> > > had for the principle of the change based on the absurdity of
> > > your
> > > arguments.
> > 
> > No, pending objections are pending objections. Daniel's responses
> > didn't
> > eliminate them.
> 
> Indeed, this is rule 4 of the check-in policy:
> 
> 4. There must be no "open" objections.
> 
> I don't view Jan's objections as unreasonable in particular for the 
> consistency part.
> 
> > As a separate aspect: I can't assume anymore that it is just
> > coincidence
> > that you taking such a controversial action is at a time when I'm
> > away.
> 
> Cheers,
>