[PATCH 0/3] xen/privcmd: support for dm_op and restriction

[PATCH 0/3] xen/privcmd: support for dm_op and restriction

[Paul Durrant]

This patch series follows on from my recent Xen series [1], to provide
support in privcmd for de-privileging of device emulators.

[1] https://lists.xen.org/archives/html/xen-devel/2017-01/msg02558.html

Paul Durrant (3):
  xen/privcmd: return -ENOSYS for unimplemented IOCTLs
  xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
  xen/privcmd: add IOCTL_PRIVCMD_RESTRICT

 arch/x86/include/asm/xen/hypercall.h |   7 ++
 drivers/xen/privcmd.c                | 183 ++++++++++++++++++++++++++++++++++-
 include/uapi/xen/privcmd.h           |  15 +++
 include/xen/interface/hvm/dm_op.h    |  32 ++++++
 include/xen/interface/xen.h          |   1 +
 5 files changed, 235 insertions(+), 3 deletions(-)
 create mode 100644 include/xen/interface/hvm/dm_op.h

-- 
2.1.4


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

[PATCH 1/3] xen/privcmd: return -ENOSYS for unimplemented IOCTLs

[Paul Durrant]

The code goes so far as to set the default return code to -ENOSYS but
then overrides this to -EINVAL in the switch() statement's default
case.

This patch removes this pointless and incorrect override.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
---
 drivers/xen/privcmd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 6e3306f..b4e5e27 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -572,7 +572,6 @@ static long privcmd_ioctl(struct file *file,
 		break;
 
 	default:
-		ret = -EINVAL;
 		break;
 	}
 
-- 
2.1.4


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

Re: [PATCH 1/3] xen/privcmd: return -ENOSYS for unimplemented IOCTLs

[Jan Beulich]

>>> On 09.02.17 at 15:17, <paul.durrant@citrix.com> wrote:
> The code goes so far as to set the default return code to -ENOSYS but
> then overrides this to -EINVAL in the switch() statement's default
> case.

If you already change this, isn't -ENOTTY the traditional way of
indicating unsupported ioctls?

Jan


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

Re: [PATCH 1/3] xen/privcmd: return -ENOSYS for unimplemented IOCTLs

[Boris Ostrovsky]

On 02/09/2017 09:40 AM, Jan Beulich wrote:
>>>> On 09.02.17 at 15:17, <paul.durrant@citrix.com> wrote:
>> The code goes so far as to set the default return code to -ENOSYS but
>> then overrides this to -EINVAL in the switch() statement's default
>> case.
>
> If you already change this, isn't -ENOTTY the traditional way of
> indicating unsupported ioctls?

In fact, a while ago David submitted a patch to do just that:

https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg00744.html

but it never went anywhere.

My question is whether anyone might be relying on current error return 
behavior.


-boris

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

Re: [PATCH 1/3] xen/privcmd: return -ENOSYS for unimplemented IOCTLs

[Paul Durrant]

> -----Original Message-----
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: 09 February 2017 15:26
> To: Jan Beulich <JBeulich@suse.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Juergen Gross <JGross@suse.com>;
> linux-kernel@vger.kernel.org
> Subject: Re: [Xen-devel] [PATCH 1/3] xen/privcmd: return -ENOSYS for
> unimplemented IOCTLs
> 
> 
> 
> On 02/09/2017 09:40 AM, Jan Beulich wrote:
> >>>> On 09.02.17 at 15:17, <paul.durrant@citrix.com> wrote:
> >> The code goes so far as to set the default return code to -ENOSYS but
> >> then overrides this to -EINVAL in the switch() statement's default
> >> case.
> >
> > If you already change this, isn't -ENOTTY the traditional way of
> > indicating unsupported ioctls?
> 
> In fact, a while ago David submitted a patch to do just that:
> 
> https://lists.xenproject.org/archives/html/xen-devel/2016-
> 08/msg00744.html
> 
> but it never went anywhere.
> 
> My question is whether anyone might be relying on current error return
> behavior.

I doubt it. It's certainly not a safe thing to do anyway. I'll change to -ENOTTY in v2 of the patch.

  Paul

> 
> 
> -boris

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

[PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Paul Durrant]

Recently a new dm_op[1] hypercall was added to Xen to provide a mechanism
for restricting device emulators (such as QEMU) to a limited set of
hypervisor operations, and being able to audit those operations in the
kernel of the domain in which they run.

This patch adds IOCTL_PRIVCMD_DM_OP as gateway for __HYPERVISOR_dm_op,
bouncing the callers buffers through kernel memory to allow the address
ranges to be audited (and negating the need to bounce through locked
memory in user-space).

