[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Daniel Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 15 May 2024 09:33:56 +0200
  • Autocrypt: addr=jbeulich@xxxxxxxx; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "committers@xxxxxxxxxxxxxx" <committers@xxxxxxxxxxxxxx>, Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>, Julien Grall <julien@xxxxxxx>
  • Delivery-date: Wed, 15 May 2024 07:34:20 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.