[RFC PATCH 0/2] Fix some multiqueue TUN problems

[RFC PATCH 0/2] Fix some multiqueue TUN problems

[Paul Moore]

A relatively short patchset to fix some problems that have arisen from
the multiqueue TUN work.  I'm sending this a RFC for now as it changes
the LSM TUN interfaces and therefore crosses a few boundaries,
e.g. SELinux, although it does so only in an effort to restore the
original behavior which was lost in the multiqueue conversion.

I'm not particularly enthused with the idea of passing a void** as a
parameter to security_tun_dev_alloc_security() but the alternatives
would be to move the tun_struct out of tun.c or have the LSM interface
return a pointer (which would require us to return a "fake" non-NULL
pointer when the LSM was disabled) ... neither of these seemed like good
alternatives to me.

I've compiled the code and booted it without drama, but I haven't really
stressed it too much so buyer beware at this point.  Comments are always
welcome ...

---

Paul Moore (2):
      tun: correctly report an error in tun_flow_init()
      tun: fix LSM/SELinux labeling of tun/tap devices


 drivers/net/tun.c                 |   16 ++++++++++----
 include/linux/security.h          |   37 ++++++++++++++++++++++-----------
 security/capability.c             |   14 +++++++++---
 security/security.c               |   22 ++++++++++++-------
 security/selinux/hooks.c          |   42 +++++++++++++++++++++++--------------
 security/selinux/include/objsec.h |    4 ++++
 6 files changed, 90 insertions(+), 45 deletions(-)

[RFC PATCH 1/2] tun: correctly report an error in tun_flow_init()

[Paul Moore]

On error, the error code from tun_flow_init() is lost inside
tun_set_iff(), this patch fixes this by assigning the tun_flow_init()
error code to the "err" variable which is returned by
the tun_flow_init() function on error.

Signed-off-by: Paul Moore <pmoore@redhat.com>
---
 drivers/net/tun.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 607a3a5..877ffe2 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1605,7 +1605,8 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 
 		tun_net_init(dev);
 
-		if (tun_flow_init(tun))
+		err = tun_flow_init(tun);
+		if (err < 0)
 			goto err_free_dev;
 
 		dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST |

Re: [RFC PATCH 1/2] tun: correctly report an error in tun_flow_init()

[Paul Moore]

On Thursday, November 29, 2012 05:06:29 PM Paul Moore wrote:
> On error, the error code from tun_flow_init() is lost inside
> tun_set_iff(), this patch fixes this by assigning the tun_flow_init()
> error code to the "err" variable which is returned by
> the tun_flow_init() function on error.
> 
> Signed-off-by: Paul Moore <pmoore@redhat.com>

Jason, we've had some good discussion around patch 2/2 but nothing on this 
fix; can I assume you are okay with this patch?  If so I think we should go 
ahead and apply this ...

> ---
>  drivers/net/tun.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 607a3a5..877ffe2 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1605,7 +1605,8 @@ static int tun_set_iff(struct net *net, struct file
> *file, struct ifreq *ifr)
> 
>  		tun_net_init(dev);
> 
> -		if (tun_flow_init(tun))
> +		err = tun_flow_init(tun);
> +		if (err < 0)
>  			goto err_free_dev;
> 
>  		dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST |
-- 
paul moore
security and virtualization @ redhat

Re: [RFC PATCH 1/2] tun: correctly report an error in tun_flow_init()

[Jason Wang]

On Wednesday, December 05, 2012 11:02:04 AM Paul Moore wrote:
> On Thursday, November 29, 2012 05:06:29 PM Paul Moore wrote:
> > On error, the error code from tun_flow_init() is lost inside
> > tun_set_iff(), this patch fixes this by assigning the tun_flow_init()
> > error code to the "err" variable which is returned by
> > the tun_flow_init() function on error.
> > 
> > Signed-off-by: Paul Moore <pmoore@redhat.com>
> 
> Jason, we've had some good discussion around patch 2/2 but nothing on this
> fix; can I assume you are okay with this patch?  If so I think we should go
> ahead and apply this ...

Yes, it looks good. Maybe we can separate this patch from this RFC series and 
tag it as "net-next" to let David apply it soon.

Thanks
> 
> > ---
> > 
> >  drivers/net/tun.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 607a3a5..877ffe2 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1605,7 +1605,8 @@ static int tun_set_iff(struct net *net, struct file
> > *file, struct ifreq *ifr)
> > 
> >  		tun_net_init(dev);
> > 
> > -		if (tun_flow_init(tun))
> > +		err = tun_flow_init(tun);
> > +		if (err < 0)
> > 
> >  			goto err_free_dev;
> >  		
> >  		dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST |

[RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Paul Moore]

This patch corrects some problems with LSM/SELinux that were introduced
with the multiqueue patchset.  The problem stems from the fact that the
multiqueue work changed the relationship between the tun device and its
associated socket; before the socket persisted for the life of the
device, however after the multiqueue changes the socket only persisted
for the life of the userspace connection (fd open).  For non-persistent
devices this is not an issue, but for persistent devices this can cause
the tun device to lose its SELinux label.

We correct this problem by adding an opaque LSM security blob to the
tun device struct which allows us to have the LSM security state, e.g.
SELinux labeling information, persist for the lifetime of the tun
device.

Signed-off-by: Paul Moore <pmoore@redhat.com>
---
 drivers/net/tun.c                 |   13 ++++++++---
 include/linux/security.h          |   37 ++++++++++++++++++++++-----------
 security/capability.c             |   14 +++++++++---
 security/security.c               |   22 ++++++++++++-------
 security/selinux/hooks.c          |   42 +++++++++++++++++++++++--------------
 security/selinux/include/objsec.h |    4 ++++
 6 files changed, 88 insertions(+), 44 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 877ffe2..85cc924 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -182,6 +182,7 @@ struct tun_struct {
 	struct hlist_head flows[TUN_NUM_FLOW_ENTRIES];
 	struct timer_list flow_gc_timer;
 	unsigned long ageing_time;
+	void *security;
 };
 
 static inline u32 tun_hashfn(u32 rxhash)
@@ -465,6 +466,10 @@ static int tun_attach(struct tun_struct *tun, struct file *file)
 	struct tun_file *tfile = file->private_data;
 	int err;
 
+	err = security_tun_dev_attach(tfile->socket.sk, tun->security);
+	if (err < 0)
+		goto out;
+
 	err = -EINVAL;
 	if (rcu_dereference_protected(tfile->tun, lockdep_rtnl_is_held()))
 		goto out;
@@ -1365,6 +1370,7 @@ static void tun_free_netdev(struct net_device *dev)
 	struct tun_struct *tun = netdev_priv(dev);
 
 	tun_flow_uninit(tun);
+	security_tun_dev_free_security(tun->security);
 	free_netdev(dev);
 }
 
@@ -1548,9 +1554,6 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 
 		if (tun_not_capable(tun))
 			return -EPERM;
-		err = security_tun_dev_attach(tfile->socket.sk);
-		if (err < 0)
-			return err;
 
 		err = tun_attach(tun, file);
 		if (err < 0)
@@ -1601,7 +1604,9 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 
 		spin_lock_init(&tun->lock);
 
-		security_tun_dev_post_create(&tfile->sk);
+		err = security_tun_dev_alloc_security(&tun->security);
+		if (err < 0)
+			goto err_free_dev;
 
 		tun_net_init(dev);
 
diff --git a/include/linux/security.h b/include/linux/security.h
index 05e88bd..260e151 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -983,17 +983,23 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	tells the LSM to decrement the number of secmark labeling rules loaded
  * @req_classify_flow:
  *	Sets the flow's sid to the openreq sid.
+ * @tun_dev_alloc_security:
+ *	This hook allows a module to allocate a security structure for a TUN
+ *	device.
+ *	@security pointer to a security structure pointer.
+ *	Returns a zero on success, negative values on failure.
+ * @tun_dev_free_security:
+ *	This hook allows a module to free the security structure for a TUN
+ *	device.
+ *	@security pointer to the TUN device's security structure
  * @tun_dev_create:
  *	Check permissions prior to creating a new TUN device.
- * @tun_dev_post_create:
- *	This hook allows a module to update or allocate a per-socket security
- *	structure.
- *	@sk contains the newly created sock structure.
  * @tun_dev_attach:
  *	Check permissions prior to attaching to a persistent TUN device.  This
  *	hook can also be used by the module to update any security state
  *	associated with the TUN device's sock structure.
  *	@sk contains the existing sock structure.
+ *	@security pointer to the TUN device's security structure.
  *
  * Security hooks for XFRM operations.
  *
@@ -1613,9 +1619,10 @@ struct security_operations {
 	void (*secmark_refcount_inc) (void);
 	void (*secmark_refcount_dec) (void);
 	void (*req_classify_flow) (const struct request_sock *req, struct flowi *fl);
-	int (*tun_dev_create)(void);
-	void (*tun_dev_post_create)(struct sock *sk);
-	int (*tun_dev_attach)(struct sock *sk);
+	int (*tun_dev_alloc_security) (void **security);
+	void (*tun_dev_free_security) (void *security);
+	int (*tun_dev_create) (void);
+	int (*tun_dev_attach) (struct sock *sk, void *security);
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
@@ -2553,9 +2560,10 @@ void security_inet_conn_established(struct sock *sk,
 int security_secmark_relabel_packet(u32 secid);
 void security_secmark_refcount_inc(void);
 void security_secmark_refcount_dec(void);
+int security_tun_dev_alloc_security(void **security);
+void security_tun_dev_free_security(void *security);
 int security_tun_dev_create(void);
-void security_tun_dev_post_create(struct sock *sk);
-int security_tun_dev_attach(struct sock *sk);
+int security_tun_dev_attach(struct sock *sk, void *security);
 
 #else	/* CONFIG_SECURITY_NETWORK */
 static inline int security_unix_stream_connect(struct sock *sock,
@@ -2720,16 +2728,21 @@ static inline void security_secmark_refcount_dec(void)
 {
 }
 
-static inline int security_tun_dev_create(void)
+static inline int security_tun_dev_alloc_security(void **security)
 {
 	return 0;
 }
 
-static inline void security_tun_dev_post_create(struct sock *sk)
+static inline void security_tun_dev_free_security(void *security)
 {
 }
 
-static inline int security_tun_dev_attach(struct sock *sk)
+static inline int security_tun_dev_create(void)
+{
+	return 0;
+}
+
+static inline int security_tun_dev_attach(struct sock *sk, void *security)
 {
 	return 0;
 }
diff --git a/security/capability.c b/security/capability.c
index b14a30c..fd6e2dc 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -704,16 +704,21 @@ static void cap_req_classify_flow(const struct request_sock *req,
 {
 }
 
-static int cap_tun_dev_create(void)
+static int cap_tun_dev_alloc_security(void **security)
 {
 	return 0;
 }
 
-static void cap_tun_dev_post_create(struct sock *sk)
+static void cap_tun_dev_free_security(void *security)
+{
+}
+
+static int cap_tun_dev_create(void)
 {
+	return 0;
 }
 
-static int cap_tun_dev_attach(struct sock *sk)
+static int cap_tun_dev_attach(struct sock *sk, void *security)
 {
 	return 0;
 }
@@ -1044,8 +1049,9 @@ void __init security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, secmark_refcount_inc);
 	set_to_cap_if_null(ops, secmark_refcount_dec);
 	set_to_cap_if_null(ops, req_classify_flow);
+	set_to_cap_if_null(ops, tun_dev_alloc_security);
+	set_to_cap_if_null(ops, tun_dev_free_security);
 	set_to_cap_if_null(ops, tun_dev_create);
-	set_to_cap_if_null(ops, tun_dev_post_create);
 	set_to_cap_if_null(ops, tun_dev_attach);
 #endif	/* CONFIG_SECURITY_NETWORK */
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
diff --git a/security/security.c b/security/security.c
index 8dcd4ae..613ad36 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1244,21 +1244,27 @@ void security_secmark_refcount_dec(void)
 }
 EXPORT_SYMBOL(security_secmark_refcount_dec);
 
-int security_tun_dev_create(void)
+int security_tun_dev_alloc_security(void **security)
 {
-	return security_ops->tun_dev_create();
+	return security_ops->tun_dev_alloc_security(security);
 }
-EXPORT_SYMBOL(security_tun_dev_create);
+EXPORT_SYMBOL(security_tun_dev_alloc_security);
 
-void security_tun_dev_post_create(struct sock *sk)
+void security_tun_dev_free_security(void *security)
 {
-	return security_ops->tun_dev_post_create(sk);
+	security_ops->tun_dev_free_security(security);
 }
-EXPORT_SYMBOL(security_tun_dev_post_create);
+EXPORT_SYMBOL(security_tun_dev_free_security);
+
+int security_tun_dev_create(void)
+{
+	return security_ops->tun_dev_create();
+}
+EXPORT_SYMBOL(security_tun_dev_create);
 
-int security_tun_dev_attach(struct sock *sk)
+int security_tun_dev_attach(struct sock *sk, void *security)
 {
-	return security_ops->tun_dev_attach(sk);
+	return security_ops->tun_dev_attach(sk, security);
 }
 EXPORT_SYMBOL(security_tun_dev_attach);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 61a5336..67b3423 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4414,40 +4414,49 @@ static int selinux_tun_dev_create(void)
 			    NULL);
 }
 