[1] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=524a98c2

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
---
 arch/x86/include/asm/xen/hypercall.h |   7 ++
 drivers/xen/privcmd.c                | 122 +++++++++++++++++++++++++++++++++++
 include/uapi/xen/privcmd.h           |  13 ++++
 include/xen/interface/hvm/dm_op.h    |  32 +++++++++
 include/xen/interface/xen.h          |   1 +
 5 files changed, 175 insertions(+)
 create mode 100644 include/xen/interface/hvm/dm_op.h

diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index a12a047..f6d20f6 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -472,6 +472,13 @@ HYPERVISOR_xenpmu_op(unsigned int op, void *arg)
 	return _hypercall2(int, xenpmu_op, op, arg);
 }
 
+static inline int
+HYPERVISOR_dm_op(
+	domid_t dom, unsigned int nr_bufs, void *bufs)
+{
+	return _hypercall3(int, dm_op, dom, nr_bufs, bufs);
+}
+
 static inline void
 MULTI_fpu_taskswitch(struct multicall_entry *mcl, int set)
 {
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index b4e5e27..31c43f4 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -32,6 +32,7 @@
 #include <xen/xen.h>
 #include <xen/privcmd.h>
 #include <xen/interface/xen.h>
+#include <xen/interface/hvm/dm_op.h>
 #include <xen/features.h>
 #include <xen/page.h>
 #include <xen/xen-ops.h>
@@ -548,6 +549,123 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
 	goto out;
 }
 
+static int bounce_in(struct privcmd_dm_op_buf kbufs[], void *kptr[],
+	unsigned int num)
+{
+	unsigned int i;
+	int rc = 0;
+
+	for (i = 0; i < num; i++) {
+		kptr[i] = kzalloc(kbufs[i].size, GFP_KERNEL);
+		if (!kptr[i]) {
+			rc = -ENOMEM;
+			break;
+		}
+
+		if (copy_from_user(kptr[i], kbufs[i].uptr, kbufs[i].size)) {
+			rc = -EFAULT;
+			break;
+		}
+	}
+
+	return rc;
+}
+
+static int bounce_out(struct privcmd_dm_op_buf kbufs[], void *kptr[],
+	unsigned int num)
+{
+	unsigned int i;
+	int rc = 0;
+
+	for (i = 0; i < num; i++)
+		if (copy_to_user(kbufs[i].uptr, kptr[i], kbufs[i].size))
+			rc = -EFAULT;
+
+	return rc;
+}
+
+static void free_kptr(void *kptr[], unsigned int num)
+{
+	unsigned int i;
+
+	if (!kptr)
+		return;
+
+	for (i = 0; i < num; i++)
+		kfree(kptr[i]);
+
+	kfree(kptr);
+}
+
+static long privcmd_ioctl_dm_op(void __user *udata)
+{
+	struct privcmd_dm_op kdata;
+	struct privcmd_dm_op_buf *kbufs;
+	void **kptr = NULL;
+	struct xen_dm_op_buf *xbufs = NULL;
+	unsigned int i;
+	long rc;
+
+	if (copy_from_user(&kdata, udata, sizeof(kdata)))
+		return -EFAULT;
+
+	if (kdata.num == 0)
+		return 0;
+
+	/*
+	 * Set a tolerable upper limit on the number of buffers
+	 * without being overly restrictive, since we can't easily
+	 * predict what future dm_ops may require.
+	 */
+	if (kdata.num * sizeof(*kbufs) > PAGE_SIZE)
+		return -EINVAL;
+
+	kbufs = kcalloc(kdata.num, sizeof(*kbufs), GFP_KERNEL);
+	if (!kbufs)
+		return -ENOMEM;
+
+	if (copy_from_user(kbufs, kdata.ubufs,
+			   sizeof(*kbufs) * kdata.num)) {
+		rc = -EFAULT;
+		goto out;
+	}
+
+	kptr = kcalloc(kdata.num, sizeof(*kptr), GFP_KERNEL);
+	if (!kptr) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	rc = bounce_in(kbufs, kptr, kdata.num);
+	if (rc)
+		goto out;
+
+	xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL);
+	if (!xbufs) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	for (i = 0; i < kdata.num; i++) {
+		set_xen_guest_handle(xbufs[i].h, kptr[i]);
+		xbufs[i].size = kbufs[i].size;
+	}
+
+	xen_preemptible_hcall_begin();
+	rc = HYPERVISOR_dm_op(kdata.dom, kdata.num, xbufs);
+	xen_preemptible_hcall_end();
+
+	if (!rc)
+		rc = bounce_out(kbufs, kptr, kdata.num);
+
+out:
+	kfree(xbufs);
+	free_kptr(kptr, kdata.num);
+	kfree(kbufs);
+
+	return rc;
+}
+
 static long privcmd_ioctl(struct file *file,
 			  unsigned int cmd, unsigned long data)
 {
@@ -571,6 +689,10 @@ static long privcmd_ioctl(struct file *file,
 		ret = privcmd_ioctl_mmap_batch(udata, 2);
 		break;
 
+	case IOCTL_PRIVCMD_DM_OP:
+		ret = privcmd_ioctl_dm_op(udata);
+		break;
+
 	default:
 		break;
 	}
diff --git a/include/uapi/xen/privcmd.h b/include/uapi/xen/privcmd.h
index 7ddeeda..f8c5d75 100644
--- a/include/uapi/xen/privcmd.h
+++ b/include/uapi/xen/privcmd.h
@@ -77,6 +77,17 @@ struct privcmd_mmapbatch_v2 {
 	int __user *err;  /* array of error codes */
 };
 
+struct privcmd_dm_op_buf {
+	void __user *uptr;
+	size_t size;
+};
+
+struct privcmd_dm_op {
+	domid_t dom;
+	__u16 num;
+	const struct privcmd_dm_op_buf __user *ubufs;
+};
+
 /*
  * @cmd: IOCTL_PRIVCMD_HYPERCALL
  * @arg: &privcmd_hypercall_t
@@ -98,5 +109,7 @@ struct privcmd_mmapbatch_v2 {
 	_IOC(_IOC_NONE, 'P', 3, sizeof(struct privcmd_mmapbatch))
 #define IOCTL_PRIVCMD_MMAPBATCH_V2				\
 	_IOC(_IOC_NONE, 'P', 4, sizeof(struct privcmd_mmapbatch_v2))
+#define IOCTL_PRIVCMD_DM_OP					\
+	_IOC(_IOC_NONE, 'P', 5, sizeof(struct privcmd_dm_op))
 
 #endif /* __LINUX_PUBLIC_PRIVCMD_H__ */
diff --git a/include/xen/interface/hvm/dm_op.h b/include/xen/interface/hvm/dm_op.h
new file mode 100644
index 0000000..ee9e480
--- /dev/null
+++ b/include/xen/interface/hvm/dm_op.h
@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) 2016, Citrix Systems Inc
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef __XEN_PUBLIC_HVM_DM_OP_H__
+#define __XEN_PUBLIC_HVM_DM_OP_H__
+
+struct xen_dm_op_buf {
+	GUEST_HANDLE(void) h;
+	xen_ulong_t size;
+};
+DEFINE_GUEST_HANDLE_STRUCT(xen_dm_op_buf);
+
+#endif /* __XEN_PUBLIC_HVM_DM_OP_H__ */
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
index 1b0d189..4f4830e 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -81,6 +81,7 @@
 #define __HYPERVISOR_tmem_op              38
 #define __HYPERVISOR_xc_reserved_op       39 /* reserved for XenClient */
 #define __HYPERVISOR_xenpmu_op            40
