Re: Xenstore domains and XS_RESTRICT

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Jan 18, 2017 at 01:42:01PM +0100, Juergen Gross wrote:
> 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?
> 

Yes, that's correct.

> 
> Juergen
> 

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