[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: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxx>, Daniel Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 16 May 2024 08:41:41 +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: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 16 May 2024 06:42:02 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 14.05.2024 11:25, 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.

For the record: With Andrew having clarified that in his revert's
description:

"The claim is false; it broke lsevtchn in dom0, a debugging utility which
 absolutely does care about all of the domain's event channels."

"all of the domain's event channels" means to include event channels Xen
uses for its own, and merely puts in the domain's table for ease of
handling, I've agreed that - in the absence of any better debugging
means - the revert may stay in place. For context, my prior understanding
of the issue was that lsevtchn simply stops probing further channels when
getting back any kind of error from EVTCHNOP_status (which I continue to
think wants addressing there, by a future version of a patch that was
already sent). However, there are follow-on concerns I have:

1) In the discussion George claimed that exposing status information in
an uncontrolled manner is okay. I'm afraid I have to disagree, seeing
how a similar assumption by CPU designers has led to a flood of
vulnerabilities over the last 6+ years. Information exposure imo is never
okay, unless it can be _proven_ that absolutely nothing "useful" can be
inferred from it. (I'm having difficulty seeing how such a proof might
look like.)

2) Me pointing out that the XSM hook might similarly get in the way of
debugging, Andrew suggested that this is not an issue because any sensible
XSM policy used in such an environment would grant sufficient privilege to
Dom0. Yet that then still doesn't cover why DomU-s also can obtain status
for Xen-internal event channels. The debugging argument then becomes weak,
as in that case the XSM hook is possibly going to get in the way.

3) In the discussion Andrew further gave the impression that evtchn_send()
had no XSM check. Yet it has; the difference to evtchn_status() is that
the latter uses XSM_TARGET while the former uses XSM_HOOK. (Much like
evtchn_status() may indeed be useful for debugging, evtchn_send() may be
similarly useful to allow getting a stuck channel unstuck.)

In summary I continue to think that an outright revert was inappropriate.
DomU-s should continue to be denied status information on Xen-internal
event channels, unconditionally and independent of whether dummy, silo, or
Flask is in use.

Plus, as stated before, evtchn_send() would better remain in sync in this
regard with evtchn_status(). The situation is less clear for evtchn_close()
and evtchn_bind_vcpu(): Those indeed have no XSM checks while they do deny
operation on Xen-internal channels. It is likely more far fetched to
assume permitting them for debugging purposes might in fact help in rare
situations. Yet it may still be a matter of consistency to keep them in
sync with the other two. (Note that there's also evtchn_usable(), which
might then also need tweaking itself or at the use sites.)

FTR, it is going to be only then that I would consider the cumulative
result as eligible for backporting. For this purpose, at the risk of
being flamed again, it might still be easier to revert the revert and then
put in place a change meeting the above criteria. That could then be taken
for backport as is.

Jan



 


Rackspace

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