+#define __HYPERVISOR_dm_op                41
 
 /* Architecture-specific hypercall definitions. */
 #define __HYPERVISOR_arch_0               48
-- 
2.1.4


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

[PATCH 3/3] xen/privcmd: add IOCTL_PRIVCMD_RESTRICT

[Paul Durrant]

The purpose if this ioctl is to allow a user of privcmd to restrict its
operation such that it will no longer service arbitrary hypercalls via
IOCTL_PRIVCMD_HYPERCALL, and will check for a matching domid when
servicing IOCTL_PRIVCMD_DM_OP. The aim of this is to limit the attack
surface for a compromised device model.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
---
 drivers/xen/privcmd.c      | 64 +++++++++++++++++++++++++++++++++++++++++++---
 include/uapi/xen/privcmd.h |  2 ++
 2 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 31c43f4..ef14ac8 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -44,16 +44,25 @@ MODULE_LICENSE("GPL");
 
 #define PRIV_VMA_LOCKED ((void *)1)
 
+struct privcmd_data {
+	domid_t domid;
+};
+
 static int privcmd_vma_range_is_mapped(
                struct vm_area_struct *vma,
                unsigned long addr,
                unsigned long nr_pages);
 
-static long privcmd_ioctl_hypercall(void __user *udata)
+static long privcmd_ioctl_hypercall(struct file *file, void __user *udata)
 {
+	struct privcmd_data *data = file->private_data;
 	struct privcmd_hypercall hypercall;
 	long ret;
 
+	/* Disallow arbitrary hypercalls if restricted */
+	if (data->domid != DOMID_INVALID)
+		return -EPERM;
+
 	if (copy_from_user(&hypercall, udata, sizeof(hypercall)))
 		return -EFAULT;
 
@@ -597,8 +606,9 @@ static void free_kptr(void *kptr[], unsigned int num)
 	kfree(kptr);
 }
 
