xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv1] xen/evtchn: add IOCTL_EVTCHN_RESTRICT
@ 2016-07-11 14:57 David Vrabel
  2016-07-11 15:31 ` Boris Ostrovsky
  2016-08-01 11:15 ` Jan Beulich
  0 siblings, 2 replies; 8+ messages in thread
From: David Vrabel @ 2016-07-11 14:57 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Jennifer Herbert, Boris Ostrovsky, Ian Jackson,
	David Vrabel

IOCTL_EVTCHN_RESTRICT limits the file descriptor to being able to bind
to interdomain event channels from a specific domain.  Event channels
that are already bound continue to work for sending and receiving
notifications.

This is useful as part of deprivileging a user space PV backend or
device model (QEMU).  e.g., Once the device model as bound to the
ioreq server event channels it can restrict the file handle so an
exploited DM cannot use it to create or bind to arbitrary event
channels.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/xen/evtchn.c      | 40 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/xen/evtchn.h | 15 +++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/drivers/xen/evtchn.c b/drivers/xen/evtchn.c
index f4edd6d..7efd1cb 100644
--- a/drivers/xen/evtchn.c
+++ b/drivers/xen/evtchn.c
@@ -73,8 +73,12 @@ struct per_user_data {
 	wait_queue_head_t evtchn_wait;
 	struct fasync_struct *evtchn_async_queue;
 	const char *name;
+
+	domid_t restrict_domid;
 };
 
