| Message ID | 1508506702-17704-2-git-send-email-ian.jackson@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 20 Oct 2017, Ian Jackson wrote: > And insist that it works. > > Drop individual use of xendevicemodel_restrict and > xenforeignmemory_restrict. These are not actually effective in this > version of qemu, because qemu has a large number of fds open onto > various Xen control devices. > > The restriction arrangements are still not right, because the > restriction needs to be done very late - after qemu has opened all of > its control fds. > > xentoolcore_restrict_all and xentoolcore.h are available in Xen 4.10 > and later, only. Provide a compatibility stub. And drop the > compatibility stubs for the old functions. > > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> > Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> > --- > v2: Modify the compatibility code, too. > Bump this patch ahead of "defer call to xen_restrict until running" > Retain call to xentoolcore_restrict_all > --- > include/hw/xen/xen_common.h | 46 +++++++++++---------------------------------- > 1 file changed, 11 insertions(+), 35 deletions(-) > > diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h > index 86c7f26..3f44a63 100644 > --- a/include/hw/xen/xen_common.h > +++ b/include/hw/xen/xen_common.h > @@ -91,6 +91,16 @@ static inline void *xenforeignmemory_map2(xenforeignmemory_handle *h, > return xenforeignmemory_map(h, dom, prot, pages, arr, err); > } > > +static inline int xentoolcore_restrict_all(domid_t domid) > +{ > + errno = ENOTTY; > + return -1; Wait, if the compat stub returns error, and this patch removed the code to check for ENOTTY, doesn't it prevent any QEMU compiled against older Xen from working? Or am I missing something? > +} > + > +#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */ > + > +#include <xentoolcore.h> > + > #endif > > #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900 > @@ -218,20 +228,6 @@ static inline int xendevicemodel_set_mem_type( > return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr); > } > > -static inline int xendevicemodel_restrict( > - xendevicemodel_handle *dmod, domid_t domid) > -{ > - errno = ENOTTY; > - return -1; > -} > - > -static inline int xenforeignmemory_restrict( > - xenforeignmemory_handle *fmem, domid_t domid) > -{ > - errno = ENOTTY; > - return -1; > -} > - > #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */ > > #undef XC_WANT_COMPAT_DEVICEMODEL_API > @@ -290,28 +286,8 @@ static inline int xen_modified_memory(domid_t domid, uint64_t first_pfn, > static inline int xen_restrict(domid_t domid) > { > int rc; > - > - /* Attempt to restrict devicemodel operations */ > - rc = xendevicemodel_restrict(xen_dmod, domid); > + rc = xentoolcore_restrict_all(domid); > trace_xen_domid_restrict(rc ? errno : 0); > - > - if (rc < 0) { > - /* > - * If errno is ENOTTY then restriction is not implemented so > - * there's no point in trying to restrict other types of > - * operation, but it should not be treated as a failure. > - */ > - if (errno == ENOTTY) { > - return 0; > - } > - > - return rc; > - } > - > - /* Restrict foreignmemory operations */ > - rc = xenforeignmemory_restrict(xen_fmem, domid); > - trace_xen_domid_restrict(rc ? errno : 0); > - > return rc; > } > > -- > 2.1.4 >
Stefano Stabellini writes ("Re: [PATCH v5.1 2/8] xen: restrict: use xentoolcore_restrict_all"):
> On Fri, 20 Oct 2017, Ian Jackson wrote:
...
> > Drop individual use of xendevicemodel_restrict and
> > xenforeignmemory_restrict. These are not actually effective in this
> > version of qemu, because qemu has a large number of fds open onto
> > various Xen control devices.
...
> Wait, if the compat stub returns error, and this patch removed the code
> to check for ENOTTY, doesn't it prevent any QEMU compiled against older
> Xen from working?
>
> Or am I missing something?
You are right, but this is intended. The paragraph I quote in the
commit message above is intended to explain.
That is: without xentoolcore_restrict_all, -xen-domid-restrict is a
booby-trap. It does not actually prevent a compromised qemu from
doing anything. So there is no reason to pass it in such a
configuration. If you do pass it it is better for the domain startup
to fail, than for it to carry on without the restriction.
The only reason I am not saying someone should be issuing an advisory
is that this feature was never supported by any of the Xen toolstacks.
Thanks,
Ian.
On Fri, 27 Oct 2017, Ian Jackson wrote: > Stefano Stabellini writes ("Re: [PATCH v5.1 2/8] xen: restrict: use xentoolcore_restrict_all"): > > On Fri, 20 Oct 2017, Ian Jackson wrote: > ... > > > Drop individual use of xendevicemodel_restrict and > > > xenforeignmemory_restrict. These are not actually effective in this > > > version of qemu, because qemu has a large number of fds open onto > > > various Xen control devices. > ... > > Wait, if the compat stub returns error, and this patch removed the code > > to check for ENOTTY, doesn't it prevent any QEMU compiled against older > > Xen from working? > > > > Or am I missing something? > > You are right, but this is intended. The paragraph I quote in the > commit message above is intended to explain. > > That is: without xentoolcore_restrict_all, -xen-domid-restrict is a > booby-trap. It does not actually prevent a compromised qemu from > doing anything. So there is no reason to pass it in such a > configuration. If you do pass it it is better for the domain startup > to fail, than for it to carry on without the restriction. > > The only reason I am not saying someone should be issuing an advisory > is that this feature was never supported by any of the Xen toolstacks. Ah, right. And libxl has never passed -xen-domid-restrict in previous releases, so we are OK. Acked-by: Stefano Stabellini <sstabellini@kernel.org>
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h index 86c7f26..3f44a63 100644 --- a/include/hw/xen/xen_common.h +++ b/include/hw/xen/xen_common.h @@ -91,6 +91,16 @@ static inline void *xenforeignmemory_map2(xenforeignmemory_handle *h, return xenforeignmemory_map(h, dom, prot, pages, arr, err); } +static inline int xentoolcore_restrict_all(domid_t domid) +{ + errno = ENOTTY; + return -1; +} + +#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */ + +#include <xentoolcore.h> + #endif #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900 @@ -218,20 +228,6 @@ static inline int xendevicemodel_set_mem_type( return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr); } -static inline int xendevicemodel_restrict( - xendevicemodel_handle *dmod, domid_t domid) -{ - errno = ENOTTY; - return -1; -} - -static inline int xenforeignmemory_restrict( - xenforeignmemory_handle *fmem, domid_t domid) -{ - errno = ENOTTY; - return -1; -} - #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */ #undef XC_WANT_COMPAT_DEVICEMODEL_API @@ -290,28 +286,8 @@ static inline int xen_modified_memory(domid_t domid, uint64_t first_pfn, static inline int xen_restrict(domid_t domid) { int rc; - - /* Attempt to restrict devicemodel operations */ - rc = xendevicemodel_restrict(xen_dmod, domid); + rc = xentoolcore_restrict_all(domid); trace_xen_domid_restrict(rc ? errno : 0); - - if (rc < 0) { - /* - * If errno is ENOTTY then restriction is not implemented so - * there's no point in trying to restrict other types of - * operation, but it should not be treated as a failure. - */ - if (errno == ENOTTY) { - return 0; - } - - return rc; - } - - /* Restrict foreignmemory operations */ - rc = xenforeignmemory_restrict(xen_fmem, domid); - trace_xen_domid_restrict(rc ? errno : 0); - return rc; }