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

Re: [XEN PATCH v2 2/5] xen: export get_free_port


  • To: Julien Grall <julien@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 27 Jan 2022 13:07:28 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=d6zYVxrnV4iqT4Z0mURDK/A+PhocD2Ui6NiNclR8aO8=; b=c5pU7IQOo+nt6JebVD4suXy2Xwk+Zv6kuTWQQyQsYPYlRbWuhhEMrcy+canQ1DK/Duoo8/6eq4ZjXEMJOd4rTeEAvaN7Jc04odJhbZ0M9duxdM5tPu5aSOjXD2rqdhlxzUqGh/A9YMcTknEn1RV34pSwJYgqLEESvLNLS7Hm6zjKkUeYADCPoiha8hudDur45l+GzJRNqQI7FPQhMFpkK81A7TZHtrMcp8BaEWybleqVlbwwz3sYoiLg1ZjNQmuTGEMWZ8RwAHtRroYSVM3xeBVKwieJVGdE3H6EbPY3MHXJgmt9jCv5zt3StnQXsrC9A3Wm2tHysdLsG1WhSGRUgQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=G29pQTsd+5VMmTa8p7QpZdngjpIhlyUB1Y7ETbj0EcBRaKZou44FthoY8Mxk6ELZlQ3EzZLe63SjhC0NjCWc6/MBOiywvk8yX1CWOc3CnusoXMu4d1gk09fhkqwr/RhqjKzhOSifzZMlzTW3tsYdePaZipfW+zsByTRm1F+VXsoDrzY0YOV/xOzzjPQOMQHVRUNfILDkCL3ZVd45v2QDM0tAM3IQolpQyut/5XBepUWoCEPHjtxTRBg/yXuA7skvU0YZDtx53q9mlZuVtwizljrihCapSR68n58yjdnYPjve5xo2/Y/oCaaxZ8noZedTvZ6sebivkfydIGOqp1E0Kg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx, jgross@xxxxxxxx, Bertrand.Marquis@xxxxxxx, Volodymyr_Babchuk@xxxxxxxx, Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • Delivery-date: Thu, 27 Jan 2022 12:07:44 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 27.01.2022 10:51, Julien Grall wrote:
> On 27/01/2022 01:50, Stefano Stabellini wrote:
>> On Wed, 26 Jan 2022, Julien Grall wrote:
>>> On 26/01/2022 07:30, Jan Beulich wrote:
>>>> On 26.01.2022 02:03, Stefano Stabellini wrote:
>>>>> Are you guys OK with something like this?
>>>>
>>>> With proper proof that this isn't going to regress anything else, maybe.
>>>
>>> That's why I sugested to also gate with system_state < SYS_STATE_boot so we
>>> reduce the potential regression (the use of hypercall should be limited at
>>> boot).
>>>
>>> FWIW, there was a thread [1] to discuss the same issue as Stefano is facing
>>> (but in the context of Live-Update).
>>>
>>>> But ...
>>>>
>>>>> --- a/xen/include/xsm/dummy.h
>>>>> +++ b/xen/include/xsm/dummy.h
>>>>> @@ -92,7 +92,9 @@ static always_inline int xsm_default_action(
>>>>>                return 0;
>>>>>            /* fall through */
>>>>>        case XSM_PRIV:
>>>>> -        if ( is_control_domain(src) )
>>>>> +        if ( is_control_domain(src) ||
>>>>> +             src->domain_id == DOMID_IDLE ||
>>>>> +             src->domain_id == DOMID_XEN )
>>>>>                return 0;
>>>>
>>>> ... my first question would be under what circumstances you might observe
>>>> DOMID_XEN here and hence why this check is there.
>>
>> For simplicity I'll answer all the points here.
>>
>> I added both DOMID_IDLE and DOMID_XEN because I thought it "made sense",
>> but there is no need for DOMID_XEN. We only need DOMID_IDLE. Also adding
>> a system_state <= SYS_STATE_boot is fine (but it needs to be <= instead
>> of <). The patch appended below works without issues.
>>
>> Alternatively, I am also quite happy with Jan's suggestion of passing an
>> extra parameter to evtchn_alloc_unbound, it could be:
>>
>> int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool disable_xsm);
>>
>> So that XSM is enabled by default.
>>
>> Adding the bool parameter to evtchn_alloc_unbound has the advantage of
>> having only a very minor impact.
> 
> We will likely need similar approach for other hypercalls in the future 
> if we need to call them from Xen context (e.g. when restoring domain 
> state during Live-Update).
> 
> So my preference would be to directly go with modifying the 
> xsm_default_action().

How would this help when a real XSM policy is in use? Already in SILO
mode I think you wouldn't get past the check, as the idle domain
doesn't satisfy silo_mode_dom_check()'s use of is_control_domain().
Actually I'm not even sure you'd get that far - I was under the
impression that the domain at other side of the channel may not even
be around at that time, in which case silo_evtchn_unbound() would
return -ESRCH.

Additionally relaxing things at a central place like this one comes
with the risk of relaxing too much. As said in reply to Stefano - if
this is to be done, proof will need to be provided that this doesn't
and won't permit operations which aren't supposed to be permitted. I
for one would much prefer relaxation on a case-by-case basis. That
said I'm afraid it hasn't become clear to me why the XSM check needs
bypassing here in the first place, and why this is acceptable from a
security standpoint.

>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -94,6 +94,8 @@ static always_inline int xsm_default_action(
>>       case XSM_PRIV:
>>           if ( is_control_domain(src) )
>>               return 0;
>> +        if ( system_state <= SYS_STATE_boot && src->domain_id == DOMID_IDLE 
>> )
> 
> I would surround this with unlikely and possibly prevent speculation (Jan?).

Unlikely - perhaps yes. Preventing speculation in principle also
yes, but not at the expense of adding a 2nd LFENCE (besides the one
in is_control_domain()). Yet open-coding is_control_domain() wouldn't
be very nice either. But as per above I hope anyway we're not going
to need to find a good solution here.

Jan




 


Rackspace

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