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

[PATCH v2 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 -ENOTTY 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                | 204 ++++++++++++++++++++++++++++++++++-
 include/uapi/xen/privcmd.h           |  15 +++
 include/xen/interface/hvm/dm_op.h    |  32 ++++++
 include/xen/interface/xen.h          |   1 +
 5 files changed, 255 insertions(+), 4 deletions(-)
 create mode 100644 include/xen/interface/hvm/dm_op.h

-- 
2.1.4

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

[Paul Durrant]

The code sets the default return code to -ENOSYS but then overrides this
to -EINVAL in the switch() statement's default case, which is clearly
silly.

This patch removes the override and sets the default return code to
-ENOTTY, which is the conventional return for an unimplemented ioctl.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>

v2:
- Use -ENOTTY rather than -ENOSYS
---
 drivers/xen/privcmd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 6e3306f..5e5c7ae 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -551,7 +551,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
 static long privcmd_ioctl(struct file *file,
 			  unsigned int cmd, unsigned long data)
 {
-	int ret = -ENOSYS;
+	int ret = -ENOTTY;
 	void __user *udata = (void __user *) data;
 
 	switch (cmd) {
@@ -572,7 +572,6 @@ static long privcmd_ioctl(struct file *file,
 		break;
 
 	default:
-		ret = -EINVAL;
 		break;
 	}
 
-- 
2.1.4

[PATCH v2 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>

v2:
- Lock the user pages rather than bouncing through kernel memory
---
 arch/x86/include/asm/xen/hypercall.h |   7 ++
 drivers/xen/privcmd.c                | 138 +++++++++++++++++++++++++++++++++++
 include/uapi/xen/privcmd.h           |  13 ++++
 include/xen/interface/hvm/dm_op.h    |  32 ++++++++
 include/xen/interface/xen.h          |   1 +
 5 files changed, 191 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 5e5c7ae..d5cf042 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,139 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
 	goto out;
 }
 
+static int lock_pages(
+	struct privcmd_dm_op_buf kbufs[], unsigned int num,
+	struct page *pages[], unsigned int nr_pages)
+{
+	unsigned int i;
+
+	for (i = 0; i < num; i++) {
+		unsigned int requested;
+		int pinned;
+
+		requested = DIV_ROUND_UP(
+			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
+			PAGE_SIZE);
+		if (requested > nr_pages)
+			return -ENOSPC;
+
+		pinned = get_user_pages_fast(
+			(unsigned long) kbufs[i].uptr,
+			requested, FOLL_WRITE, pages);
+		if (pinned < 0)
+			return pinned;
+
+		nr_pages -= pinned;
+		pages += pinned;
+	}
+
+	return 0;
+}
+
+static void unlock_pages(struct page *pages[], unsigned int nr_pages)
+{
+	unsigned int i;
+
+	if (!pages)
+		return;
+
+	for (i = 0; i < nr_pages; i++) {
+		if (pages[i])
+			put_page(pages[i]);
+	}
+}
+
+static long privcmd_ioctl_dm_op(void __user *udata)
+{
+	struct privcmd_dm_op kdata;
+	struct privcmd_dm_op_buf *kbufs;
+	unsigned int nr_pages = 0;
+	struct page **pages = 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 -E2BIG;
+
+	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;
+	}
+
+	for (i = 0; i < kdata.num; i++) {
+		if (!access_ok(VERIFY_WRITE, kbufs[i].uptr,
+			       kbufs[i].size)) {
+			rc = -EFAULT;
+			goto out;
+		}
+
+		nr_pages += DIV_ROUND_UP(
+			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
+			PAGE_SIZE);
+	}
+
+	/*
+	 * Again, set a tolerable upper limit on the number of pages
+	 * needed to lock all the buffers without being overly
+	 * restrictive, since we can't easily predict the size of
+	 * buffers future dm_ops may use.
+	 */
+	if (nr_pages * sizeof(*pages) > PAGE_SIZE) {
+		rc = -E2BIG;
+		goto out;
+	}
+
+	pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
+	if (!pages) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL);
+	if (!xbufs) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	rc = lock_pages(kbufs, kdata.num, pages, nr_pages);
+	if (rc)
+		goto out;
+
+	for (i = 0; i < kdata.num; i++) {
+		set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
+		xbufs[i].size = kbufs[i].size;
+	}
+
+	xen_preemptible_hcall_begin();
+	rc = HYPERVISOR_dm_op(kdata.dom, kdata.num, xbufs);
+	xen_preemptible_hcall_end();
+
+out:
+	unlock_pages(pages, nr_pages);
+	kfree(xbufs);
+	kfree(pages);
+	kfree(kbufs);
+
+	return rc;
+}
+
 static long privcmd_ioctl(struct file *file,
 			  unsigned int cmd, unsigned long data)
 {
@@ -571,6 +705,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

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

[Boris Ostrovsky]

On 02/10/2017 09:24 AM, Paul Durrant wrote:
> +static long privcmd_ioctl_dm_op(void __user *udata)
> +{
> +	struct privcmd_dm_op kdata;
> +	struct privcmd_dm_op_buf *kbufs;
> +	unsigned int nr_pages = 0;
> +	struct page **pages = 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.
> +	 */

I think this deserves its own macro since it really has nothing to do
with page size, has it? Especially since you are referencing it again
below too.


> +	if (kdata.num * sizeof(*kbufs) > PAGE_SIZE)
> +		return -E2BIG;
> +
> +	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;
> +	}
> +
> +	for (i = 0; i < kdata.num; i++) {
> +		if (!access_ok(VERIFY_WRITE, kbufs[i].uptr,
> +			       kbufs[i].size)) {
> +			rc = -EFAULT;
> +			goto out;
> +		}
> +
> +		nr_pages += DIV_ROUND_UP(
> +			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
> +			PAGE_SIZE);
> +	}
> +
> +	/*
> +	 * Again, set a tolerable upper limit on the number of pages
> +	 * needed to lock all the buffers without being overly
> +	 * restrictive, since we can't easily predict the size of
> +	 * buffers future dm_ops may use.
> +	 */

OTOH, these two cases describe different types of copying (the first one
is for buffer descriptors and the second is for buffers themselves). And
so should they be limited by the same value?

> +	if (nr_pages * sizeof(*pages) > PAGE_SIZE) {
> +		rc = -E2BIG;
> +		goto out;
> +	}
> +
> +	pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
> +	if (!pages) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL);
> +	if (!xbufs) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	rc = lock_pages(kbufs, kdata.num, pages, nr_pages);


Aren't those buffers already locked (as Andrew mentioned)? They are
mmapped with MAP_LOCKED.

And I also wonder whether we need to take rlimit(RLIMIT_MEMLOCK) into
account.

-boris

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

[Paul Durrant]

> -----Original Message-----
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: 10 February 2017 16:18
> 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 v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
> 
> On 02/10/2017 09:24 AM, Paul Durrant wrote:
> > +static long privcmd_ioctl_dm_op(void __user *udata)
> > +{
> > +	struct privcmd_dm_op kdata;
> > +	struct privcmd_dm_op_buf *kbufs;
> > +	unsigned int nr_pages = 0;
> > +	struct page **pages = 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.
> > +	 */
> 
> I think this deserves its own macro since it really has nothing to do
> with page size, has it? Especially since you are referencing it again
> below too.
> 
> 
> > +	if (kdata.num * sizeof(*kbufs) > PAGE_SIZE)
> > +		return -E2BIG;
> > +
> > +	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;
> > +	}
> > +
> > +	for (i = 0; i < kdata.num; i++) {
> > +		if (!access_ok(VERIFY_WRITE, kbufs[i].uptr,
> > +			       kbufs[i].size)) {
> > +			rc = -EFAULT;
> > +			goto out;
> > +		}
> > +
> > +		nr_pages += DIV_ROUND_UP(
> > +			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
> > +			PAGE_SIZE);
> > +	}
> > +
> > +	/*
> > +	 * Again, set a tolerable upper limit on the number of pages
> > +	 * needed to lock all the buffers without being overly
> > +	 * restrictive, since we can't easily predict the size of
> > +	 * buffers future dm_ops may use.
> > +	 */
> 
> OTOH, these two cases describe different types of copying (the first one
> is for buffer descriptors and the second is for buffers themselves). And
> so should they be limited by the same value?
> 

I think there needs to be some limit and limiting the allocation to a page was the best I came up with. Can you think of a better one?

> > +	if (nr_pages * sizeof(*pages) > PAGE_SIZE) {
> > +		rc = -E2BIG;
> > +		goto out;
> > +	}
> > +
> > +	pages = kcalloc(nr_pages, sizeof(*pages), GFP_KERNEL);
> > +	if (!pages) {
> > +		rc = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL);
> > +	if (!xbufs) {
> > +		rc = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	rc = lock_pages(kbufs, kdata.num, pages, nr_pages);
> 
> 
> Aren't those buffers already locked (as Andrew mentioned)? They are
> mmapped with MAP_LOCKED.

No, they're not. The new libxendevicemodel code I have does not make any use of xencall or guest handles when privcmd supports the DM_OP ioctl, so the caller buffers will not be locked.

> 
> And I also wonder whether we need to take rlimit(RLIMIT_MEMLOCK) into
> account.
> 

Maybe. I'll look at that.

  Paul

> -boris
> 
> 

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

[Boris Ostrovsky]

On 02/10/2017 11:28 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>> Sent: 10 February 2017 16:18
>> 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 v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
>>
>> On 02/10/2017 09:24 AM, Paul Durrant wrote:
>>> +static long privcmd_ioctl_dm_op(void __user *udata)
>>> +{
>>> +	struct privcmd_dm_op kdata;
>>> +	struct privcmd_dm_op_buf *kbufs;
>>> +	unsigned int nr_pages = 0;
>>> +	struct page **pages = 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.
>>> +	 */
>> I think this deserves its own macro since it really has nothing to do
>> with page size, has it? Especially since you are referencing it again
>> below too.
>>
>>
>>> +	if (kdata.num * sizeof(*kbufs) > PAGE_SIZE)
>>> +		return -E2BIG;
>>> +
>>> +	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;
>>> +	}
>>> +
>>> +	for (i = 0; i < kdata.num; i++) {
>>> +		if (!access_ok(VERIFY_WRITE, kbufs[i].uptr,
>>> +			       kbufs[i].size)) {
>>> +			rc = -EFAULT;
>>> +			goto out;
>>> +		}
>>> +
>>> +		nr_pages += DIV_ROUND_UP(
>>> +			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
>>> +			PAGE_SIZE);
>>> +	}
>>> +
>>> +	/*
>>> +	 * Again, set a tolerable upper limit on the number of pages
>>> +	 * needed to lock all the buffers without being overly
>>> +	 * restrictive, since we can't easily predict the size of
>>> +	 * buffers future dm_ops may use.
>>> +	 */
>> OTOH, these two cases describe different types of copying (the first one
>> is for buffer descriptors and the second is for buffers themselves). And
>> so should they be limited by the same value?
>>
> I think there needs to be some limit and limiting the allocation to a page was the best I came up with. Can you think of a better one?

How about something like (with rather arbitrary values)

#define PRIVCMD_DMOP_MAX_NUM_BUFFERS       16
#define PRIVCMD_DMOP_MAX_TOT_BUFFER_SZ     4096

and make them part of the interface (i.e. put them into privcmd.h)?

-boris

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

[Paul Durrant]

> -----Original Message-----
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: 10 February 2017 17:45
> 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 v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
> 
> On 02/10/2017 11:28 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> >> Sent: 10 February 2017 16:18
> >> 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 v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
> >>
> >> On 02/10/2017 09:24 AM, Paul Durrant wrote:
> >>> +static long privcmd_ioctl_dm_op(void __user *udata)
> >>> +{
> >>> +	struct privcmd_dm_op kdata;
> >>> +	struct privcmd_dm_op_buf *kbufs;
> >>> +	unsigned int nr_pages = 0;
> >>> +	struct page **pages = 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.
> >>> +	 */
> >> I think this deserves its own macro since it really has nothing to do
> >> with page size, has it? Especially since you are referencing it again
> >> below too.
> >>
> >>
> >>> +	if (kdata.num * sizeof(*kbufs) > PAGE_SIZE)
> >>> +		return -E2BIG;
> >>> +
> >>> +	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;
> >>> +	}
> >>> +
> >>> +	for (i = 0; i < kdata.num; i++) {
> >>> +		if (!access_ok(VERIFY_WRITE, kbufs[i].uptr,
> >>> +			       kbufs[i].size)) {
> >>> +			rc = -EFAULT;
> >>> +			goto out;
> >>> +		}
> >>> +
> >>> +		nr_pages += DIV_ROUND_UP(
> >>> +			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
> >>> +			PAGE_SIZE);
> >>> +	}
> >>> +
> >>> +	/*
> >>> +	 * Again, set a tolerable upper limit on the number of pages
> >>> +	 * needed to lock all the buffers without being overly
> >>> +	 * restrictive, since we can't easily predict the size of
> >>> +	 * buffers future dm_ops may use.
> >>> +	 */
> >> OTOH, these two cases describe different types of copying (the first one
> >> is for buffer descriptors and the second is for buffers themselves). And
> >> so should they be limited by the same value?
> >>
> > I think there needs to be some limit and limiting the allocation to a page
> was the best I came up with. Can you think of a better one?
> 
> How about something like (with rather arbitrary values)
> 
> #define PRIVCMD_DMOP_MAX_NUM_BUFFERS       16
> #define PRIVCMD_DMOP_MAX_TOT_BUFFER_SZ     4096
> 
> and make them part of the interface (i.e. put them into privcmd.h)?

Given that the values are arbitrary, I think it may be better to make them module params. They can then at least be tweaked if privcmd becomes a problem with later dm_ops.

  Paul

> 
> -boris

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

[Boris Ostrovsky]

>> How about something like (with rather arbitrary values)
>>
>> #define PRIVCMD_DMOP_MAX_NUM_BUFFERS       16
>> #define PRIVCMD_DMOP_MAX_TOT_BUFFER_SZ     4096
>>
>> and make them part of the interface (i.e. put them into privcmd.h)?
>
> Given that the values are arbitrary, I think it may be better to make them module params. They can then at least be tweaked if privcmd becomes a problem with later dm_ops.

Making them adjustable is a good thing but we still need default values.


-boris

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

[Paul Durrant]

> -----Original Message-----
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: 13 February 2017 14:09
> 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 v2 2/3] xen/privcmd: Add IOCTL_PRIVCMD_DM_OP
> 
> 
> >> How about something like (with rather arbitrary values)
> >>
> >> #define PRIVCMD_DMOP_MAX_NUM_BUFFERS       16
> >> #define PRIVCMD_DMOP_MAX_TOT_BUFFER_SZ     4096
> >>
> >> and make them part of the interface (i.e. put them into privcmd.h)?
> >
> > Given that the values are arbitrary, I think it may be better to make them
> module params. They can then at least be tweaked if privcmd becomes a
> problem with later dm_ops.
> 
> Making them adjustable is a good thing but we still need default values.
> 

Oh, sure. I think I'll actually go for 16 bufs and a max of 4k per bufs as defaults. Patch v3 (including a fix for the arm build) coming shortly.

Cheers,

  Paul

> 
> -boris

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1659 bytes --]

