On 14.05.2024 23:35, Stefano Stabellini wrote: > On Tue, 14 May 2024, Julien Grall wrote: >> On 14/05/2024 11:03, Jan Beulich wrote: >>> On 14.05.2024 11:51, Andrew Cooper wrote: >>>> 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
I particularly disagree with the "since it is access control and falls under the purview of XSM" in there, without addressing my point regarding Xen- internal resources. It is a fundamental hypervisor decision whether to leave access to Xen-internal resources to XSM control. If that decision ended up being "yes", then I agree XSM maintainers may ack a respective change. If that decision as "no", though, acking would purely fall to REST for code like what is being touched here. Just to further clarify: If it was "yes" above, other Xen-internal resources then also ought to be domain accessible based on XSM policy. I don't think that's the case e.g. for Xen-private memory. IOW I can't help the impression that both the patch and the ack were provided looking at just the one special case, driven by the (perceived) tool breakage (see below). > https://marc.info/?l=xen-devel&m=171215073502479 In there he said in particular: "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." Daniel, did you even look at the code when saying so? With the revert in place, lsevtchn is still going to fall on the nose when XSM denies access to a particular channel. I didn't think this needed calling out explicitly; the tool needs fixing. > 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". I don't think it would be, but I also don't know what exact check was thought about. I think I was quite clear about evtchn_send()'s similar code (there may be more). All of these want to behave the same: All involving XSM, or all not doing so. This is the kind of thing where I don't think any "majority" can trump technical aspects. If the verdict was "XSM", then yes, my original patch would have moved us in the wrong direction. But then a plain revert is insufficient, and the blaming in there also should have been done at least differently, if at all. Jan