-static void selinux_tun_dev_post_create(struct sock *sk)
+static int selinux_tun_dev_alloc_security(void **security)
 {
-	struct sk_security_struct *sksec = sk->sk_security;
+	struct tun_security_struct *tunsec;
 
-	/* we don't currently perform any NetLabel based labeling here and it
-	 * isn't clear that we would want to do so anyway; while we could apply
-	 * labeling without the support of the TUN user the resulting labeled
-	 * traffic from the other end of the connection would almost certainly
-	 * cause confusion to the TUN user that had no idea network labeling
-	 * protocols were being used */
+	tunsec = kzalloc(sizeof(*tunsec), GFP_KERNEL);
+	if (!tunsec)
+		return -ENOMEM;
+	tunsec->sid = current_sid();
 
-	/* see the comments in selinux_tun_dev_create() about why we don't use
-	 * the sockcreate SID here */
+	*security = tunsec;
+	return 0;
+}
 
-	sksec->sid = current_sid();
-	sksec->sclass = SECCLASS_TUN_SOCKET;
+static void selinux_tun_dev_free_security(void *security)
+{
+	kfree(security);
 }
 
-static int selinux_tun_dev_attach(struct sock *sk)
+static int selinux_tun_dev_attach(struct sock *sk, void *security)
 {
+	struct tun_security_struct *tunsec = security;
 	struct sk_security_struct *sksec = sk->sk_security;
 	u32 sid = current_sid();
 	int err;
 
+	/* we don't currently perform any NetLabel based labeling here and it
+	 * isn't clear that we would want to do so anyway; while we could apply
+	 * labeling without the support of the TUN user the resulting labeled
+	 * traffic from the other end of the connection would almost certainly
+	 * cause confusion to the TUN user that had no idea network labeling
+	 * protocols were being used */
+
 	err = avc_has_perm(sid, sksec->sid, SECCLASS_TUN_SOCKET,
 			   TUN_SOCKET__RELABELFROM, NULL);
 	if (err)
 		return err;
-	err = avc_has_perm(sid, sid, SECCLASS_TUN_SOCKET,
+	err = avc_has_perm(sid, tunsec->sid, SECCLASS_TUN_SOCKET,
 			   TUN_SOCKET__RELABELTO, NULL);
 	if (err)
 		return err;
 
-	sksec->sid = sid;
+	sksec->sid = tunsec->sid;
+	sksec->sclass = SECCLASS_TUN_SOCKET;
 
 	return 0;
 }
@@ -5642,8 +5651,9 @@ static struct security_operations selinux_ops = {
 	.secmark_refcount_inc =		selinux_secmark_refcount_inc,
 	.secmark_refcount_dec =		selinux_secmark_refcount_dec,
 	.req_classify_flow =		selinux_req_classify_flow,
+	.tun_dev_alloc_security =	selinux_tun_dev_alloc_security,
+	.tun_dev_free_security =	selinux_tun_dev_free_security,
 	.tun_dev_create =		selinux_tun_dev_create,
-	.tun_dev_post_create = 		selinux_tun_dev_post_create,
 	.tun_dev_attach =		selinux_tun_dev_attach,
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 26c7eee..aa47bca 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -110,6 +110,10 @@ struct sk_security_struct {
 	u16 sclass;			/* sock security class */
 };
 
+struct tun_security_struct {
+	u32 sid;			/* SID for the tun device sockets */
+};
+
 struct key_security_struct {
 	u32 sid;	/* SID of key */
 };

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Jason Wang]

On 11/30/2012 06:06 AM, Paul Moore wrote:
> This patch corrects some problems with LSM/SELinux that were introduced
> with the multiqueue patchset.  The problem stems from the fact that the
> multiqueue work changed the relationship between the tun device and its
> associated socket; before the socket persisted for the life of the
> device, however after the multiqueue changes the socket only persisted
> for the life of the userspace connection (fd open).  For non-persistent
> devices this is not an issue, but for persistent devices this can cause
> the tun device to lose its SELinux label.
>
> We correct this problem by adding an opaque LSM security blob to the
> tun device struct which allows us to have the LSM security state, e.g.
> SELinux labeling information, persist for the lifetime of the tun
> device.

Hi Paul, thanks for the patchset. I've one question, see below.

>
> Signed-off-by: Paul Moore <pmoore@redhat.com>
> ---
>  drivers/net/tun.c                 |   13 ++++++++---
>  include/linux/security.h          |   37 ++++++++++++++++++++++-----------
>  security/capability.c             |   14 +++++++++---
>  security/security.c               |   22 ++++++++++++-------
>  security/selinux/hooks.c          |   42 +++++++++++++++++++++++--------------
>  security/selinux/include/objsec.h |    4 ++++
>  6 files changed, 88 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 877ffe2..85cc924 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -182,6 +182,7 @@ struct tun_struct {
>  	struct hlist_head flows[TUN_NUM_FLOW_ENTRIES];
>  	struct timer_list flow_gc_timer;
>  	unsigned long ageing_time;
> +	void *security;
>  };
>  
>  static inline u32 tun_hashfn(u32 rxhash)
> @@ -465,6 +466,10 @@ static int tun_attach(struct tun_struct *tun, struct file *file)
>  	struct tun_file *tfile = file->private_data;
>  	int err;
>  
> +	err = security_tun_dev_attach(tfile->socket.sk, tun->security);
> +	if (err < 0)
> +		goto out;
> +
>  	err = -EINVAL;
>  	if (rcu_dereference_protected(tfile->tun, lockdep_rtnl_is_held()))
>  		goto out;
> @@ -1365,6 +1370,7 @@ static void tun_free_netdev(struct net_device *dev)
>  	struct tun_struct *tun = netdev_priv(dev);
>  
>  	tun_flow_uninit(tun);
> +	security_tun_dev_free_security(tun->security);
>  	free_netdev(dev);
>  }
>  
> @@ -1548,9 +1554,6 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  
>  		if (tun_not_capable(tun))
>  			return -EPERM;
> -		err = security_tun_dev_attach(tfile->socket.sk);
> -		if (err < 0)
> -			return err;
>  
>  		err = tun_attach(tun, file);
>  		if (err < 0)
> @@ -1601,7 +1604,9 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  
>  		spin_lock_init(&tun->lock);
>  
> -		security_tun_dev_post_create(&tfile->sk);
> +		err = security_tun_dev_alloc_security(&tun->security);
> +		if (err < 0)
> +			goto err_free_dev;
>  
>  		tun_net_init(dev);
>  
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 05e88bd..260e151 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -983,17 +983,23 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>   *	tells the LSM to decrement the number of secmark labeling rules loaded
>   * @req_classify_flow:
>   *	Sets the flow's sid to the openreq sid.
> + * @tun_dev_alloc_security:
> + *	This hook allows a module to allocate a security structure for a TUN
> + *	device.
> + *	@security pointer to a security structure pointer.
> + *	Returns a zero on success, negative values on failure.
> + * @tun_dev_free_security:
> + *	This hook allows a module to free the security structure for a TUN
> + *	device.
> + *	@security pointer to the TUN device's security structure
>   * @tun_dev_create:
>   *	Check permissions prior to creating a new TUN device.
> - * @tun_dev_post_create:
> - *	This hook allows a module to update or allocate a per-socket security
> - *	structure.
> - *	@sk contains the newly created sock structure.
>   * @tun_dev_attach:
>   *	Check permissions prior to attaching to a persistent TUN device.  This
>   *	hook can also be used by the module to update any security state
>   *	associated with the TUN device's sock structure.
>   *	@sk contains the existing sock structure.
> + *	@security pointer to the TUN device's security structure.
>   *
>   * Security hooks for XFRM operations.
>   *
> @@ -1613,9 +1619,10 @@ struct security_operations {
>  	void (*secmark_refcount_inc) (void);
>  	void (*secmark_refcount_dec) (void);
>  	void (*req_classify_flow) (const struct request_sock *req, struct flowi *fl);
> -	int (*tun_dev_create)(void);
> -	void (*tun_dev_post_create)(struct sock *sk);
> -	int (*tun_dev_attach)(struct sock *sk);
> +	int (*tun_dev_alloc_security) (void **security);
> +	void (*tun_dev_free_security) (void *security);
> +	int (*tun_dev_create) (void);
> +	int (*tun_dev_attach) (struct sock *sk, void *security);
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> @@ -2553,9 +2560,10 @@ void security_inet_conn_established(struct sock *sk,
>  int security_secmark_relabel_packet(u32 secid);
>  void security_secmark_refcount_inc(void);
>  void security_secmark_refcount_dec(void);
> +int security_tun_dev_alloc_security(void **security);
> +void security_tun_dev_free_security(void *security);
>  int security_tun_dev_create(void);
> -void security_tun_dev_post_create(struct sock *sk);
> -int security_tun_dev_attach(struct sock *sk);
> +int security_tun_dev_attach(struct sock *sk, void *security);
>  
>  #else	/* CONFIG_SECURITY_NETWORK */
>  static inline int security_unix_stream_connect(struct sock *sock,
> @@ -2720,16 +2728,21 @@ static inline void security_secmark_refcount_dec(void)
>  {
>  }
>  
> -static inline int security_tun_dev_create(void)
> +static inline int security_tun_dev_alloc_security(void **security)
>  {
>  	return 0;
>  }
>  
> -static inline void security_tun_dev_post_create(struct sock *sk)
> +static inline void security_tun_dev_free_security(void *security)
>  {
>  }
>  
> -static inline int security_tun_dev_attach(struct sock *sk)
> +static inline int security_tun_dev_create(void)
> +{
> +	return 0;
> +}
> +
> +static inline int security_tun_dev_attach(struct sock *sk, void *security)
>  {
>  	return 0;
>  }
> diff --git a/security/capability.c b/security/capability.c
> index b14a30c..fd6e2dc 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -704,16 +704,21 @@ static void cap_req_classify_flow(const struct request_sock *req,
>  {
>  }
>  
> -static int cap_tun_dev_create(void)
> +static int cap_tun_dev_alloc_security(void **security)
>  {
>  	return 0;
>  }
>  
> -static void cap_tun_dev_post_create(struct sock *sk)
> +static void cap_tun_dev_free_security(void *security)
> +{
> +}
> +
> +static int cap_tun_dev_create(void)
>  {
> +	return 0;
>  }
>  
> -static int cap_tun_dev_attach(struct sock *sk)
> +static int cap_tun_dev_attach(struct sock *sk, void *security)
>  {
>  	return 0;
>  }
> @@ -1044,8 +1049,9 @@ void __init security_fixup_ops(struct security_operations *ops)
>  	set_to_cap_if_null(ops, secmark_refcount_inc);
>  	set_to_cap_if_null(ops, secmark_refcount_dec);
>  	set_to_cap_if_null(ops, req_classify_flow);
> +	set_to_cap_if_null(ops, tun_dev_alloc_security);
> +	set_to_cap_if_null(ops, tun_dev_free_security);
>  	set_to_cap_if_null(ops, tun_dev_create);
> -	set_to_cap_if_null(ops, tun_dev_post_create);
>  	set_to_cap_if_null(ops, tun_dev_attach);
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> diff --git a/security/security.c b/security/security.c
> index 8dcd4ae..613ad36 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1244,21 +1244,27 @@ void security_secmark_refcount_dec(void)
>  }
>  EXPORT_SYMBOL(security_secmark_refcount_dec);
>  
> -int security_tun_dev_create(void)
> +int security_tun_dev_alloc_security(void **security)
>  {
> -	return security_ops->tun_dev_create();
> +	return security_ops->tun_dev_alloc_security(security);
>  }
> -EXPORT_SYMBOL(security_tun_dev_create);
> +EXPORT_SYMBOL(security_tun_dev_alloc_security);
>  
> -void security_tun_dev_post_create(struct sock *sk)
> +void security_tun_dev_free_security(void *security)
>  {
> -	return security_ops->tun_dev_post_create(sk);
> +	security_ops->tun_dev_free_security(security);
>  }
> -EXPORT_SYMBOL(security_tun_dev_post_create);
> +EXPORT_SYMBOL(security_tun_dev_free_security);
> +
> +int security_tun_dev_create(void)
> +{
> +	return security_ops->tun_dev_create();
> +}
> +EXPORT_SYMBOL(security_tun_dev_create);
>  
> -int security_tun_dev_attach(struct sock *sk)
> +int security_tun_dev_attach(struct sock *sk, void *security)
>  {
> -	return security_ops->tun_dev_attach(sk);
> +	return security_ops->tun_dev_attach(sk, security);
>  }
>  EXPORT_SYMBOL(security_tun_dev_attach);
>  
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 61a5336..67b3423 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4414,40 +4414,49 @@ static int selinux_tun_dev_create(void)
>  			    NULL);
>  }
>  
> -static void selinux_tun_dev_post_create(struct sock *sk)
> +static int selinux_tun_dev_alloc_security(void **security)
>  {
> -	struct sk_security_struct *sksec = sk->sk_security;
> +	struct tun_security_struct *tunsec;
>  
> -	/* we don't currently perform any NetLabel based labeling here and it
> -	 * isn't clear that we would want to do so anyway; while we could apply
> -	 * labeling without the support of the TUN user the resulting labeled
> -	 * traffic from the other end of the connection would almost certainly
> -	 * cause confusion to the TUN user that had no idea network labeling
> -	 * protocols were being used */
> +	tunsec = kzalloc(sizeof(*tunsec), GFP_KERNEL);
> +	if (!tunsec)
> +		return -ENOMEM;
> +	tunsec->sid = current_sid();
>  
> -	/* see the comments in selinux_tun_dev_create() about why we don't use
> -	 * the sockcreate SID here */
> +	*security = tunsec;
> +	return 0;
> +}
>  
> -	sksec->sid = current_sid();
> -	sksec->sclass = SECCLASS_TUN_SOCKET;
> +static void selinux_tun_dev_free_security(void *security)
> +{
> +	kfree(security);
>  }
>  
> -static int selinux_tun_dev_attach(struct sock *sk)
> +static int selinux_tun_dev_attach(struct sock *sk, void *security)
>  {
> +	struct tun_security_struct *tunsec = security;
>  	struct sk_security_struct *sksec = sk->sk_security;
>  	u32 sid = current_sid();
>  	int err;
>  
> +	/* we don't currently perform any NetLabel based labeling here and it
> +	 * isn't clear that we would want to do so anyway; while we could apply
> +	 * labeling without the support of the TUN user the resulting labeled
> +	 * traffic from the other end of the connection would almost certainly
> +	 * cause confusion to the TUN user that had no idea network labeling
> +	 * protocols were being used */
> +
>  	err = avc_has_perm(sid, sksec->sid, SECCLASS_TUN_SOCKET,
>  			   TUN_SOCKET__RELABELFROM, NULL);
>  	if (err)
>  		return err;
> -	err = avc_has_perm(sid, sid, SECCLASS_TUN_SOCKET,
> +	err = avc_has_perm(sid, tunsec->sid, SECCLASS_TUN_SOCKET,
>  			   TUN_SOCKET__RELABELTO, NULL);
>  	if (err)
>  		return err;
>  
> -	sksec->sid = sid;
> +	sksec->sid = tunsec->sid;
> +	sksec->sclass = SECCLASS_TUN_SOCKET;

I'm not sure whether this is correct, looks like we need to differ between TUNSETQUEUE and TUNSETIFF. When userspace call TUNSETIFF for persistent device, looks like we need change the sid of tunsec
like in the past.

Thanks
>  
>  	return 0;
>  }
> @@ -5642,8 +5651,9 @@ static struct security_operations selinux_ops = {
>  	.secmark_refcount_inc =		selinux_secmark_refcount_inc,
>  	.secmark_refcount_dec =		selinux_secmark_refcount_dec,
>  	.req_classify_flow =		selinux_req_classify_flow,
> +	.tun_dev_alloc_security =	selinux_tun_dev_alloc_security,
> +	.tun_dev_free_security =	selinux_tun_dev_free_security,
>  	.tun_dev_create =		selinux_tun_dev_create,
> -	.tun_dev_post_create = 		selinux_tun_dev_post_create,
>  	.tun_dev_attach =		selinux_tun_dev_attach,
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
> index 26c7eee..aa47bca 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -110,6 +110,10 @@ struct sk_security_struct {
>  	u16 sclass;			/* sock security class */
>  };
>  
> +struct tun_security_struct {
> +	u32 sid;			/* SID for the tun device sockets */
> +};
> +
>  struct key_security_struct {
>  	u32 sid;	/* SID of key */
>  };
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Paul Moore]

On Monday, December 03, 2012 06:15:42 PM Jason Wang wrote:
> On 11/30/2012 06:06 AM, Paul Moore wrote:
> > This patch corrects some problems with LSM/SELinux that were introduced
> > with the multiqueue patchset.  The problem stems from the fact that the
> > multiqueue work changed the relationship between the tun device and its
> > associated socket; before the socket persisted for the life of the
> > device, however after the multiqueue changes the socket only persisted
> > for the life of the userspace connection (fd open).  For non-persistent
> > devices this is not an issue, but for persistent devices this can cause
> > the tun device to lose its SELinux label.
> > 
> > We correct this problem by adding an opaque LSM security blob to the
> > tun device struct which allows us to have the LSM security state, e.g.
> > SELinux labeling information, persist for the lifetime of the tun
> > device.

...

> > -static int selinux_tun_dev_attach(struct sock *sk)
> > +static int selinux_tun_dev_attach(struct sock *sk, void *security)
> > 
> >  {
> > 
> > +	struct tun_security_struct *tunsec = security;
> > 
> >  	struct sk_security_struct *sksec = sk->sk_security;
> >  	u32 sid = current_sid();
> >  	int err;
> > 
> > +	/* we don't currently perform any NetLabel based labeling here ...
> >
> >  	err = avc_has_perm(sid, sksec->sid, SECCLASS_TUN_SOCKET,
> >  	
> >  			   TUN_SOCKET__RELABELFROM, NULL);
> >  	
> >  	if (err)
> >  	
> >  		return err;
> > 
> > -	err = avc_has_perm(sid, sid, SECCLASS_TUN_SOCKET,
> > +	err = avc_has_perm(sid, tunsec->sid, SECCLASS_TUN_SOCKET,
> > 
> >  			   TUN_SOCKET__RELABELTO, NULL);
> >  	
> >  	if (err)
> >  	
> >  		return err;
> > 
> > -	sksec->sid = sid;
> > +	sksec->sid = tunsec->sid;
> > +	sksec->sclass = SECCLASS_TUN_SOCKET;
> 
> I'm not sure whether this is correct, looks like we need to differ between
> TUNSETQUEUE and TUNSETIFF. When userspace call TUNSETIFF for persistent
> device, looks like we need change the sid of tunsec like in the past.

It may be that I'm misunderstanding TUNSETQUEUE and/or TUNSETIFF.  Can you 
elaborate as to why they should be different?

One thing that I think we probably should change is the relabelto/from 
permissions in the function above (selinux_tun_dev_attach()); in the case 
where the socket does not yet have a label, e.g. 'sksec->sid == 0', we should 
probably skip the relabel permissions since we want to assign the TUN device 
label regardless in this case.

-- 
paul moore
security and virtualization @ redhat

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Jason Wang]

On Monday, December 03, 2012 11:22:29 AM Paul Moore wrote:
> On Monday, December 03, 2012 06:15:42 PM Jason Wang wrote:
> > On 11/30/2012 06:06 AM, Paul Moore wrote:
> > > This patch corrects some problems with LSM/SELinux that were introduced
> > > with the multiqueue patchset.  The problem stems from the fact that the
> > > multiqueue work changed the relationship between the tun device and its
> > > associated socket; before the socket persisted for the life of the
> > > device, however after the multiqueue changes the socket only persisted
> > > for the life of the userspace connection (fd open).  For non-persistent
> > > devices this is not an issue, but for persistent devices this can cause
> > > the tun device to lose its SELinux label.
> > > 
> > > We correct this problem by adding an opaque LSM security blob to the
> > > tun device struct which allows us to have the LSM security state, e.g.
> > > SELinux labeling information, persist for the lifetime of the tun
> > > device.
> 
> ...
> 
> > > -static int selinux_tun_dev_attach(struct sock *sk)
> > > +static int selinux_tun_dev_attach(struct sock *sk, void *security)
> > > 
> > >  {
> > > 
> > > +	struct tun_security_struct *tunsec = security;
> > > 
> > >  	struct sk_security_struct *sksec = sk->sk_security;
> > >  	u32 sid = current_sid();
> > >  	int err;
> > > 
> > > +	/* we don't currently perform any NetLabel based labeling here ...
> > > 
> > >  	err = avc_has_perm(sid, sksec->sid, SECCLASS_TUN_SOCKET,
> > >  	
> > >  			   TUN_SOCKET__RELABELFROM, NULL);
> > >  	
> > >  	if (err)
> > >  	
> > >  		return err;
> > > 
> > > -	err = avc_has_perm(sid, sid, SECCLASS_TUN_SOCKET,
> > > +	err = avc_has_perm(sid, tunsec->sid, SECCLASS_TUN_SOCKET,
> > > 
> > >  			   TUN_SOCKET__RELABELTO, NULL);
> > >  	
> > >  	if (err)
> > >  	
> > >  		return err;
> > > 
> > > -	sksec->sid = sid;
> > > +	sksec->sid = tunsec->sid;
> > > +	sksec->sclass = SECCLASS_TUN_SOCKET;
> > 
> > I'm not sure whether this is correct, looks like we need to differ between
> > TUNSETQUEUE and TUNSETIFF. When userspace call TUNSETIFF for persistent
> > device, looks like we need change the sid of tunsec like in the past.
> 
> It may be that I'm misunderstanding TUNSETQUEUE and/or TUNSETIFF.  Can you
> elaborate as to why they should be different?

If I understand correctly, before multiqueue patchset, TUNSETIFF is used to:

1) Create the tun/tap network device
2) For persistent device, re-attach the fd to the network device / socket. In 
this case, we call selinux_tun_dev_attch() to relabel the socket sid (in fact 
also the device's since the socket were persistent also) to the sid of process 
that calls TUNSETIFF.

So, after the changes of multiqueue, we need try to preserve those policy. The 
interesting part is the introducing of TUNSETQUEUE, it's used to attach more 
file descriptors/sockets to a tun/tap device after at least one file descriptor 
were attached to the tun/tap device through TUNSETIFF. So I think maybe we 
need differ those two ioctls. This patch looks fine for TUNSETQUEUE, but for 
TUNSETIFF, we need relabel the tunsec to the process that calling TUNSETIFF 
for persistent device?

btw. Current code does allow calling TUNSETQUEUE to a persistent tun/tap 
device with no file attached. It should be a bug and need to be fixed.
> 
> One thing that I think we probably should change is the relabelto/from
> permissions in the function above (selinux_tun_dev_attach()); in the case
> where the socket does not yet have a label, e.g. 'sksec->sid == 0', we
> should probably skip the relabel permissions since we want to assign the
> TUN device label regardless in this case.

I'm not familiar with the selinux, have a quick glance of the code, looks like 
the label has been initialized to SECINITSID_KERNEL in 
selinux_socket_post_create().

Thanks

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Michael S. Tsirkin]

On Tue, Dec 04, 2012 at 09:24:43PM +0800, Jason Wang wrote:
> On Monday, December 03, 2012 11:22:29 AM Paul Moore wrote:
> > On Monday, December 03, 2012 06:15:42 PM Jason Wang wrote:
> > > On 11/30/2012 06:06 AM, Paul Moore wrote:
> > > > This patch corrects some problems with LSM/SELinux that were introduced
> > > > with the multiqueue patchset.  The problem stems from the fact that the
> > > > multiqueue work changed the relationship between the tun device and its
> > > > associated socket; before the socket persisted for the life of the
> > > > device, however after the multiqueue changes the socket only persisted
> > > > for the life of the userspace connection (fd open).  For non-persistent
> > > > devices this is not an issue, but for persistent devices this can cause
> > > > the tun device to lose its SELinux label.
> > > > 
> > > > We correct this problem by adding an opaque LSM security blob to the
> > > > tun device struct which allows us to have the LSM security state, e.g.
> > > > SELinux labeling information, persist for the lifetime of the tun
> > > > device.
> > 
> > ...
> > 
> > > > -static int selinux_tun_dev_attach(struct sock *sk)
> > > > +static int selinux_tun_dev_attach(struct sock *sk, void *security)
> > > > 
> > > >  {
> > > > 
> > > > +	struct tun_security_struct *tunsec = security;
> > > > 
> > > >  	struct sk_security_struct *sksec = sk->sk_security;
> > > >  	u32 sid = current_sid();
> > > >  	int err;
> > > > 
> > > > +	/* we don't currently perform any NetLabel based labeling here ...
> > > > 
> > > >  	err = avc_has_perm(sid, sksec->sid, SECCLASS_TUN_SOCKET,
> > > >  	
> > > >  			   TUN_SOCKET__RELABELFROM, NULL);
> > > >  	
> > > >  	if (err)
> > > >  	
> > > >  		return err;
> > > > 
> > > > -	err = avc_has_perm(sid, sid, SECCLASS_TUN_SOCKET,
> > > > +	err = avc_has_perm(sid, tunsec->sid, SECCLASS_TUN_SOCKET,
> > > > 
> > > >  			   TUN_SOCKET__RELABELTO, NULL);
> > > >  	
> > > >  	if (err)
> > > >  	
> > > >  		return err;
> > > > 
> > > > -	sksec->sid = sid;
> > > > +	sksec->sid = tunsec->sid;
> > > > +	sksec->sclass = SECCLASS_TUN_SOCKET;
> > > 
> > > I'm not sure whether this is correct, looks like we need to differ between
> > > TUNSETQUEUE and TUNSETIFF. When userspace call TUNSETIFF for persistent
> > > device, looks like we need change the sid of tunsec like in the past.
> > 
> > It may be that I'm misunderstanding TUNSETQUEUE and/or TUNSETIFF.  Can you
> > elaborate as to why they should be different?
> 
> If I understand correctly, before multiqueue patchset, TUNSETIFF is used to:
> 
> 1) Create the tun/tap network device
> 2) For persistent device, re-attach the fd to the network device / socket. In 
> this case, we call selinux_tun_dev_attch() to relabel the socket sid (in fact 
> also the device's since the socket were persistent also) to the sid of process 
> that calls TUNSETIFF.
> 
> So, after the changes of multiqueue, we need try to preserve those policy. The 
> interesting part is the introducing of TUNSETQUEUE, it's used to attach more 
> file descriptors/sockets to a tun/tap device after at least one file descriptor 
> were attached to the tun/tap device through TUNSETIFF. So I think maybe we 
> need differ those two ioctls. This patch looks fine for TUNSETQUEUE, but for 
> TUNSETIFF, we need relabel the tunsec to the process that calling TUNSETIFF 
> for persistent device?

Basically, it looks like currently once you get a tun fd,
you can attach it to any device even if normally
selinux would prevent you from accessing it.

If we reuse selinux_tun_dev_attach, we won't need to
change selinux policy, with a new capability we will need to change it
to allow libvirt to do TUNSETQUEUE.


> 
> btw. Current code does allow calling TUNSETQUEUE to a persistent tun/tap 
> device with no file attached. It should be a bug and need to be fixed.

Is this a problem? You can always
attach
set queue
detach

and it would be hard to prevent this ...

> > 
> > One thing that I think we probably should change is the relabelto/from
> > permissions in the function above (selinux_tun_dev_attach()); in the case
> > where the socket does not yet have a label, e.g. 'sksec->sid == 0', we
> > should probably skip the relabel permissions since we want to assign the
> > TUN device label regardless in this case.
> 
> I'm not familiar with the selinux, have a quick glance of the code, looks like 
> the label has been initialized to SECINITSID_KERNEL in 
> selinux_socket_post_create().
> 
> Thanks

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Jason Wang]

On 12/04/2012 11:24 PM, Michael S. Tsirkin wrote:
> On Tue, Dec 04, 2012 at 09:24:43PM +0800, Jason Wang wrote:
>> On Monday, December 03, 2012 11:22:29 AM Paul Moore wrote:
>>> On Monday, December 03, 2012 06:15:42 PM Jason Wang wrote:
>>>> On 11/30/2012 06:06 AM, Paul Moore wrote:
>>>>> This patch corrects some problems with LSM/SELinux that were introduced
>>>>> with the multiqueue patchset.  The problem stems from the fact that the
>>>>> multiqueue work changed the relationship between the tun device and its
>>>>> associated socket; before the socket persisted for the life of the
>>>>> device, however after the multiqueue changes the socket only persisted
>>>>> for the life of the userspace connection (fd open).  For non-persistent
>>>>> devices this is not an issue, but for persistent devices this can cause
>>>>> the tun device to lose its SELinux label.
>>>>>
>>>>> We correct this problem by adding an opaque LSM security blob to the
>>>>> tun device struct which allows us to have the LSM security state, e.g.
>>>>> SELinux labeling information, persist for the lifetime of the tun
>>>>> device.
>>> ...
>>>
>>>>> -static int selinux_tun_dev_attach(struct sock *sk)
>>>>> +static int selinux_tun_dev_attach(struct sock *sk, void *security)
>>>>>
>>>>>  {
>>>>>
>>>>> +	struct tun_security_struct *tunsec = security;
>>>>>
>>>>>  	struct sk_security_struct *sksec = sk->sk_security;
>>>>>  	u32 sid = current_sid();
>>>>>  	int err;
>>>>>
>>>>> +	/* we don't currently perform any NetLabel based labeling here ...
>>>>>
>>>>>  	err = avc_has_perm(sid, sksec->sid, SECCLASS_TUN_SOCKET,
>>>>>  	
>>>>>  			   TUN_SOCKET__RELABELFROM, NULL);
>>>>>  	
>>>>>  	if (err)
>>>>>  	
>>>>>  		return err;
>>>>>
>>>>> -	err = avc_has_perm(sid, sid, SECCLASS_TUN_SOCKET,
>>>>> +	err = avc_has_perm(sid, tunsec->sid, SECCLASS_TUN_SOCKET,
>>>>>
>>>>>  			   TUN_SOCKET__RELABELTO, NULL);
>>>>>  	
>>>>>  	if (err)
>>>>>  	
>>>>>  		return err;
>>>>>
>>>>> -	sksec->sid = sid;
>>>>> +	sksec->sid = tunsec->sid;
>>>>> +	sksec->sclass = SECCLASS_TUN_SOCKET;
>>>> I'm not sure whether this is correct, looks like we need to differ between
>>>> TUNSETQUEUE and TUNSETIFF. When userspace call TUNSETIFF for persistent
>>>> device, looks like we need change the sid of tunsec like in the past.
>>> It may be that I'm misunderstanding TUNSETQUEUE and/or TUNSETIFF.  Can you
>>> elaborate as to why they should be different?
>> If I understand correctly, before multiqueue patchset, TUNSETIFF is used to:
>>
>> 1) Create the tun/tap network device
>> 2) For persistent device, re-attach the fd to the network device / socket. In 
>> this case, we call selinux_tun_dev_attch() to relabel the socket sid (in fact 
>> also the device's since the socket were persistent also) to the sid of process 
>> that calls TUNSETIFF.
>>
>> So, after the changes of multiqueue, we need try to preserve those policy. The 
>> interesting part is the introducing of TUNSETQUEUE, it's used to attach more 
>> file descriptors/sockets to a tun/tap device after at least one file descriptor 
>> were attached to the tun/tap device through TUNSETIFF. So I think maybe we 
>> need differ those two ioctls. This patch looks fine for TUNSETQUEUE, but for 
>> TUNSETIFF, we need relabel the tunsec to the process that calling TUNSETIFF 
>> for persistent device?
> Basically, it looks like currently once you get a tun fd,
> you can attach it to any device even if normally
> selinux would prevent you from accessing it.

Yes some checking during TUNSETQUEUE is missed.
> If we reuse selinux_tun_dev_attach, we won't need to
> change selinux policy, with a new capability we will need to change it
> to allow libvirt to do TUNSETQUEUE.
>

Also needed for qemu too since it may call TUNSETQUEUE when guest wants
to change the number of queues.
>> btw. Current code does allow calling TUNSETQUEUE to a persistent tun/tap 
>> device with no file attached. It should be a bug and need to be fixed.
> Is this a problem? You can always
> attach
> set queue
> detach
>
> and it would be hard to prevent this ...

Currently, the following steps is allowed:

1. fd1 = open("/dev/net/tun");
2. tunsetiff(fd1, "tap0");
3. tunsetpersistent("tap0");
4. close(fd1);
5. fd2 = open("/dev/net/tun");
6. tunsetqueue(fd2, "tap0);

Looks like step 6 should be forbidden since:

- no fd/sockets were attached to the device, we need use TUNSETIFF
instead to keep the API as we used do in single queue tun
- we need update the security information in tun_struct just like what
we discussed in this mail
- it may also miss checks in TUNSETIFF

>>> One thing that I think we probably should change is the relabelto/from
>>> permissions in the function above (selinux_tun_dev_attach()); in the case
>>> where the socket does not yet have a label, e.g. 'sksec->sid == 0', we
>>> should probably skip the relabel permissions since we want to assign the
>>> TUN device label regardless in this case.
>> I'm not familiar with the selinux, have a quick glance of the code, looks like 
>> the label has been initialized to SECINITSID_KERNEL in 
>> selinux_socket_post_create().
>>
>> Thanks

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Michael S. Tsirkin]

On Wed, Dec 05, 2012 at 02:17:30PM +0800, Jason Wang wrote:
> On 12/04/2012 11:24 PM, Michael S. Tsirkin wrote:
> > On Tue, Dec 04, 2012 at 09:24:43PM +0800, Jason Wang wrote:
> >> On Monday, December 03, 2012 11:22:29 AM Paul Moore wrote:
> >>> On Monday, December 03, 2012 06:15:42 PM Jason Wang wrote:
> >>>> On 11/30/2012 06:06 AM, Paul Moore wrote:
> >>>>> This patch corrects some problems with LSM/SELinux that were introduced
> >>>>> with the multiqueue patchset.  The problem stems from the fact that the
> >>>>> multiqueue work changed the relationship between the tun device and its
> >>>>> associated socket; before the socket persisted for the life of the
> >>>>> device, however after the multiqueue changes the socket only persisted
> >>>>> for the life of the userspace connection (fd open).  For non-persistent
> >>>>> devices this is not an issue, but for persistent devices this can cause
> >>>>> the tun device to lose its SELinux label.
> >>>>>
> >>>>> We correct this problem by adding an opaque LSM security blob to the
> >>>>> tun device struct which allows us to have the LSM security state, e.g.
> >>>>> SELinux labeling information, persist for the lifetime of the tun
> >>>>> device.
> >>> ...
> >>>
> >>>>> -static int selinux_tun_dev_attach(struct sock *sk)
> >>>>> +static int selinux_tun_dev_attach(struct sock *sk, void *security)
> >>>>>
> >>>>>  {
> >>>>>
> >>>>> +	struct tun_security_struct *tunsec = security;
> >>>>>
> >>>>>  	struct sk_security_struct *sksec = sk->sk_security;
> >>>>>  	u32 sid = current_sid();
> >>>>>  	int err;
> >>>>>
> >>>>> +	/* we don't currently perform any NetLabel based labeling here ...
> >>>>>
> >>>>>  	err = avc_has_perm(sid, sksec->sid, SECCLASS_TUN_SOCKET,
> >>>>>  	
> >>>>>  			   TUN_SOCKET__RELABELFROM, NULL);
> >>>>>  	
> >>>>>  	if (err)
> >>>>>  	
> >>>>>  		return err;
> >>>>>
> >>>>> -	err = avc_has_perm(sid, sid, SECCLASS_TUN_SOCKET,
> >>>>> +	err = avc_has_perm(sid, tunsec->sid, SECCLASS_TUN_SOCKET,
> >>>>>
> >>>>>  			   TUN_SOCKET__RELABELTO, NULL);
> >>>>>  	
> >>>>>  	if (err)
> >>>>>  	
> >>>>>  		return err;
> >>>>>
> >>>>> -	sksec->sid = sid;
> >>>>> +	sksec->sid = tunsec->sid;
> >>>>> +	sksec->sclass = SECCLASS_TUN_SOCKET;
> >>>> I'm not sure whether this is correct, looks like we need to differ between
> >>>> TUNSETQUEUE and TUNSETIFF. When userspace call TUNSETIFF for persistent
> >>>> device, looks like we need change the sid of tunsec like in the past.
> >>> It may be that I'm misunderstanding TUNSETQUEUE and/or TUNSETIFF.  Can you
> >>> elaborate as to why they should be different?
> >> If I understand correctly, before multiqueue patchset, TUNSETIFF is used to:
> >>
> >> 1) Create the tun/tap network device
> >> 2) For persistent device, re-attach the fd to the network device / socket. In 
> >> this case, we call selinux_tun_dev_attch() to relabel the socket sid (in fact 
> >> also the device's since the socket were persistent also) to the sid of process 
> >> that calls TUNSETIFF.
> >>
> >> So, after the changes of multiqueue, we need try to preserve those policy. The 
> >> interesting part is the introducing of TUNSETQUEUE, it's used to attach more 
> >> file descriptors/sockets to a tun/tap device after at least one file descriptor 
> >> were attached to the tun/tap device through TUNSETIFF. So I think maybe we 
> >> need differ those two ioctls. This patch looks fine for TUNSETQUEUE, but for 
> >> TUNSETIFF, we need relabel the tunsec to the process that calling TUNSETIFF 
> >> for persistent device?
> > Basically, it looks like currently once you get a tun fd,
> > you can attach it to any device even if normally
> > selinux would prevent you from accessing it.
> 
> Yes some checking during TUNSETQUEUE is missed.
> > If we reuse selinux_tun_dev_attach, we won't need to
> > change selinux policy, with a new capability we will need to change it
> > to allow libvirt to do TUNSETQUEUE.
> >
> 
> Also needed for qemu too since it may call TUNSETQUEUE when guest wants
> to change the number of queues.

Hmm that's nasty. If you allow qemu to do TUNSETQUEUE then how
do you prevent it from attaching to some other tun?
Maybe we can extend TUNSETQUEUE or add another ioctl to
mark a queue active/inactive? Probably control transmit
and receive being active separately as well.

> >> btw. Current code does allow calling TUNSETQUEUE to a persistent tun/tap 
> >> device with no file attached. It should be a bug and need to be fixed.
> > Is this a problem? You can always
> > attach
> > set queue
> > detach
> >
> > and it would be hard to prevent this ...
> 
> Currently, the following steps is allowed:
> 
> 1. fd1 = open("/dev/net/tun");
> 2. tunsetiff(fd1, "tap0");
> 3. tunsetpersistent("tap0");
> 4. close(fd1);
> 5. fd2 = open("/dev/net/tun");
> 6. tunsetqueue(fd2, "tap0);

Allowed for libvirt, right? Not for qemu.

> Looks like step 6 should be forbidden since:
> 
> - no fd/sockets were attached to the device, we need use TUNSETIFF
> instead to keep the API as we used do in single queue tun
> - we need update the security information in tun_struct just like what
> we discussed in this mail
> - it may also miss checks in TUNSETIFF

We need to fix the checks anyway. Basically if you allow
a queue to be the only fd that is attached
(and I don't see how you can prevent this since
userspace does not expect close to fail),
I don't see why one way to get to this state
should be legal and another illegal.

The rest is implementation detail.

> >>> One thing that I think we probably should change is the relabelto/from
> >>> permissions in the function above (selinux_tun_dev_attach()); in the case
> >>> where the socket does not yet have a label, e.g. 'sksec->sid == 0', we
> >>> should probably skip the relabel permissions since we want to assign the
> >>> TUN device label regardless in this case.
> >> I'm not familiar with the selinux, have a quick glance of the code, looks like 
> >> the label has been initialized to SECINITSID_KERNEL in 
> >> selinux_socket_post_create().
> >>
> >> Thanks

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Jason Wang]

On Wednesday, December 05, 2012 01:43:39 PM Michael S. Tsirkin wrote:
> On Wed, Dec 05, 2012 at 02:17:30PM +0800, Jason Wang wrote:
> > On 12/04/2012 11:24 PM, Michael S. Tsirkin wrote:
> > > On Tue, Dec 04, 2012 at 09:24:43PM +0800, Jason Wang wrote:
> > >> On Monday, December 03, 2012 11:22:29 AM Paul Moore wrote:
> > >>> On Monday, December 03, 2012 06:15:42 PM Jason Wang wrote:
> > >>>> On 11/30/2012 06:06 AM, Paul Moore wrote:
> > >>>>> This patch corrects some problems with LSM/SELinux that were
> > >>>>> introduced
> > >>>>> with the multiqueue patchset.  The problem stems from the fact that
> > >>>>> the
> > >>>>> multiqueue work changed the relationship between the tun device and
> > >>>>> its
> > >>>>> associated socket; before the socket persisted for the life of the
> > >>>>> device, however after the multiqueue changes the socket only
> > >>>>> persisted
> > >>>>> for the life of the userspace connection (fd open).  For
> > >>>>> non-persistent
> > >>>>> devices this is not an issue, but for persistent devices this can
> > >>>>> cause
> > >>>>> the tun device to lose its SELinux label.
> > >>>>> 
> > >>>>> We correct this problem by adding an opaque LSM security blob to the
> > >>>>> tun device struct which allows us to have the LSM security state,
> > >>>>> e.g.
> > >>>>> SELinux labeling information, persist for the lifetime of the tun
> > >>>>> device.
> > >>> 
> > >>> ...
> > >>> 
> > >>>>> -static int selinux_tun_dev_attach(struct sock *sk)
> > >>>>> +static int selinux_tun_dev_attach(struct sock *sk, void *security)
> > >>>>> 
> > >>>>>  {
> > >>>>> 
> > >>>>> +	struct tun_security_struct *tunsec = security;
> > >>>>> 
> > >>>>>  	struct sk_security_struct *sksec = sk->sk_security;
> > >>>>>  	u32 sid = current_sid();
> > >>>>>  	int err;
> > >>>>> 
> > >>>>> +	/* we don't currently perform any NetLabel based labeling here ...
> > >>>>> 
> > >>>>>  	err = avc_has_perm(sid, sksec->sid, SECCLASS_TUN_SOCKET,
> > >>>>>  	
> > >>>>>  			   TUN_SOCKET__RELABELFROM, NULL);
> > >>>>>  	
> > >>>>>  	if (err)
> > >>>>>  	
> > >>>>>  		return err;
> > >>>>> 
> > >>>>> -	err = avc_has_perm(sid, sid, SECCLASS_TUN_SOCKET,
> > >>>>> +	err = avc_has_perm(sid, tunsec->sid, SECCLASS_TUN_SOCKET,
> > >>>>> 
> > >>>>>  			   TUN_SOCKET__RELABELTO, NULL);
> > >>>>>  	
> > >>>>>  	if (err)
> > >>>>>  	
> > >>>>>  		return err;
> > >>>>> 
> > >>>>> -	sksec->sid = sid;
> > >>>>> +	sksec->sid = tunsec->sid;
> > >>>>> +	sksec->sclass = SECCLASS_TUN_SOCKET;
> > >>>> 
> > >>>> I'm not sure whether this is correct, looks like we need to differ
> > >>>> between
> > >>>> TUNSETQUEUE and TUNSETIFF. When userspace call TUNSETIFF for
> > >>>> persistent
> > >>>> device, looks like we need change the sid of tunsec like in the past.
> > >>> 
> > >>> It may be that I'm misunderstanding TUNSETQUEUE and/or TUNSETIFF.  Can
> > >>> you
> > >>> elaborate as to why they should be different?
> > >> 
> > >> If I understand correctly, before multiqueue patchset, TUNSETIFF is
> > >> used to:
> > >> 
> > >> 1) Create the tun/tap network device
> > >> 2) For persistent device, re-attach the fd to the network device /
> > >> socket. In this case, we call selinux_tun_dev_attch() to relabel the
> > >> socket sid (in fact also the device's since the socket were persistent
> > >> also) to the sid of process that calls TUNSETIFF.
> > >> 
> > >> So, after the changes of multiqueue, we need try to preserve those
> > >> policy. The interesting part is the introducing of TUNSETQUEUE, it's
> > >> used to attach more file descriptors/sockets to a tun/tap device after
> > >> at least one file descriptor were attached to the tun/tap device
> > >> through TUNSETIFF. So I think maybe we need differ those two ioctls.
> > >> This patch looks fine for TUNSETQUEUE, but for TUNSETIFF, we need
> > >> relabel the tunsec to the process that calling TUNSETIFF for
> > >> persistent device?
> > > 
> > > Basically, it looks like currently once you get a tun fd,
> > > you can attach it to any device even if normally
> > > selinux would prevent you from accessing it.
> > 
> > Yes some checking during TUNSETQUEUE is missed.
> > 
> > > If we reuse selinux_tun_dev_attach, we won't need to
> > > change selinux policy, with a new capability we will need to change it
> > > to allow libvirt to do TUNSETQUEUE.
> > 
> > Also needed for qemu too since it may call TUNSETQUEUE when guest wants
> > to change the number of queues.
> 
> Hmm that's nasty. If you allow qemu to do TUNSETQUEUE then how
> do you prevent it from attaching to some other tun?

Currently, we do the uid & gid check in tun_not_capable() also for 
TUNSETQUEUE. So the fd can't be attach to the device if current process is not 
the owner. And if it's the owner, is it harm to allow the fd to be attached to 
another device?

There's no state information of the tun stored in the tun_file, and it was 
something like after you open a fd of tun, you can call TUNSETIFF to any tun 
if the owner is current process.
> Maybe we can extend TUNSETQUEUE or add another ioctl to
> mark a queue active/inactive? Probably control transmit
> and receive being active separately as well.

It's better to extend current TUNSETQUEUE if needed, for controling 
transmitting and receiving, we can add more parameters to this ioctl (e.g. 
IFF_TX_ENABLE, IFF_TX_DISABLE)
> 
> > >> btw. Current code does allow calling TUNSETQUEUE to a persistent
> > >> tun/tap
> > >> device with no file attached. It should be a bug and need to be fixed.
> > > 
> > > Is this a problem? You can always
> > > attach
> > > set queue
> > > detach
> > > 
> > > and it would be hard to prevent this ...
> > 
> > Currently, the following steps is allowed:
> > 
> > 1. fd1 = open("/dev/net/tun");
> > 2. tunsetiff(fd1, "tap0");
> > 3. tunsetpersistent("tap0");
> > 4. close(fd1);
> > 5. fd2 = open("/dev/net/tun");
> > 6. tunsetqueue(fd2, "tap0);
> 
> Allowed for libvirt, right? Not for qemu.

But what if step 1-4 is done by one process, step 5-6 is done by another? The 
issuse only existed for persistent device with no fd attached. 

I agree we can add checks and let TUNSETQUEUE and let it to differ those 
conditions. But for simplicity, I suggest is to let userspace call TUNSETIFF 
first just like what we did when only single queue is supported in tun, and 
relabel the security of tun_struct there. And we can use different check (e.g. 
selinux for TUNSETQUEUE and TUNSETIFF) to enable the qemu to does TUNSETQUEUE 
itself.

> 
> > Looks like step 6 should be forbidden since:
> > 
> > - no fd/sockets were attached to the device, we need use TUNSETIFF
> > instead to keep the API as we used do in single queue tun
> > - we need update the security information in tun_struct just like what
> > we discussed in this mail
> > - it may also miss checks in TUNSETIFF
> 
> We need to fix the checks anyway. Basically if you allow
> a queue to be the only fd that is attached
> (and I don't see how you can prevent this since
> userspace does not expect close to fail),
> I don't see why one way to get to this state
> should be legal and another illegal.

Well, we can return -EPERM when userspace call TUNSETQUEUE to a tun_struct 
with zero fd/queues.
> 
> The rest is implementation detail.
> 
> > >>> One thing that I think we probably should change is the relabelto/from
> > >>> permissions in the function above (selinux_tun_dev_attach()); in the
> > >>> case
> > >>> where the socket does not yet have a label, e.g. 'sksec->sid == 0', we
> > >>> should probably skip the relabel permissions since we want to assign
> > >>> the
> > >>> TUN device label regardless in this case.
> > >> 
> > >> I'm not familiar with the selinux, have a quick glance of the code,
> > >> looks like the label has been initialized to SECINITSID_KERNEL in
> > >> selinux_socket_post_create().
> > >> 
> > >> Thanks
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Paul Moore]

On Tuesday, December 04, 2012 09:24:43 PM Jason Wang wrote:
> On Monday, December 03, 2012 11:22:29 AM Paul Moore wrote:
> > It may be that I'm misunderstanding TUNSETQUEUE and/or TUNSETIFF.  Can you
> > elaborate as to why they should be different?
> 
> If I understand correctly, before multiqueue patchset, TUNSETIFF is used to:
> 
> 1) Create the tun/tap network device
> 2) For persistent device, re-attach the fd to the network device / socket.
> In this case, we call selinux_tun_dev_attch() to relabel the socket sid (in
> fact also the device's since the socket were persistent also) to the sid of
> process that calls TUNSETIFF.
> 
> So, after the changes of multiqueue, we need try to preserve those policy.
> The interesting part is the introducing of TUNSETQUEUE, it's used to attach
> more file descriptors/sockets to a tun/tap device after at least one file
> descriptor were attached to the tun/tap device through TUNSETIFF. So I
> think maybe we need differ those two ioctls. This patch looks fine for
> TUNSETQUEUE, but for TUNSETIFF, we need relabel the tunsec to the process
> that calling TUNSETIFF for persistent device?

Okay, based on your explanation of TUNSETQUEUE, the steps below are what I 
believe we need to do ... if you disagree speak up quickly please.

A. TUNSETIFF (new, non-persistent device)

[Allocate and initialize the tun_struct LSM state based on the calling 
process, use this state to label the TUN socket.]

1. Call security_tun_dev_create() which authorizes the action.
2. Call security_tun_dev_alloc_security() which allocates the tun_struct LSM 
blob and SELinux sets some internal blob state to record the label of the 
calling process.
3. Call security_tun_dev_attach() which sets the label of the TUN socket to 
match the label stored in the tun_struct LSM blob during A2.  No authorization 
is done at this point since the socket is new/unlabeled.

B. TUNSETIFF (existing, persistent device)

[Relabel the existing tun_struct LSM state based on the calling process, use
this state to label the TUN socket.]

1. Attempt to relabel/reset the tun_struct LSM blob from the currently stored 
value, set during A2, to the label of the current calling process. *** THIS IS 
NOT CURRENTLY DONE IN THE RFC PATCH ***
2. Call security_tun_dev_attach() which sets the label of the TUN socket to
match the label stored in the tun_struct LSM blob during B1. No authorization 
is done at this point since the socket is new/unlabeled.

C. TUNSETQUEUE

[Use the existing tun_struct LSM state to label the new TUN socket.]

1. Call security_tun_dev_attach() which sets the label of the TUN socket to 
match the label stored in the tun_struct LSM blob set during either A2 or B1.  
No authorization is done at this point since the socket is new/unlabeled.

> btw. Current code does allow calling TUNSETQUEUE to a persistent tun/tap
> device with no file attached. It should be a bug and need to be fixed.

Since you wrote that code will you be submitting a patch to fix that problem?

> > One thing that I think we probably should change is the relabelto/from
> > permissions in the function above (selinux_tun_dev_attach()); in the case
> > where the socket does not yet have a label, e.g. 'sksec->sid == 0', we
> > should probably skip the relabel permissions since we want to assign the
> > TUN device label regardless in this case.
> 
> I'm not familiar with the selinux, have a quick glance of the code, looks
> like the label has been initialized to SECINITSID_KERNEL in
> selinux_socket_post_create().

Unless I've missed something in your changes, the multiqueue code never calls 
any socket code which ends up calling {security,selinux}_socket_post_create(); 
I believe you only call sk_alloc() which ends up calling 
{security,selinux}_sk_alloc() which sets SECINITSID_UNLABELED (I mistakenly 
wrote 0 instead in my earlier email which is techincally SECSID_NULL).  Either 
way, I still think the logic I originally described above is correct.

-- 
paul moore
security and virtualization @ redhat

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Michael S. Tsirkin]

On Tue, Dec 04, 2012 at 11:18:57AM -0500, Paul Moore wrote:
> On Tuesday, December 04, 2012 09:24:43 PM Jason Wang wrote:
> > On Monday, December 03, 2012 11:22:29 AM Paul Moore wrote:
> > > It may be that I'm misunderstanding TUNSETQUEUE and/or TUNSETIFF.  Can you
> > > elaborate as to why they should be different?
> > 
> > If I understand correctly, before multiqueue patchset, TUNSETIFF is used to:
> > 
> > 1) Create the tun/tap network device
> > 2) For persistent device, re-attach the fd to the network device / socket.
> > In this case, we call selinux_tun_dev_attch() to relabel the socket sid (in
> > fact also the device's since the socket were persistent also) to the sid of
> > process that calls TUNSETIFF.
> > 
> > So, after the changes of multiqueue, we need try to preserve those policy.
> > The interesting part is the introducing of TUNSETQUEUE, it's used to attach
> > more file descriptors/sockets to a tun/tap device after at least one file
> > descriptor were attached to the tun/tap device through TUNSETIFF. So I
> > think maybe we need differ those two ioctls. This patch looks fine for
> > TUNSETQUEUE, but for TUNSETIFF, we need relabel the tunsec to the process
> > that calling TUNSETIFF for persistent device?
> 
> Okay, based on your explanation of TUNSETQUEUE, the steps below are what I 
> believe we need to do ... if you disagree speak up quickly please.
> 
> A. TUNSETIFF (new, non-persistent device)
> 
> [Allocate and initialize the tun_struct LSM state based on the calling 
> process, use this state to label the TUN socket.]
> 
> 1. Call security_tun_dev_create() which authorizes the action.
> 2. Call security_tun_dev_alloc_security() which allocates the tun_struct LSM 
> blob and SELinux sets some internal blob state to record the label of the 
> calling process.
> 3. Call security_tun_dev_attach() which sets the label of the TUN socket to 
> match the label stored in the tun_struct LSM blob during A2.  No authorization 
> is done at this point since the socket is new/unlabeled.

> B. TUNSETIFF (existing, persistent device)
> 
> [Relabel the existing tun_struct LSM state based on the calling process, use
> this state to label the TUN socket.]
> 
> 1. Attempt to relabel/reset the tun_struct LSM blob from the currently stored 
> value, set during A2, to the label of the current calling process. *** THIS IS 
> NOT CURRENTLY DONE IN THE RFC PATCH ***
> 2. Call security_tun_dev_attach() which sets the label of the TUN socket to
> match the label stored in the tun_struct LSM blob during B1. No authorization 
> is done at this point since the socket is new/unlabeled.
> 
> C. TUNSETQUEUE
> 
> [Use the existing tun_struct LSM state to label the new TUN socket.]
> 
> 1. Call security_tun_dev_attach() which sets the label of the TUN socket to 
> match the label stored in the tun_struct LSM blob set during either A2 or B1.  
> No authorization is done at this point since the socket is new/unlabeled.


Here's what bothers me. libvirt currently opens tun and passes
fd to qemu.
What would prevent qemu from attaching fd using TUNSETQUEUE
to another device it does not own?


> > btw. Current code does allow calling TUNSETQUEUE to a persistent tun/tap
> > device with no file attached. It should be a bug and need to be fixed.
> 
> Since you wrote that code will you be submitting a patch to fix that problem?
> 
> > > One thing that I think we probably should change is the relabelto/from
> > > permissions in the function above (selinux_tun_dev_attach()); in the case
> > > where the socket does not yet have a label, e.g. 'sksec->sid == 0', we
> > > should probably skip the relabel permissions since we want to assign the
> > > TUN device label regardless in this case.
> > 
> > I'm not familiar with the selinux, have a quick glance of the code, looks
> > like the label has been initialized to SECINITSID_KERNEL in
> > selinux_socket_post_create().
> 
> Unless I've missed something in your changes, the multiqueue code never calls 
> any socket code which ends up calling {security,selinux}_socket_post_create(); 
> I believe you only call sk_alloc() which ends up calling 
> {security,selinux}_sk_alloc() which sets SECINITSID_UNLABELED (I mistakenly 
> wrote 0 instead in my earlier email which is techincally SECSID_NULL).  Either 
> way, I still think the logic I originally described above is correct.
> 
> -- 
> paul moore
> security and virtualization @ redhat

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Paul Moore]

On Tuesday, December 04, 2012 07:36:26 PM Michael S. Tsirkin wrote:
> On Tue, Dec 04, 2012 at 11:18:57AM -0500, Paul Moore wrote:
> > Okay, based on your explanation of TUNSETQUEUE, the steps below are what I
> > believe we need to do ... if you disagree speak up quickly please.
> > 
> > A. TUNSETIFF (new, non-persistent device)
> > 
> > [Allocate and initialize the tun_struct LSM state based on the calling
> > process, use this state to label the TUN socket.]
> > 
> > 1. Call security_tun_dev_create() which authorizes the action.
> > 2. Call security_tun_dev_alloc_security() which allocates the tun_struct
> > LSM blob and SELinux sets some internal blob state to record the label of
> > the calling process.
> > 3. Call security_tun_dev_attach() which sets the label of the TUN socket
> > to match the label stored in the tun_struct LSM blob during A2.  No
> > authorization is done at this point since the socket is new/unlabeled.
> > 
> > B. TUNSETIFF (existing, persistent device)
> > 
> > [Relabel the existing tun_struct LSM state based on the calling process,
> > use this state to label the TUN socket.]
> > 
> > 1. Attempt to relabel/reset the tun_struct LSM blob from the currently
> > stored value, set during A2, to the label of the current calling process.
> > *** THIS IS NOT CURRENTLY DONE IN THE RFC PATCH ***
> > 2. Call security_tun_dev_attach() which sets the label of the TUN socket
> > to match the label stored in the tun_struct LSM blob during B1. No
> > authorization is done at this point since the socket is new/unlabeled.
> > 
> > C. TUNSETQUEUE
> > 
> > [Use the existing tun_struct LSM state to label the new TUN socket.]
> > 
> > 1. Call security_tun_dev_attach() which sets the label of the TUN socket
> > to match the label stored in the tun_struct LSM blob set during either A2
> > or B1. No authorization is done at this point since the socket is
> > new/unlabeled.
>
> Here's what bothers me. libvirt currently opens tun and passes
> fd to qemu. What would prevent qemu from attaching fd using TUNSETQUEUE
> to another device it does not own?

True, assuming all the above is correct and that I'm understanding it 
correctly (Jason?), we should probably add a new SELinux access control for 
TUNSETQUEUE.

The current DAC code exists in tun_not_capable().

-- 
paul moore
security and virtualization @ redhat

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Jason Wang]

On 12/05/2012 02:17 AM, Paul Moore wrote:
> On Tuesday, December 04, 2012 07:36:26 PM Michael S. Tsirkin wrote:
>> On Tue, Dec 04, 2012 at 11:18:57AM -0500, Paul Moore wrote:
>>> Okay, based on your explanation of TUNSETQUEUE, the steps below are what I
>>> believe we need to do ... if you disagree speak up quickly please.
>>>
>>> A. TUNSETIFF (new, non-persistent device)
>>>
>>> [Allocate and initialize the tun_struct LSM state based on the calling
>>> process, use this state to label the TUN socket.]
>>>
>>> 1. Call security_tun_dev_create() which authorizes the action.
>>> 2. Call security_tun_dev_alloc_security() which allocates the tun_struct
>>> LSM blob and SELinux sets some internal blob state to record the label of
>>> the calling process.
>>> 3. Call security_tun_dev_attach() which sets the label of the TUN socket
>>> to match the label stored in the tun_struct LSM blob during A2.  No
>>> authorization is done at this point since the socket is new/unlabeled.
>>>
>>> B. TUNSETIFF (existing, persistent device)
>>>
>>> [Relabel the existing tun_struct LSM state based on the calling process,
>>> use this state to label the TUN socket.]
>>>
>>> 1. Attempt to relabel/reset the tun_struct LSM blob from the currently
>>> stored value, set during A2, to the label of the current calling process.
>>> *** THIS IS NOT CURRENTLY DONE IN THE RFC PATCH ***
>>> 2. Call security_tun_dev_attach() which sets the label of the TUN socket
>>> to match the label stored in the tun_struct LSM blob during B1. No
>>> authorization is done at this point since the socket is new/unlabeled.
>>>
>>> C. TUNSETQUEUE
>>>
>>> [Use the existing tun_struct LSM state to label the new TUN socket.]
>>>
>>> 1. Call security_tun_dev_attach() which sets the label of the TUN socket
>>> to match the label stored in the tun_struct LSM blob set during either A2
>>> or B1. No authorization is done at this point since the socket is
>>> new/unlabeled.
>> Here's what bothers me. libvirt currently opens tun and passes
>> fd to qemu. What would prevent qemu from attaching fd using TUNSETQUEUE
>> to another device it does not own?
> True, assuming all the above is correct and that I'm understanding it 
> correctly (Jason?), we should probably add a new SELinux access control for 
> TUNSETQUEUE.

Yes, we need make sure qemu can call TUNSETQUEUE for the device it does
not own.
>
> The current DAC code exists in tun_not_capable().
>

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Michael S. Tsirkin]

On Wed, Dec 05, 2012 at 02:19:22PM +0800, Jason Wang wrote:
> On 12/05/2012 02:17 AM, Paul Moore wrote:
> > On Tuesday, December 04, 2012 07:36:26 PM Michael S. Tsirkin wrote:
> >> On Tue, Dec 04, 2012 at 11:18:57AM -0500, Paul Moore wrote:
> >>> Okay, based on your explanation of TUNSETQUEUE, the steps below are what I
> >>> believe we need to do ... if you disagree speak up quickly please.
> >>>
> >>> A. TUNSETIFF (new, non-persistent device)
> >>>
> >>> [Allocate and initialize the tun_struct LSM state based on the calling
> >>> process, use this state to label the TUN socket.]
> >>>
> >>> 1. Call security_tun_dev_create() which authorizes the action.
> >>> 2. Call security_tun_dev_alloc_security() which allocates the tun_struct
> >>> LSM blob and SELinux sets some internal blob state to record the label of
> >>> the calling process.
> >>> 3. Call security_tun_dev_attach() which sets the label of the TUN socket
> >>> to match the label stored in the tun_struct LSM blob during A2.  No
> >>> authorization is done at this point since the socket is new/unlabeled.
> >>>
> >>> B. TUNSETIFF (existing, persistent device)
> >>>
> >>> [Relabel the existing tun_struct LSM state based on the calling process,
> >>> use this state to label the TUN socket.]
> >>>
> >>> 1. Attempt to relabel/reset the tun_struct LSM blob from the currently
> >>> stored value, set during A2, to the label of the current calling process.
> >>> *** THIS IS NOT CURRENTLY DONE IN THE RFC PATCH ***
> >>> 2. Call security_tun_dev_attach() which sets the label of the TUN socket
> >>> to match the label stored in the tun_struct LSM blob during B1. No
> >>> authorization is done at this point since the socket is new/unlabeled.
> >>>
> >>> C. TUNSETQUEUE
> >>>
> >>> [Use the existing tun_struct LSM state to label the new TUN socket.]
> >>>
> >>> 1. Call security_tun_dev_attach() which sets the label of the TUN socket
> >>> to match the label stored in the tun_struct LSM blob set during either A2
> >>> or B1. No authorization is done at this point since the socket is
> >>> new/unlabeled.
> >> Here's what bothers me. libvirt currently opens tun and passes
> >> fd to qemu. What would prevent qemu from attaching fd using TUNSETQUEUE
> >> to another device it does not own?
> > True, assuming all the above is correct and that I'm understanding it 
> > correctly (Jason?), we should probably add a new SELinux access control for 
> > TUNSETQUEUE.
> 
> Yes, we need make sure qemu can call TUNSETQUEUE for the device it does
> not own.

Meaning can *not* call?

> >
> > The current DAC code exists in tun_not_capable().
> >

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Jason Wang]

On Wednesday, December 05, 2012 01:44:55 PM Michael S. Tsirkin wrote:
> On Wed, Dec 05, 2012 at 02:19:22PM +0800, Jason Wang wrote:
> > On 12/05/2012 02:17 AM, Paul Moore wrote:
> > > On Tuesday, December 04, 2012 07:36:26 PM Michael S. Tsirkin wrote:
> > >> On Tue, Dec 04, 2012 at 11:18:57AM -0500, Paul Moore wrote:
> > >>> Okay, based on your explanation of TUNSETQUEUE, the steps below are
> > >>> what I
> > >>> believe we need to do ... if you disagree speak up quickly please.
> > >>> 
> > >>> A. TUNSETIFF (new, non-persistent device)
> > >>> 
> > >>> [Allocate and initialize the tun_struct LSM state based on the calling
> > >>> process, use this state to label the TUN socket.]
> > >>> 
> > >>> 1. Call security_tun_dev_create() which authorizes the action.
> > >>> 2. Call security_tun_dev_alloc_security() which allocates the
> > >>> tun_struct
> > >>> LSM blob and SELinux sets some internal blob state to record the label
> > >>> of
> > >>> the calling process.
> > >>> 3. Call security_tun_dev_attach() which sets the label of the TUN
> > >>> socket
> > >>> to match the label stored in the tun_struct LSM blob during A2.  No
> > >>> authorization is done at this point since the socket is new/unlabeled.
> > >>> 
> > >>> B. TUNSETIFF (existing, persistent device)
> > >>> 
> > >>> [Relabel the existing tun_struct LSM state based on the calling
> > >>> process,
> > >>> use this state to label the TUN socket.]
> > >>> 
> > >>> 1. Attempt to relabel/reset the tun_struct LSM blob from the currently
> > >>> stored value, set during A2, to the label of the current calling
> > >>> process.
> > >>> *** THIS IS NOT CURRENTLY DONE IN THE RFC PATCH ***
> > >>> 2. Call security_tun_dev_attach() which sets the label of the TUN
> > >>> socket
> > >>> to match the label stored in the tun_struct LSM blob during B1. No
> > >>> authorization is done at this point since the socket is new/unlabeled.
> > >>> 
> > >>> C. TUNSETQUEUE
> > >>> 
> > >>> [Use the existing tun_struct LSM state to label the new TUN socket.]
> > >>> 
> > >>> 1. Call security_tun_dev_attach() which sets the label of the TUN
> > >>> socket
> > >>> to match the label stored in the tun_struct LSM blob set during either
> > >>> A2
> > >>> or B1. No authorization is done at this point since the socket is
> > >>> new/unlabeled.
> > >> 
> > >> Here's what bothers me. libvirt currently opens tun and passes
> > >> fd to qemu. What would prevent qemu from attaching fd using TUNSETQUEUE
> > >> to another device it does not own?
> > > 
> > > True, assuming all the above is correct and that I'm understanding it
> > > correctly (Jason?), we should probably add a new SELinux access control
> > > for
> > > TUNSETQUEUE.
> > 
> > Yes, we need make sure qemu can call TUNSETQUEUE for the device it does
> > not own.
> 
> Meaning can *not* call?

Sorry for not being clear, I mean qemu can call TUNSETQUEUE for the device it 
owns and for the device it does not own, it can't call.
> 
> > > The current DAC code exists in tun_not_capable().

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Paul Moore]

On Wednesday, December 05, 2012 10:01:31 PM Jason Wang wrote:
> On Wednesday, December 05, 2012 01:44:55 PM Michael S. Tsirkin wrote:
> > On Wed, Dec 05, 2012 at 02:19:22PM +0800, Jason Wang wrote:
> > > On 12/05/2012 02:17 AM, Paul Moore wrote:
> > > > On Tuesday, December 04, 2012 07:36:26 PM Michael S. Tsirkin wrote:
> > > >> On Tue, Dec 04, 2012 at 11:18:57AM -0500, Paul Moore wrote:
> > > >>> Okay, based on your explanation of TUNSETQUEUE, the steps below are
> > > >>> what I
> > > >>> believe we need to do ... if you disagree speak up quickly please.
> > > >>> 
> > > >>> A. TUNSETIFF (new, non-persistent device)
> > > >>> 
> > > >>> [Allocate and initialize the tun_struct LSM state based on the
> > > >>> calling
> > > >>> process, use this state to label the TUN socket.]
> > > >>> 
> > > >>> 1. Call security_tun_dev_create() which authorizes the action.
> > > >>> 2. Call security_tun_dev_alloc_security() which allocates the
> > > >>> tun_struct
> > > >>> LSM blob and SELinux sets some internal blob state to record the
> > > >>> label
> > > >>> of
> > > >>> the calling process.
> > > >>> 3. Call security_tun_dev_attach() which sets the label of the TUN
> > > >>> socket
> > > >>> to match the label stored in the tun_struct LSM blob during A2.  No
> > > >>> authorization is done at this point since the socket is
> > > >>> new/unlabeled.
> > > >>> 
> > > >>> B. TUNSETIFF (existing, persistent device)
> > > >>> 
> > > >>> [Relabel the existing tun_struct LSM state based on the calling
> > > >>> process,
> > > >>> use this state to label the TUN socket.]
> > > >>> 
> > > >>> 1. Attempt to relabel/reset the tun_struct LSM blob from the
> > > >>> currently
> > > >>> stored value, set during A2, to the label of the current calling
> > > >>> process.
> > > >>> *** THIS IS NOT CURRENTLY DONE IN THE RFC PATCH ***
> > > >>> 2. Call security_tun_dev_attach() which sets the label of the TUN
> > > >>> socket
> > > >>> to match the label stored in the tun_struct LSM blob during B1. No
> > > >>> authorization is done at this point since the socket is
> > > >>> new/unlabeled.
> > > >>> 
> > > >>> C. TUNSETQUEUE
> > > >>> 
> > > >>> [Use the existing tun_struct LSM state to label the new TUN socket.]
> > > >>> 
> > > >>> 1. Call security_tun_dev_attach() which sets the label of the TUN
> > > >>> socket
> > > >>> to match the label stored in the tun_struct LSM blob set during
> > > >>> either
> > > >>> A2
> > > >>> or B1. No authorization is done at this point since the socket is
> > > >>> new/unlabeled.
> > > >> 
> > > >> Here's what bothers me. libvirt currently opens tun and passes
> > > >> fd to qemu. What would prevent qemu from attaching fd using
> > > >> TUNSETQUEUE
> > > >> to another device it does not own?
> > > > 
> > > > True, assuming all the above is correct and that I'm understanding it
> > > > correctly (Jason?), we should probably add a new SELinux access
> > > > control
> > > > for
> > > > TUNSETQUEUE.
> > > 
> > > Yes, we need make sure qemu can call TUNSETQUEUE for the device it does
> > > not own.
> > 
> > Meaning can *not* call?
> 
> Sorry for not being clear, I mean qemu can call TUNSETQUEUE for the device
> it owns and for the device it does not own, it can't call.

Okay, let me add a access control for TUNSETQUEUE and I'll post an updated 
patchset later today.

-- 
paul moore
security and virtualization @ redhat

Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

[Jason Wang]

On Tuesday, December 04, 2012 11:18:57 AM Paul Moore wrote:
> On Tuesday, December 04, 2012 09:24:43 PM Jason Wang wrote:
> > On Monday, December 03, 2012 11:22:29 AM Paul Moore wrote:
> > > It may be that I'm misunderstanding TUNSETQUEUE and/or TUNSETIFF.  Can
> > > you
> > > elaborate as to why they should be different?
> > 
> > If I understand correctly, before multiqueue patchset, TUNSETIFF is used
> > to:
> > 
> > 1) Create the tun/tap network device
> > 2) For persistent device, re-attach the fd to the network device / socket.
> > In this case, we call selinux_tun_dev_attch() to relabel the socket sid
> > (in
> > fact also the device's since the socket were persistent also) to the sid
> > of
> > process that calls TUNSETIFF.
> > 
> > So, after the changes of multiqueue, we need try to preserve those policy.
> > The interesting part is the introducing of TUNSETQUEUE, it's used to
> > attach
> > more file descriptors/sockets to a tun/tap device after at least one file
> > descriptor were attached to the tun/tap device through TUNSETIFF. So I
> > think maybe we need differ those two ioctls. This patch looks fine for
> > TUNSETQUEUE, but for TUNSETIFF, we need relabel the tunsec to the process
> > that calling TUNSETIFF for persistent device?
> 
> Okay, based on your explanation of TUNSETQUEUE, the steps below are what I
> believe we need to do ... if you disagree speak up quickly please.
> 
> A. TUNSETIFF (new, non-persistent device)
> 
> [Allocate and initialize the tun_struct LSM state based on the calling
> process, use this state to label the TUN socket.]
> 
> 1. Call security_tun_dev_create() which authorizes the action.
> 2. Call security_tun_dev_alloc_security() which allocates the tun_struct LSM
> blob and SELinux sets some internal blob state to record the label of the
> calling process.
> 3. Call security_tun_dev_attach() which sets the label of the TUN socket to
> match the label stored in the tun_struct LSM blob during A2.  No
> authorization is done at this point since the socket is new/unlabeled.
> 
> B. TUNSETIFF (existing, persistent device)
> 
> [Relabel the existing tun_struct LSM state based on the calling process, use
> this state to label the TUN socket.]
> 
> 1. Attempt to relabel/reset the tun_struct LSM blob from the currently
> stored value, set during A2, to the label of the current calling process.
> *** THIS IS NOT CURRENTLY DONE IN THE RFC PATCH ***
> 2. Call security_tun_dev_attach() which sets the label of the TUN socket to
> match the label stored in the tun_struct LSM blob during B1. No
> authorization is done at this point since the socket is new/unlabeled.
> 
> C. TUNSETQUEUE
> 
> [Use the existing tun_struct LSM state to label the new TUN socket.]
> 
> 1. Call security_tun_dev_attach() which sets the label of the TUN socket to
> match the label stored in the tun_struct LSM blob set during either A2 or
> B1. No authorization is done at this point since the socket is
> new/unlabeled.

This looks fine to me.
> > btw. Current code does allow calling TUNSETQUEUE to a persistent tun/tap
> > device with no file attached. It should be a bug and need to be fixed.
> 
> Since you wrote that code will you be submitting a patch to fix that
> problem?

Yes, I will fix it.
> > > One thing that I think we probably should change is the relabelto/from
> > > permissions in the function above (selinux_tun_dev_attach()); in the
> > > case
> > > where the socket does not yet have a label, e.g. 'sksec->sid == 0', we
> > > should probably skip the relabel permissions since we want to assign the
> > > TUN device label regardless in this case.
> > 
> > I'm not familiar with the selinux, have a quick glance of the code, looks
> > like the label has been initialized to SECINITSID_KERNEL in
> > selinux_socket_post_create().
> 
> Unless I've missed something in your changes, the multiqueue code never
> calls any socket code which ends up calling
> {security,selinux}_socket_post_create(); I believe you only call sk_alloc()
> which ends up calling
> {security,selinux}_sk_alloc() which sets SECINITSID_UNLABELED (I mistakenly
> wrote 0 instead in my earlier email which is techincally SECSID_NULL). 
> Either way, I still think the logic I originally described above is
> correct.

Yes, I was wrong. Thanks for the checking.