Hi Paul,

[auto build test ERROR on xen-tip/linux-next]
[also build test ERROR on v4.10-rc7 next-20170210]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Paul-Durrant/xen-privcmd-support-for-dm_op-and-restriction/20170211-001520
base:   https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git linux-next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   drivers/xen/privcmd.c: In function 'privcmd_ioctl_dm_op':
>> drivers/xen/privcmd.c:673:7: error: implicit declaration of function 'HYPERVISOR_dm_op' [-Werror=implicit-function-declaration]
     rc = HYPERVISOR_dm_op(kdata.dom, kdata.num, xbufs);
          ^~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/HYPERVISOR_dm_op +673 drivers/xen/privcmd.c

   667		for (i = 0; i < kdata.num; i++) {
   668			set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
   669			xbufs[i].size = kbufs[i].size;
   670		}
   671	
   672		xen_preemptible_hcall_begin();
 > 673		rc = HYPERVISOR_dm_op(kdata.dom, kdata.num, xbufs);
   674		xen_preemptible_hcall_end();
   675	
   676	out:

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33932 bytes --]

[PATCH v2 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>

v2:
- Make sure that a restriction cannot be cleared
---
 drivers/xen/privcmd.c      | 67 +++++++++++++++++++++++++++++++++++++++++++---
 include/uapi/xen/privcmd.h |  2 ++
 2 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index d5cf042..e372aae 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;
 
@@ -591,8 +600,9 @@ static void unlock_pages(struct page *pages[], unsigned int nr_pages)
 	}
 }
 
-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;
 	unsigned int nr_pages = 0;
@@ -604,6 +614,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;
 
@@ -682,6 +696,23 @@ 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, or check it matches */
+	if (data->domid == DOMID_INVALID)
+		data->domid = dom;
+	else if (data->domid != dom)
+		return -EINVAL;
+
+	return 0;
+}
+
 static long privcmd_ioctl(struct file *file,
 			  unsigned int cmd, unsigned long data)
 {
@@ -690,7 +721,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:
@@ -706,7 +737,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:
@@ -716,6 +751,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;
@@ -784,6 +841,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