Re: [Xen-devel] Xenstore domains and XS_RESTRICT

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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)".

 -George


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