+#define UNRESTRICTED_DOMID ((domid_t)-1)
+
 struct user_evtchn {
 	struct rb_node node;
 	struct per_user_data *user;
@@ -443,6 +447,10 @@ static long evtchn_ioctl(struct file *file,
 		struct ioctl_evtchn_bind_virq bind;
 		struct evtchn_bind_virq bind_virq;
 
+		rc = -EACCES;
+		if (u->restrict_domid != UNRESTRICTED_DOMID)
+			break;
+
 		rc = -EFAULT;
 		if (copy_from_user(&bind, uarg, sizeof(bind)))
 			break;
@@ -468,6 +476,11 @@ static long evtchn_ioctl(struct file *file,
 		if (copy_from_user(&bind, uarg, sizeof(bind)))
 			break;
 
+		rc = -EACCES;
+		if (u->restrict_domid != UNRESTRICTED_DOMID &&
+		    u->restrict_domid != bind.remote_domain)
+			break;
+
 		bind_interdomain.remote_dom  = bind.remote_domain;
 		bind_interdomain.remote_port = bind.remote_port;
 		rc = HYPERVISOR_event_channel_op(EVTCHNOP_bind_interdomain,
@@ -485,6 +498,10 @@ static long evtchn_ioctl(struct file *file,
 		struct ioctl_evtchn_bind_unbound_port bind;
 		struct evtchn_alloc_unbound alloc_unbound;
 
+		rc = -EACCES;
+		if (u->restrict_domid != UNRESTRICTED_DOMID)
+			break;
+
 		rc = -EFAULT;
 		if (copy_from_user(&bind, uarg, sizeof(bind)))
 			break;
@@ -553,6 +570,27 @@ static long evtchn_ioctl(struct file *file,
 		break;
 	}
 
+	case IOCTL_EVTCHN_RESTRICT_DOMID: {
+		struct ioctl_evtchn_restrict_domid ierd;
+
+		rc = -EACCES;
+		if (u->restrict_domid != UNRESTRICTED_DOMID)
+			break;
+
+		rc = -EFAULT;
+		if (copy_from_user(&ierd, uarg, sizeof(ierd)))
+		    break;
+
+		rc = -EINVAL;
+		if (ierd.domid == 0 || ierd.domid >= DOMID_FIRST_RESERVED)
+			break;
+
+		u->restrict_domid = ierd.domid;
+		rc = 0;
+
+		break;
+	}
+
 	default:
 		rc = -ENOSYS;
 		break;
@@ -601,6 +639,8 @@ static int evtchn_open(struct inode *inode, struct file *filp)
 	mutex_init(&u->ring_cons_mutex);
 	spin_lock_init(&u->ring_prod_lock);
 
+	u->restrict_domid = UNRESTRICTED_DOMID;
+
 	filp->private_data = u;
 
 	return nonseekable_open(inode, filp);
diff --git a/include/uapi/xen/evtchn.h b/include/uapi/xen/evtchn.h
index 14e833ee4..f057b53 100644
--- a/include/uapi/xen/evtchn.h
+++ b/include/uapi/xen/evtchn.h
@@ -85,4 +85,19 @@ struct ioctl_evtchn_notify {
 #define IOCTL_EVTCHN_RESET				\
 	_IOC(_IOC_NONE, 'E', 5, 0)
 
+/*
+ * Restrict this file descriptor so that it can only be used to bind
+ * new interdomain events from one domain.
+ *
+ * Once a file descriptor has been restricted it cannot be
+ * de-restricted, and must be closed and re-opened.  Event channels
+ * which were bound before restricting remain bound afterwards, and
+ * can be notified as usual.
+ */
+#define IOCTL_EVTCHN_RESTRICT_DOMID			\
+	_IOC(_IOC_NONE, 'E', 100, sizeof(struct ioctl_evtchn_restrict_domid))
+struct ioctl_evtchn_restrict_domid {
+	domid_t domid;
+};
+
 #endif /* __LINUX_PUBLIC_EVTCHN_H__ */
-- 
2.1.4


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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCHv1] xen/evtchn: add IOCTL_EVTCHN_RESTRICT
  2016-07-11 14:57 [PATCHv1] xen/evtchn: add IOCTL_EVTCHN_RESTRICT David Vrabel
@ 2016-07-11 15:31 ` Boris Ostrovsky
  2016-07-11 16:15   ` David Vrabel
  2016-08-01 11:15 ` Jan Beulich
  1 sibling, 1 reply; 8+ messages in thread
From: Boris Ostrovsky @ 2016-07-11 15:31 UTC (permalink / raw)
  To: David Vrabel, xen-devel; +Cc: Juergen Gross, Jennifer Herbert, Ian Jackson

On 07/11/2016 10:57 AM, David Vrabel wrote:
> diff --git a/include/uapi/xen/evtchn.h b/include/uapi/xen/evtchn.h
> index 14e833ee4..f057b53 100644
> --- a/include/uapi/xen/evtchn.h
> +++ b/include/uapi/xen/evtchn.h
> @@ -85,4 +85,19 @@ struct ioctl_evtchn_notify {
>  #define IOCTL_EVTCHN_RESET				\
>  	_IOC(_IOC_NONE, 'E', 5, 0)
>  
> +/*
> + * Restrict this file descriptor so that it can only be used to bind
> + * new interdomain events from one domain.
> + *
> + * Once a file descriptor has been restricted it cannot be
> + * de-restricted, and must be closed and re-opened.  Event channels
> + * which were bound before restricting remain bound afterwards, and
> + * can be notified as usual.
> + */
> +#define IOCTL_EVTCHN_RESTRICT_DOMID			\
> +	_IOC(_IOC_NONE, 'E', 100, sizeof(struct ioctl_evtchn_restrict_domid))

Is there a reason why you picked 100 and not 6?

-boris



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv1] xen/evtchn: add IOCTL_EVTCHN_RESTRICT
  2016-07-11 15:31 ` Boris Ostrovsky
@ 2016-07-11 16:15   ` David Vrabel
  2016-07-11 16:33     ` Andrew Cooper
  0 siblings, 1 reply; 8+ messages in thread
From: David Vrabel @ 2016-07-11 16:15 UTC (permalink / raw)
  To: Boris Ostrovsky, xen-devel; +Cc: Juergen Gross, Jennifer Herbert, Ian Jackson

On 11/07/16 16:31, Boris Ostrovsky wrote:
> On 07/11/2016 10:57 AM, David Vrabel wrote:
>> diff --git a/include/uapi/xen/evtchn.h b/include/uapi/xen/evtchn.h
>> index 14e833ee4..f057b53 100644
>> --- a/include/uapi/xen/evtchn.h
>> +++ b/include/uapi/xen/evtchn.h
>> @@ -85,4 +85,19 @@ struct ioctl_evtchn_notify {
>>  #define IOCTL_EVTCHN_RESET				\
>>  	_IOC(_IOC_NONE, 'E', 5, 0)
>>  
>> +/*
>> + * Restrict this file descriptor so that it can only be used to bind
>> + * new interdomain events from one domain.
>> + *
>> + * Once a file descriptor has been restricted it cannot be
>> + * de-restricted, and must be closed and re-opened.  Event channels
>> + * which were bound before restricting remain bound afterwards, and
>> + * can be notified as usual.
>> + */
>> +#define IOCTL_EVTCHN_RESTRICT_DOMID			\
>> +	_IOC(_IOC_NONE, 'E', 100, sizeof(struct ioctl_evtchn_restrict_domid))
> 
> Is there a reason why you picked 100 and not 6?

Because we've had this patch for years in xenserver like this and I
didn't see any need to change the ABI.  But if it's preferred I can make
this 6 (and manage the transition internally).

David

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv1] xen/evtchn: add IOCTL_EVTCHN_RESTRICT
  2016-07-11 16:15   ` David Vrabel
@ 2016-07-11 16:33     ` Andrew Cooper
  2016-07-11 16:44       ` David Vrabel
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Cooper @ 2016-07-11 16:33 UTC (permalink / raw)
  To: David Vrabel, Boris Ostrovsky, xen-devel
  Cc: Juergen Gross, Ian Jackson, Jennifer Herbert

On 11/07/16 17:15, David Vrabel wrote:
> On 11/07/16 16:31, Boris Ostrovsky wrote:
>> On 07/11/2016 10:57 AM, David Vrabel wrote:
>>> diff --git a/include/uapi/xen/evtchn.h b/include/uapi/xen/evtchn.h
>>> index 14e833ee4..f057b53 100644
>>> --- a/include/uapi/xen/evtchn.h
>>> +++ b/include/uapi/xen/evtchn.h
>>> @@ -85,4 +85,19 @@ struct ioctl_evtchn_notify {
>>>  #define IOCTL_EVTCHN_RESET				\
>>>  	_IOC(_IOC_NONE, 'E', 5, 0)
>>>  
>>> +/*
>>> + * Restrict this file descriptor so that it can only be used to bind
>>> + * new interdomain events from one domain.
>>> + *
>>> + * Once a file descriptor has been restricted it cannot be
>>> + * de-restricted, and must be closed and re-opened.  Event channels
>>> + * which were bound before restricting remain bound afterwards, and
>>> + * can be notified as usual.
>>> + */
>>> +#define IOCTL_EVTCHN_RESTRICT_DOMID			\
>>> +	_IOC(_IOC_NONE, 'E', 100, sizeof(struct ioctl_evtchn_restrict_domid))
>> Is there a reason why you picked 100 and not 6?
> Because we've had this patch for years in xenserver like this and I
> didn't see any need to change the ABI.  But if it's preferred I can make
> this 6 (and manage the transition internally).

This should become 6, and we manage the transition.  It is not like its
hard to manage.

~Andrew

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv1] xen/evtchn: add IOCTL_EVTCHN_RESTRICT
  2016-07-11 16:33     ` Andrew Cooper
@ 2016-07-11 16:44       ` David Vrabel
  2016-07-11 16:55         ` Boris Ostrovsky
  0 siblings, 1 reply; 8+ messages in thread
From: David Vrabel @ 2016-07-11 16:44 UTC (permalink / raw)
  To: Andrew Cooper, David Vrabel, Boris Ostrovsky, xen-devel
  Cc: Juergen Gross, Jennifer Herbert, Ian Jackson

On 11/07/16 17:33, Andrew Cooper wrote:
> On 11/07/16 17:15, David Vrabel wrote:
>> On 11/07/16 16:31, Boris Ostrovsky wrote:
>>> On 07/11/2016 10:57 AM, David Vrabel wrote:
>>>> diff --git a/include/uapi/xen/evtchn.h b/include/uapi/xen/evtchn.h
>>>> index 14e833ee4..f057b53 100644
>>>> --- a/include/uapi/xen/evtchn.h
>>>> +++ b/include/uapi/xen/evtchn.h
>>>> @@ -85,4 +85,19 @@ struct ioctl_evtchn_notify {
>>>>  #define IOCTL_EVTCHN_RESET				\
>>>>  	_IOC(_IOC_NONE, 'E', 5, 0)
>>>>  
>>>> +/*
>>>> + * Restrict this file descriptor so that it can only be used to bind
>>>> + * new interdomain events from one domain.
>>>> + *
>>>> + * Once a file descriptor has been restricted it cannot be
>>>> + * de-restricted, and must be closed and re-opened.  Event channels
>>>> + * which were bound before restricting remain bound afterwards, and
>>>> + * can be notified as usual.
>>>> + */
>>>> +#define IOCTL_EVTCHN_RESTRICT_DOMID			\
>>>> +	_IOC(_IOC_NONE, 'E', 100, sizeof(struct ioctl_evtchn_restrict_domid))
>>> Is there a reason why you picked 100 and not 6?
>> Because we've had this patch for years in xenserver like this and I
>> didn't see any need to change the ABI.  But if it's preferred I can make
>> this 6 (and manage the transition internally).
> 
> This should become 6, and we manage the transition.  It is not like its
> hard to manage.

Ok.

David

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv1] xen/evtchn: add IOCTL_EVTCHN_RESTRICT
  2016-07-11 16:44       ` David Vrabel
@ 2016-07-11 16:55         ` Boris Ostrovsky
  0 siblings, 0 replies; 8+ messages in thread
From: Boris Ostrovsky @ 2016-07-11 16:55 UTC (permalink / raw)
  To: David Vrabel, Andrew Cooper, xen-devel
  Cc: Juergen Gross, Jennifer Herbert, Ian Jackson

On 07/11/2016 12:44 PM, David Vrabel wrote:
> On 11/07/16 17:33, Andrew Cooper wrote:
>> On 11/07/16 17:15, David Vrabel wrote:
>>> On 11/07/16 16:31, Boris Ostrovsky wrote:
>>>> On 07/11/2016 10:57 AM, David Vrabel wrote:
>>>>> diff --git a/include/uapi/xen/evtchn.h b/include/uapi/xen/evtchn.h
>>>>> index 14e833ee4..f057b53 100644
>>>>> --- a/include/uapi/xen/evtchn.h
>>>>> +++ b/include/uapi/xen/evtchn.h
>>>>> @@ -85,4 +85,19 @@ struct ioctl_evtchn_notify {
>>>>>  #define IOCTL_EVTCHN_RESET				\
>>>>>  	_IOC(_IOC_NONE, 'E', 5, 0)
>>>>>  
>>>>> +/*
>>>>> + * Restrict this file descriptor so that it can only be used to bind
>>>>> + * new interdomain events from one domain.
>>>>> + *
>>>>> + * Once a file descriptor has been restricted it cannot be
>>>>> + * de-restricted, and must be closed and re-opened.  Event channels
>>>>> + * which were bound before restricting remain bound afterwards, and
>>>>> + * can be notified as usual.
>>>>> + */
>>>>> +#define IOCTL_EVTCHN_RESTRICT_DOMID			\
>>>>> +	_IOC(_IOC_NONE, 'E', 100, sizeof(struct ioctl_evtchn_restrict_domid))
>>>> Is there a reason why you picked 100 and not 6?
>>> Because we've had this patch for years in xenserver like this and I
>>> didn't see any need to change the ABI.  But if it's preferred I can make
>>> this 6 (and manage the transition internally).
>> This should become 6, and we manage the transition.  It is not like its
>> hard to manage.
> Ok.

With that

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>




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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv1] xen/evtchn: add IOCTL_EVTCHN_RESTRICT
  2016-07-11 14:57 [PATCHv1] xen/evtchn: add IOCTL_EVTCHN_RESTRICT David Vrabel
  2016-07-11 15:31 ` Boris Ostrovsky
@ 2016-08-01 11:15 ` Jan Beulich
  2016-08-01 12:58   ` David Vrabel
  1 sibling, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2016-08-01 11:15 UTC (permalink / raw)
  To: David Vrabel
  Cc: Juergen Gross, Ian Jackson, Boris Ostrovsky, Jennifer Herbert, xen-devel

>>> On 11.07.16 at 16:57, <david.vrabel@citrix.com> wrote:
> @@ -553,6 +570,27 @@ static long evtchn_ioctl(struct file *file,
>  		break;
>  	}
>  
> +	case IOCTL_EVTCHN_RESTRICT_DOMID: {
> +		struct ioctl_evtchn_restrict_domid ierd;
> +
> +		rc = -EACCES;
> +		if (u->restrict_domid != UNRESTRICTED_DOMID)
> +			break;
> +
> +		rc = -EFAULT;
> +		if (copy_from_user(&ierd, uarg, sizeof(ierd)))
> +		    break;
> +
> +		rc = -EINVAL;
> +		if (ierd.domid == 0 || ierd.domid >= DOMID_FIRST_RESERVED)
> +			break;

Any reason you special case Dom0 here, but not the conceptually
more general hardware and/or control domain?

Jan


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv1] xen/evtchn: add IOCTL_EVTCHN_RESTRICT
  2016-08-01 11:15 ` Jan Beulich
@ 2016-08-01 12:58   ` David Vrabel
  0 siblings, 0 replies; 8+ messages in thread
From: David Vrabel @ 2016-08-01 12:58 UTC (permalink / raw)
  To: Jan Beulich, David Vrabel
  Cc: Juergen Gross, Jennifer Herbert, Boris Ostrovsky, Ian Jackson, xen-devel

On 01/08/16 12:15, Jan Beulich wrote:
>>>> On 11.07.16 at 16:57, <david.vrabel@citrix.com> wrote:
>> @@ -553,6 +570,27 @@ static long evtchn_ioctl(struct file *file,
>>  		break;
>>  	}
>>  
>> +	case IOCTL_EVTCHN_RESTRICT_DOMID: {
>> +		struct ioctl_evtchn_restrict_domid ierd;
>> +
>> +		rc = -EACCES;
>> +		if (u->restrict_domid != UNRESTRICTED_DOMID)
>> +			break;
>> +
>> +		rc = -EFAULT;
>> +		if (copy_from_user(&ierd, uarg, sizeof(ierd)))
>> +		    break;
>> +
>> +		rc = -EINVAL;
>> +		if (ierd.domid == 0 || ierd.domid >= DOMID_FIRST_RESERVED)
>> +			break;
> 
> Any reason you special case Dom0 here, but not the conceptually
> more general hardware and/or control domain?

I don't know why that check is there.  I think it can be removed.

David

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-08-01 12:58 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-11 14:57 [PATCHv1] xen/evtchn: add IOCTL_EVTCHN_RESTRICT David Vrabel
2016-07-11 15:31 ` Boris Ostrovsky
2016-07-11 16:15   ` David Vrabel
2016-07-11 16:33     ` Andrew Cooper
2016-07-11 16:44       ` David Vrabel
2016-07-11 16:55         ` Boris Ostrovsky
2016-08-01 11:15 ` Jan Beulich
2016-08-01 12:58   ` David Vrabel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).