Re: [Xen-devel] Xenstore domains and XS_RESTRICT

[Juergen Gross <jgross@xxxxxxxx>]

On 18/01/17 13:39, George Dunlap wrote:
> On 18/01/17 12:37, Andrew Cooper wrote:
>> On 18/01/17 12:08, Juergen Gross wrote:
>>> On 18/01/17 12:39, Wei Liu wrote:
>>>> On Wed, Jan 18, 2017 at 12:21:48PM +0100, Juergen Gross wrote:
>>>>> On 18/01/17 12:03, Wei Liu wrote:
>>>>>> On Mon, Jan 16, 2017 at 05:47:15PM +0100, Juergen Gross wrote:
>>>>>>> On 07/12/16 08:44, Juergen Gross wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> today the XS_RESTRICT wire command of Xenstore is supported by
>>>>>>>> oxenstored only to drop the privilege of a connection to that of the
>>>>>>>> domid given as a parameter to the command.
>>>>>>>>
>>>>>>>> Using this mechanism with Xenstore running in a stubdom will lead to
>>>>>>>> problems as instead of only a dom0 process dropping its privileges
>>>>>>>> the privileges of dom0 will be dropped (all dom0 Xenstore requests
>>>>>>>> share the same connection).
>>>>>>>>
>>>>>>>> In order to solve the problem I suggest the following change to the
>>>>>>>> Xenstore wire protocol:
>>>>>>>>
>>>>>>>>  struct xsd_sockmsg
>>>>>>>>  {
>>>>>>>> -    uint32_t type;  /* XS_??? */
>>>>>>>> +    uint16_t type;  /* XS_??? */
>>>>>>>> +    uint16_t domid; /* Use privileges of this domain */
>>>>>>>>      uint32_t req_id;/* Request identifier, echoed in daemon's 
>>>>>>>> response.  */
>>>>>>>>      uint32_t tx_id; /* Transaction id (0 if not related to a
>>>>>>>> transaction). */
>>>>>>>>      uint32_t len;   /* Length of data following this. */
>>>>>>>>
>>>>>>>>      /* Generally followed by nul-terminated string(s). */
>>>>>>>>  };
>>>>>>>>
>>>>>>>> domid will normally be zero having the same effect as today.
>>>>>>>>
>>>>>>>> Using XS_RESTRICT via a socket connection will run as today by dropping
>>>>>>>> the privileges of that connection.
>>>>>>>>
>>>>>>>> Using XS_RESTRICT via the kernel (Xenstore domain case) will save the
>>>>>>>> domid given as parameter in the connection specific private kernel
>>>>>>>> structure. All future Xenstore commands of the connection will have
>>>>>>>> this domid set in xsd_sockmsg. The kernel will never forward the
>>>>>>>> XS_RESTRICT command to Xenstore.
>>>>>>>>
>>>>>>>> A domid other than 0 in xsd_sockmsg will be handled by Xenstore to use
>>>>>>>> the privileges of that domain. Specifying a domid in xsd_sockmsg is
>>>>>>>> allowed for privileged domain only, of course. XS_RESTRICT via a
>>>>>>>> non-socket connection will be rejected in all cases.
>>>>>>>>
>>>>>>>> The needed modifications for Xenstore and the kernel are rather small.
>>>>>>>> As there is currently no Xenstore domain available supporting
>>>>>>>> XS_RESTRICT there are no compatibility issues to expect.
>>>>>>>>
>>>>>>>> Thoughts?
>>>>>>> As I don't get any further constructive responses even after asking for
>>>>>>> them: would patches removing all XS_RESTRICT support be accepted?
>>>>>>>
>>>>>> We don't need to actually remove it, do we? If XS_RESTRICT is not 
>>>>>> supported by
>>>>>> xenstored, the client would get meaningful error code. A patch to
>>>>>> deprecate that command should be good enough, right?
>>>>> Uuh, no.
>>>>>
>>>>> oxenstored does support XS_RESTRICT. The longer it stays the better the
>>>>> chances someone is using it.
>>>>>
>>>> Right. That's what I'm getting at.
>>>>
>>>> As a developer I'm in favour of ripping XS_RESTRICT out completely, but
>>>> as a maintainer I'm a bit uncomfortable with that...
>>>>
>>>> If current users are happy with this limiting interface, let them use
>>>> it.  We just need to provide a better alternative for future users.
>>> I'm not sure it is a good decision to let them use XS_RESTRICT. It is
>>> an interface with weird consequences in some cases which are not
>>> visible until some rare use cases (like hot-plugging a qdisk) are
>>> effective.
>>>
>>>> And even if we want to eventually remove it, we should try our best
>>>> provide an upgrade path. In this particular case, I think whatever
>>>> scheme we agree on is going to be a natural upgrade path. We can choose
>>>> to either keep XS_RESTRICT or remove it after that.
>>> Today XS_RESTRICT is encapsulated by xs_restrict(). We could keep
>>> this function and let it return false always. This way XS_RESTRICT
>>> could be removed without breaking any current users as xs_restrict()
>>> is returning false with xenstored today.
>>
>> I don't think XS_RESTRICT is actually used by anyone.
>>
>> It was added to oxenstored in the dim and distant past, but nothing I
>> can find in XenServer uses it.  In particular, its intended usecase (for
>> deprivileging qemu) doesn't work because qemu currently needs access to
>> dom0 keys to work.
>>
>> I'd tentatively rip it out.  No point keeping unused broken
>> functionality around and getting in the way of fixing the problem properly.
> 
> I think I'd go with "Rip it out and if anyone complains, we can figure
> out what to do (including puting it back in)".

I'll post a patch to do this. In case someone is strictly against
deleting XS_RESTRICT he can still NAK the patch, right?


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel