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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 14 May 2024, 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.

Same here


> 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...).

I realize 3 years have passed and it is a long time, but many
downstreams (including some which are widely used) don't rebase
regularly and we are still missing lots of tests from gitlab-ci. The
unfortunate result is that it can take years to realize there is a
breakage. We need more gitlab-ci (or OSSTest) tests.


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

I am convinced by Daniel's argument here:

https://marc.info/?l=xen-devel&m=171215093102694
https://marc.info/?l=xen-devel&m=171215073502479

I would ack Andrew's revert. If we decide to revert Andrew's revert, I
also think that we should make the alternative solution, whatever that
might be, a blocker for Xen 4.19.

My favorite alternative solition is Daniel's suggestion of adding a
check to the dummy XSM policy. I am not sure if this is the same thing
you mean with "change the XSM policy consistently".


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