-static long privcmd_ioctl_dm_op(void __user *udata)
+static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
 {
+	struct privcmd_data *data = file->private_data;
 	struct privcmd_dm_op kdata;
 	struct privcmd_dm_op_buf *kbufs;
 	void **kptr = NULL;
@@ -609,6 +619,10 @@ static long privcmd_ioctl_dm_op(void __user *udata)
 	if (copy_from_user(&kdata, udata, sizeof(kdata)))
 		return -EFAULT;
 
+	/* If restriction is in place, check the domid matches */
+	if (data->domid != DOMID_INVALID && data->domid != kdata.dom)
+		return -EPERM;
+
 	if (kdata.num == 0)
 		return 0;
 
@@ -666,6 +680,20 @@ static long privcmd_ioctl_dm_op(void __user *udata)
 	return rc;
 }
 
+static long privcmd_ioctl_restrict(struct file *file, void __user *udata)
+{
+	struct privcmd_data *data = file->private_data;
+	domid_t dom;
+
+	if (copy_from_user(&dom, udata, sizeof(dom)))
+		return -EFAULT;
+
+	/* Set restriction to the specified domain */
+	data->domid = dom;
+
+	return 0;
+}
+
 static long privcmd_ioctl(struct file *file,
 			  unsigned int cmd, unsigned long data)
 {
@@ -674,7 +702,7 @@ static long privcmd_ioctl(struct file *file,
 
 	switch (cmd) {
 	case IOCTL_PRIVCMD_HYPERCALL:
-		ret = privcmd_ioctl_hypercall(udata);
+		ret = privcmd_ioctl_hypercall(file, udata);
 		break;
 
 	case IOCTL_PRIVCMD_MMAP:
@@ -690,7 +718,11 @@ static long privcmd_ioctl(struct file *file,
 		break;
 
 	case IOCTL_PRIVCMD_DM_OP:
-		ret = privcmd_ioctl_dm_op(udata);
+		ret = privcmd_ioctl_dm_op(file, udata);
+		break;
+
+	case IOCTL_PRIVCMD_RESTRICT:
+		ret = privcmd_ioctl_restrict(file, udata);
 		break;
 
 	default:
@@ -700,6 +732,28 @@ static long privcmd_ioctl(struct file *file,
 	return ret;
 }
 
+static int privcmd_open(struct inode *ino, struct file *file)
+{
+	struct privcmd_data *data = kzalloc(sizeof(*data), GFP_KERNEL);
+
+	if (!data)
+		return -ENOMEM;
+
+	/* DOMID_INVALID implies no restriction */
+	data->domid = DOMID_INVALID;
+
+	file->private_data = data;
+	return 0;
+}
+
+static int privcmd_release(struct inode *ino, struct file *file)
+{
+	struct privcmd_data *data = file->private_data;
+
+	kfree(data);
+	return 0;
+}
+
 static void privcmd_close(struct vm_area_struct *vma)
 {
 	struct page **pages = vma->vm_private_data;
@@ -768,6 +822,8 @@ static int privcmd_vma_range_is_mapped(
 const struct file_operations xen_privcmd_fops = {
 	.owner = THIS_MODULE,
 	.unlocked_ioctl = privcmd_ioctl,
+	.open = privcmd_open,
+	.release = privcmd_release,
 	.mmap = privcmd_mmap,
 };
 EXPORT_SYMBOL_GPL(xen_privcmd_fops);
diff --git a/include/uapi/xen/privcmd.h b/include/uapi/xen/privcmd.h
index f8c5d75..63ee95c 100644
--- a/include/uapi/xen/privcmd.h
+++ b/include/uapi/xen/privcmd.h
@@ -111,5 +111,7 @@ struct privcmd_dm_op {
 	_IOC(_IOC_NONE, 'P', 4, sizeof(struct privcmd_mmapbatch_v2))
 #define IOCTL_PRIVCMD_DM_OP					\
 	_IOC(_IOC_NONE, 'P', 5, sizeof(struct privcmd_dm_op))
+#define IOCTL_PRIVCMD_RESTRICT					\
+	_IOC(_IOC_NONE, 'P', 6, sizeof(domid_t))
 
 #endif /* __LINUX_PUBLIC_PRIVCMD_H__ */
-- 
2.1.4


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

Re: [PATCH 3/3] xen/privcmd: add IOCTL_PRIVCMD_RESTRICT

[Jan Beulich]

>>> On 09.02.17 at 15:17, <paul.durrant@citrix.com> wrote:
> @@ -666,6 +680,20 @@ static long privcmd_ioctl_dm_op(void __user *udata)
>  	return rc;
>  }
>  
> +static long privcmd_ioctl_restrict(struct file *file, void __user *udata)
> +{
> +	struct privcmd_data *data = file->private_data;
> +	domid_t dom;
> +
> +	if (copy_from_user(&dom, udata, sizeof(dom)))
> +		return -EFAULT;
> +
> +	/* Set restriction to the specified domain */
> +	data->domid = dom;
> +
> +	return 0;
> +}

Is it really intended for the caller to be able to undo this, by passing
in DOMID_INVALID?

Jan


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

Re: [PATCH 3/3] xen/privcmd: add IOCTL_PRIVCMD_RESTRICT

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 09 February 2017 14:43
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>; Juergen Gross <JGross@suse.com>; linux-
> kernel@vger.kernel.org
> Subject: Re: [Xen-devel] [PATCH 3/3] xen/privcmd: add
> IOCTL_PRIVCMD_RESTRICT
> 
> >>> On 09.02.17 at 15:17, <paul.durrant@citrix.com> wrote:
> > @@ -666,6 +680,20 @@ static long privcmd_ioctl_dm_op(void __user
> *udata)
> >  	return rc;
> >  }
> >
> > +static long privcmd_ioctl_restrict(struct file *file, void __user *udata)
> > +{
> > +	struct privcmd_data *data = file->private_data;
> > +	domid_t dom;
> > +
> > +	if (copy_from_user(&dom, udata, sizeof(dom)))
> > +		return -EFAULT;
> > +
> > +	/* Set restriction to the specified domain */
> > +	data->domid = dom;
> > +
> > +	return 0;
> > +}
> 
> Is it really intended for the caller to be able to undo this, by passing
> in DOMID_INVALID?

Good point. I was intending to fix that, but forgot.

  Paul

> 
> Jan


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

Re: [PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Paul Durrant]

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: 09 February 2017 14:18
> To: xen-devel@lists.xenproject.org; linux-kernel@vger.kernel.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>; Juergen Gross <jgross@suse.com>
> Subject: [PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
> 
> Recently a new dm_op[1] hypercall was added to Xen to provide a
> mechanism
> for restricting device emulators (such as QEMU) to a limited set of
> hypervisor operations, and being able to audit those operations in the
> kernel of the domain in which they run.
> 
> This patch adds IOCTL_PRIVCMD_DM_OP as gateway for
> __HYPERVISOR_dm_op,
> bouncing the callers buffers through kernel memory to allow the address
> ranges to be audited (and negating the need to bounce through locked
> memory in user-space).

Actually, it strikes me (now that I've posted the patch) that I should probably just mlock the user buffers rather than bouncing them through kernel... Anyway, I'd still appreciate review on other aspects of the patch.

  Paul

> 
> [1] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=524a98c2
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/include/asm/xen/hypercall.h |   7 ++
>  drivers/xen/privcmd.c                | 122
> +++++++++++++++++++++++++++++++++++
>  include/uapi/xen/privcmd.h           |  13 ++++
>  include/xen/interface/hvm/dm_op.h    |  32 +++++++++
>  include/xen/interface/xen.h          |   1 +
>  5 files changed, 175 insertions(+)
>  create mode 100644 include/xen/interface/hvm/dm_op.h
> 
> diff --git a/arch/x86/include/asm/xen/hypercall.h
> b/arch/x86/include/asm/xen/hypercall.h
> index a12a047..f6d20f6 100644
> --- a/arch/x86/include/asm/xen/hypercall.h
> +++ b/arch/x86/include/asm/xen/hypercall.h
> @@ -472,6 +472,13 @@ HYPERVISOR_xenpmu_op(unsigned int op, void
> *arg)
>  	return _hypercall2(int, xenpmu_op, op, arg);
>  }
> 
> +static inline int
> +HYPERVISOR_dm_op(
> +	domid_t dom, unsigned int nr_bufs, void *bufs)
> +{
> +	return _hypercall3(int, dm_op, dom, nr_bufs, bufs);
> +}
> +
>  static inline void
>  MULTI_fpu_taskswitch(struct multicall_entry *mcl, int set)
>  {
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index b4e5e27..31c43f4 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -32,6 +32,7 @@
>  #include <xen/xen.h>
>  #include <xen/privcmd.h>
>  #include <xen/interface/xen.h>
> +#include <xen/interface/hvm/dm_op.h>
>  #include <xen/features.h>
>  #include <xen/page.h>
>  #include <xen/xen-ops.h>
> @@ -548,6 +549,123 @@ static long privcmd_ioctl_mmap_batch(void __user
> *udata, int version)
>  	goto out;
>  }
> 
> +static int bounce_in(struct privcmd_dm_op_buf kbufs[], void *kptr[],
> +	unsigned int num)
> +{
> +	unsigned int i;
> +	int rc = 0;
> +
> +	for (i = 0; i < num; i++) {
> +		kptr[i] = kzalloc(kbufs[i].size, GFP_KERNEL);
> +		if (!kptr[i]) {
> +			rc = -ENOMEM;
> +			break;
> +		}
> +
> +		if (copy_from_user(kptr[i], kbufs[i].uptr, kbufs[i].size)) {
> +			rc = -EFAULT;
> +			break;
> +		}
> +	}
> +
> +	return rc;
> +}
> +
> +static int bounce_out(struct privcmd_dm_op_buf kbufs[], void *kptr[],
> +	unsigned int num)
> +{
> +	unsigned int i;
> +	int rc = 0;
> +
> +	for (i = 0; i < num; i++)
> +		if (copy_to_user(kbufs[i].uptr, kptr[i], kbufs[i].size))
> +			rc = -EFAULT;
> +
> +	return rc;
> +}
> +
> +static void free_kptr(void *kptr[], unsigned int num)
> +{
> +	unsigned int i;
> +
> +	if (!kptr)
> +		return;
> +
> +	for (i = 0; i < num; i++)
> +		kfree(kptr[i]);
> +
> +	kfree(kptr);
> +}
> +
> +static long privcmd_ioctl_dm_op(void __user *udata)
> +{
> +	struct privcmd_dm_op kdata;
> +	struct privcmd_dm_op_buf *kbufs;
> +	void **kptr = NULL;
> +	struct xen_dm_op_buf *xbufs = NULL;
> +	unsigned int i;
> +	long rc;
> +
> +	if (copy_from_user(&kdata, udata, sizeof(kdata)))
> +		return -EFAULT;
> +
> +	if (kdata.num == 0)
> +		return 0;
> +
> +	/*
> +	 * Set a tolerable upper limit on the number of buffers
> +	 * without being overly restrictive, since we can't easily
> +	 * predict what future dm_ops may require.
> +	 */
> +	if (kdata.num * sizeof(*kbufs) > PAGE_SIZE)
> +		return -EINVAL;
> +
> +	kbufs = kcalloc(kdata.num, sizeof(*kbufs), GFP_KERNEL);
> +	if (!kbufs)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(kbufs, kdata.ubufs,
> +			   sizeof(*kbufs) * kdata.num)) {
> +		rc = -EFAULT;
> +		goto out;
> +	}
> +
> +	kptr = kcalloc(kdata.num, sizeof(*kptr), GFP_KERNEL);
> +	if (!kptr) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	rc = bounce_in(kbufs, kptr, kdata.num);
> +	if (rc)
> +		goto out;
> +
> +	xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL);
> +	if (!xbufs) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < kdata.num; i++) {
> +		set_xen_guest_handle(xbufs[i].h, kptr[i]);
> +		xbufs[i].size = kbufs[i].size;
> +	}
> +
> +	xen_preemptible_hcall_begin();
> +	rc = HYPERVISOR_dm_op(kdata.dom, kdata.num, xbufs);
> +	xen_preemptible_hcall_end();
> +
> +	if (!rc)
> +		rc = bounce_out(kbufs, kptr, kdata.num);
> +
> +out:
> +	kfree(xbufs);
> +	free_kptr(kptr, kdata.num);
> +	kfree(kbufs);
> +
> +	return rc;
> +}
> +
>  static long privcmd_ioctl(struct file *file,
>  			  unsigned int cmd, unsigned long data)
>  {
> @@ -571,6 +689,10 @@ static long privcmd_ioctl(struct file *file,
>  		ret = privcmd_ioctl_mmap_batch(udata, 2);
>  		break;
> 
> +	case IOCTL_PRIVCMD_DM_OP:
> +		ret = privcmd_ioctl_dm_op(udata);
> +		break;
> +
>  	default:
>  		break;
>  	}
> diff --git a/include/uapi/xen/privcmd.h b/include/uapi/xen/privcmd.h
> index 7ddeeda..f8c5d75 100644
> --- a/include/uapi/xen/privcmd.h
> +++ b/include/uapi/xen/privcmd.h
> @@ -77,6 +77,17 @@ struct privcmd_mmapbatch_v2 {
>  	int __user *err;  /* array of error codes */
>  };
> 
> +struct privcmd_dm_op_buf {
> +	void __user *uptr;
> +	size_t size;
> +};
> +
> +struct privcmd_dm_op {
> +	domid_t dom;
> +	__u16 num;
> +	const struct privcmd_dm_op_buf __user *ubufs;
> +};
> +
>  /*
>   * @cmd: IOCTL_PRIVCMD_HYPERCALL
>   * @arg: &privcmd_hypercall_t
> @@ -98,5 +109,7 @@ struct privcmd_mmapbatch_v2 {
>  	_IOC(_IOC_NONE, 'P', 3, sizeof(struct privcmd_mmapbatch))
>  #define IOCTL_PRIVCMD_MMAPBATCH_V2				\
>  	_IOC(_IOC_NONE, 'P', 4, sizeof(struct privcmd_mmapbatch_v2))
> +#define IOCTL_PRIVCMD_DM_OP					\
> +	_IOC(_IOC_NONE, 'P', 5, sizeof(struct privcmd_dm_op))
> 
>  #endif /* __LINUX_PUBLIC_PRIVCMD_H__ */
> diff --git a/include/xen/interface/hvm/dm_op.h
> b/include/xen/interface/hvm/dm_op.h
> new file mode 100644
> index 0000000..ee9e480
> --- /dev/null
> +++ b/include/xen/interface/hvm/dm_op.h
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright (c) 2016, Citrix Systems Inc
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software
> is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef __XEN_PUBLIC_HVM_DM_OP_H__
> +#define __XEN_PUBLIC_HVM_DM_OP_H__
> +
> +struct xen_dm_op_buf {
> +	GUEST_HANDLE(void) h;
> +	xen_ulong_t size;
> +};
> +DEFINE_GUEST_HANDLE_STRUCT(xen_dm_op_buf);
> +
> +#endif /* __XEN_PUBLIC_HVM_DM_OP_H__ */
> diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
> index 1b0d189..4f4830e 100644
> --- a/include/xen/interface/xen.h
> +++ b/include/xen/interface/xen.h
> @@ -81,6 +81,7 @@
>  #define __HYPERVISOR_tmem_op              38
>  #define __HYPERVISOR_xc_reserved_op       39 /* reserved for XenClient */
>  #define __HYPERVISOR_xenpmu_op            40
> +#define __HYPERVISOR_dm_op                41
> 
>  /* Architecture-specific hypercall definitions. */
>  #define __HYPERVISOR_arch_0               48
> --
> 2.1.4


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

Re: [PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Boris Ostrovsky]

On 02/09/2017 09:27 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Paul Durrant [mailto:paul.durrant@citrix.com]
>> Sent: 09 February 2017 14:18
>> To: xen-devel@lists.xenproject.org; linux-kernel@vger.kernel.org
>> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Boris Ostrovsky
>> <boris.ostrovsky@oracle.com>; Juergen Gross <jgross@suse.com>
>> Subject: [PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
>>
>> Recently a new dm_op[1] hypercall was added to Xen to provide a
>> mechanism
>> for restricting device emulators (such as QEMU) to a limited set of
>> hypervisor operations, and being able to audit those operations in the
>> kernel of the domain in which they run.
>>
>> This patch adds IOCTL_PRIVCMD_DM_OP as gateway for
>> __HYPERVISOR_dm_op,
>> bouncing the callers buffers through kernel memory to allow the address
>> ranges to be audited (and negating the need to bounce through locked
>> memory in user-space).
>
> Actually, it strikes me (now that I've posted the patch) that I should probably just mlock the user buffers rather than bouncing them through kernel... Anyway, I'd still appreciate review on other aspects of the patch.


Are you suggesting that the caller (user) mlocks the buffers?

-boris


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

Re: [PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Andrew Cooper]

On 09/02/17 15:50, Boris Ostrovsky wrote:
>
>
> On 02/09/2017 09:27 AM, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Paul Durrant [mailto:paul.durrant@citrix.com]
>>> Sent: 09 February 2017 14:18
>>> To: xen-devel@lists.xenproject.org; linux-kernel@vger.kernel.org
>>> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Boris Ostrovsky
>>> <boris.ostrovsky@oracle.com>; Juergen Gross <jgross@suse.com>
>>> Subject: [PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
>>>
>>> Recently a new dm_op[1] hypercall was added to Xen to provide a
>>> mechanism
>>> for restricting device emulators (such as QEMU) to a limited set of
>>> hypervisor operations, and being able to audit those operations in the
>>> kernel of the domain in which they run.
>>>
>>> This patch adds IOCTL_PRIVCMD_DM_OP as gateway for
>>> __HYPERVISOR_dm_op,
>>> bouncing the callers buffers through kernel memory to allow the address
>>> ranges to be audited (and negating the need to bounce through locked
>>> memory in user-space).
>>
>> Actually, it strikes me (now that I've posted the patch) that I
>> should probably just mlock the user buffers rather than bouncing them
>> through kernel... Anyway, I'd still appreciate review on other
>> aspects of the patch.
>
>
> Are you suggesting that the caller (user) mlocks the buffers?

Doesn't libxc already use the hypercall buffer API for each of the buffers?

The kernel oughtn’t to need to do anything special to the user pointers
it has, other than call access_ok() on them.

~Andrew

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

Re: [PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Jan Beulich]

>>> On 09.02.17 at 16:56, <andrew.cooper3@citrix.com> wrote:
> On 09/02/17 15:50, Boris Ostrovsky wrote:
>>
>>
>> On 02/09/2017 09:27 AM, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Paul Durrant [mailto:paul.durrant@citrix.com]
>>>> Sent: 09 February 2017 14:18
>>>> To: xen-devel@lists.xenproject.org; linux-kernel@vger.kernel.org 
>>>> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Boris Ostrovsky
>>>> <boris.ostrovsky@oracle.com>; Juergen Gross <jgross@suse.com>
>>>> Subject: [PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
>>>>
>>>> Recently a new dm_op[1] hypercall was added to Xen to provide a
>>>> mechanism
>>>> for restricting device emulators (such as QEMU) to a limited set of
>>>> hypervisor operations, and being able to audit those operations in the
>>>> kernel of the domain in which they run.
>>>>
>>>> This patch adds IOCTL_PRIVCMD_DM_OP as gateway for
>>>> __HYPERVISOR_dm_op,
>>>> bouncing the callers buffers through kernel memory to allow the address
>>>> ranges to be audited (and negating the need to bounce through locked
>>>> memory in user-space).
>>>
>>> Actually, it strikes me (now that I've posted the patch) that I
>>> should probably just mlock the user buffers rather than bouncing them
>>> through kernel... Anyway, I'd still appreciate review on other
>>> aspects of the patch.
>>
>>
>> Are you suggesting that the caller (user) mlocks the buffers?
> 
> Doesn't libxc already use the hypercall buffer API for each of the buffers?
> 
> The kernel oughtn’t to need to do anything special to the user pointers
> it has, other than call access_ok() on them.

And translate 32-bit layout to 64-bit for a compat caller.

Jan

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

Re: [PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Andrew Cooper]

On 09/02/17 16:03, Jan Beulich wrote:
>>>> On 09.02.17 at 16:56, <andrew.cooper3@citrix.com> wrote:
>> On 09/02/17 15:50, Boris Ostrovsky wrote:
>>>
>>> On 02/09/2017 09:27 AM, Paul Durrant wrote:
>>>>> -----Original Message-----
>>>>> From: Paul Durrant [mailto:paul.durrant@citrix.com]
>>>>> Sent: 09 February 2017 14:18
>>>>> To: xen-devel@lists.xenproject.org; linux-kernel@vger.kernel.org 
>>>>> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Boris Ostrovsky
>>>>> <boris.ostrovsky@oracle.com>; Juergen Gross <jgross@suse.com>
>>>>> Subject: [PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
>>>>>
>>>>> Recently a new dm_op[1] hypercall was added to Xen to provide a
>>>>> mechanism
>>>>> for restricting device emulators (such as QEMU) to a limited set of
>>>>> hypervisor operations, and being able to audit those operations in the
>>>>> kernel of the domain in which they run.
>>>>>
>>>>> This patch adds IOCTL_PRIVCMD_DM_OP as gateway for
>>>>> __HYPERVISOR_dm_op,
>>>>> bouncing the callers buffers through kernel memory to allow the address
>>>>> ranges to be audited (and negating the need to bounce through locked
>>>>> memory in user-space).
>>>> Actually, it strikes me (now that I've posted the patch) that I
>>>> should probably just mlock the user buffers rather than bouncing them
>>>> through kernel... Anyway, I'd still appreciate review on other
>>>> aspects of the patch.
>>>
>>> Are you suggesting that the caller (user) mlocks the buffers?
>> Doesn't libxc already use the hypercall buffer API for each of the buffers?
>>
>> The kernel oughtn’t to need to do anything special to the user pointers
>> it has, other than call access_ok() on them.
> And translate 32-bit layout to 64-bit for a compat caller.

Ah yes (although that looks to be done suitably in the patch as presented).

~Andrew

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

Re: [PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP

[Paul Durrant]

> -----Original Message-----
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: 09 February 2017 15:50
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org;
> linux-kernel@vger.kernel.org
> Cc: Juergen Gross <jgross@suse.com>
> Subject: Re: [PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
> 
> 
> 
> On 02/09/2017 09:27 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> >> Sent: 09 February 2017 14:18
> >> To: xen-devel@lists.xenproject.org; linux-kernel@vger.kernel.org
> >> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Boris Ostrovsky
> >> <boris.ostrovsky@oracle.com>; Juergen Gross <jgross@suse.com>
> >> Subject: [PATCH 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
> >>
> >> Recently a new dm_op[1] hypercall was added to Xen to provide a
> >> mechanism
> >> for restricting device emulators (such as QEMU) to a limited set of
> >> hypervisor operations, and being able to audit those operations in the
> >> kernel of the domain in which they run.
> >>
> >> This patch adds IOCTL_PRIVCMD_DM_OP as gateway for
> >> __HYPERVISOR_dm_op,
> >> bouncing the callers buffers through kernel memory to allow the address
> >> ranges to be audited (and negating the need to bounce through locked
> >> memory in user-space).
> >
> > Actually, it strikes me (now that I've posted the patch) that I should
> probably just mlock the user buffers rather than bouncing them through
> kernel... Anyway, I'd still appreciate review on other aspects of the patch.
> 
> 
> Are you suggesting that the caller (user) mlocks the buffers?

No, I meant calling get_user_pages() (which AIUI is essentially what the internals of sys_mlock does) on the buffers to make sure they don't get paged during execution of the (unlocked) ioctl.

  Paul

> 
> -boris


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