[PATCH 0/5] tun/macvtap: TUNSETIFF fixes

[PATCH 0/5] tun/macvtap: TUNSETIFF fixes

[Michael S. Tsirkin]

Dan Carpenter reported the following:
	static checker warning:

		drivers/net/tun.c:1694 tun_set_iff()
		warn: 0x17100 is larger than 16 bits

	drivers/net/tun.c
	  1692
	  1693          tun->flags = (tun->flags & ~TUN_FEATURES) |
	  1694                  (ifr->ifr_flags & TUN_FEATURES);
	  1695

	It's complaining because the "ifr->ifr_flags" variable is a short
	(should it be unsigned?).  The new define:

	#define IFF_VNET_LE    0x10000

	doesn't fit in two bytes.  Other suspect looking code could be:

		return __virtio16_to_cpu(q->flags & IFF_VNET_LE, val);

And that's true: we have run out of IFF flags in tun.

So let's not try to add more: add simple GET/SET ioctls
instead. Easy to test, leads to clear semantics.

Alternatively we'll have to revert the whole thing for 3.19,
but that seems more work as this has dependencies
in other places.

While here, I noticed that macvtap was actually reading
ifreq flags as a 32 bit field.
Fix that up as well.

Michael S. Tsirkin (5):
  macvtap: fix uninitialized access on TUNSETIFF
  if_tun: add TUNSETVNETLE/TUNGETVNETLE
  tun: drop broken IFF_VNET_LE
  macvtap: drop broken IFF_VNET_LE
  if_tun: drop broken IFF_VNET_LE

 include/uapi/linux/if_tun.h |  3 ++-
 drivers/net/macvtap.c       | 30 ++++++++++++++++++++++++------
 drivers/net/tun.c           | 26 +++++++++++++++++++++++---
 3 files changed, 49 insertions(+), 10 deletions(-)

-- 
MST

[PATCH 1/5] macvtap: fix uninitialized access on TUNSETIFF

[Michael S. Tsirkin]

flags field in ifreq is only 16 bit wide, but
we read it as a 32 bit value.
If userspace doesn't zero-initialize unused fields,
this will lead to failures.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/macvtap.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index af90ab5..de88285 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -1011,7 +1011,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
 	void __user *argp = (void __user *)arg;
 	struct ifreq __user *ifr = argp;
 	unsigned int __user *up = argp;
-	unsigned int u;
+	unsigned short u;
 	int __user *sp = argp;
 	int s;
 	int ret;
@@ -1026,7 +1026,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
 		if ((u & ~MACVTAP_FEATURES) != (IFF_NO_PI | IFF_TAP))
 			ret = -EINVAL;
 		else
-			q->flags = u;
+			q->flags = (q->flags & ~MACVTAP_FEATURES) | u;
 
 		return ret;
 
@@ -1039,8 +1039,9 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
 		}
 
 		ret = 0;
+		u = q->flags;
 		if (copy_to_user(&ifr->ifr_name, vlan->dev->name, IFNAMSIZ) ||
-		    put_user(q->flags, &ifr->ifr_flags))
+		    put_user(u, &ifr->ifr_flags))
 			ret = -EFAULT;
 		macvtap_put_vlan(vlan);
 		rtnl_unlock();
-- 
MST

[PATCH 2/5] if_tun: add TUNSETVNETLE/TUNGETVNETLE

[Michael S. Tsirkin]

ifreq flags field is only 16 bit wide, so setting IFF_VNET_LE there has
no effect:
doesn't fit in two bytes.

The tests passed apparently because they have an even number of bugs,
all cancelling out.

Luckily we didn't release a kernel with this flag, so it's
not too late to fix this.

Add TUNSETVNETLE/TUNGETVNETLE to really achieve the purpose
of IFF_VNET_LE.

This has an added benefit that if we ever want a BE flag,
we won't have to deal with weird configurations like
setting both LE and BE at the same time.

IFF_VNET_LE will be dropped in a follow-up patch.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/uapi/linux/if_tun.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h
index 18b2403..274630c 100644
--- a/include/uapi/linux/if_tun.h
+++ b/include/uapi/linux/if_tun.h
@@ -48,6 +48,8 @@
 #define TUNSETQUEUE  _IOW('T', 217, int)
 #define TUNSETIFINDEX	_IOW('T', 218, unsigned int)
 #define TUNGETFILTER _IOR('T', 219, struct sock_fprog)
+#define TUNSETVNETLE _IOW('T', 220, int)
+#define TUNGETVNETLE _IOR('T', 221, int)
 
 /* TUNSETIFF ifr flags */
 #define IFF_TUN		0x0001
-- 
MST

[PATCH 3/5] tun: drop broken IFF_VNET_LE

[Michael S. Tsirkin]

Use TUNSETVNETLE/TUNGETVNETLE instead.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/tun.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index c052bd6b..e3e8a0e 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -109,9 +109,11 @@ do {								\
  * overload it to mean fasync when stored there.
  */
 #define TUN_FASYNC	IFF_ATTACH_QUEUE
+/* High bits in flags field are unused. */
+#define TUN_VNET_LE     0x80000000
 
 #define TUN_FEATURES (IFF_NO_PI | IFF_ONE_QUEUE | IFF_VNET_HDR | \
-		      IFF_VNET_LE | IFF_MULTI_QUEUE)
+		      IFF_MULTI_QUEUE)
 #define GOODCOPY_LEN 128
 
 #define FLT_EXACT_COUNT 8
@@ -207,12 +209,12 @@ struct tun_struct {
 
 static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val)
 {
-	return __virtio16_to_cpu(tun->flags & IFF_VNET_LE, val);
+	return __virtio16_to_cpu(tun->flags & TUN_VNET_LE, val);
 }
 
 static inline __virtio16 cpu_to_tun16(struct tun_struct *tun, u16 val)
 {
-	return __cpu_to_virtio16(tun->flags & IFF_VNET_LE, val);
+	return __cpu_to_virtio16(tun->flags & TUN_VNET_LE, val);
 }
 
 static inline u32 tun_hashfn(u32 rxhash)
@@ -1853,6 +1855,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 	int sndbuf;
 	int vnet_hdr_sz;
 	unsigned int ifindex;
+	int le;
 	int ret;
 
 	if (cmd == TUNSETIFF || cmd == TUNSETQUEUE || _IOC_TYPE(cmd) == 0x89) {
@@ -2052,6 +2055,23 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 		tun->vnet_hdr_sz = vnet_hdr_sz;
 		break;
 
+	case TUNGETVNETLE:
+		le = !!(tun->flags & TUN_VNET_LE);
+		if (put_user(le, (int __user *)argp))
+			ret = -EFAULT;
+		break;
+
+	case TUNSETVNETLE:
+		if (get_user(le, (int __user *)argp)) {
+			ret = -EFAULT;
+			break;
+		}
+		if (le)
+			tun->flags |= TUN_VNET_LE;
+		else
+			tun->flags &= ~TUN_VNET_LE;
+		break;
+
 	case TUNATTACHFILTER:
 		/* Can be set only for TAPs */
 		ret = -EINVAL;
-- 
MST

[PATCH 4/5] macvtap: drop broken IFF_VNET_LE

[Michael S. Tsirkin]

Use TUNSETVNETLE/TUNGETVNETLE instead.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/macvtap.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index de88285..f1f0df1 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -45,16 +45,18 @@ struct macvtap_queue {
 	struct list_head next;
 };
 
-#define MACVTAP_FEATURES (IFF_VNET_HDR | IFF_VNET_LE | IFF_MULTI_QUEUE)
+#define MACVTAP_FEATURES (IFF_VNET_HDR | IFF_MULTI_QUEUE)
+
+#define MACVTAP_VNET_LE 0x80000000
 
 static inline u16 macvtap16_to_cpu(struct macvtap_queue *q, __virtio16 val)
 {
-	return __virtio16_to_cpu(q->flags & IFF_VNET_LE, val);
+	return __virtio16_to_cpu(q->flags & MACVTAP_VNET_LE, val);
 }
 
 static inline __virtio16 cpu_to_macvtap16(struct macvtap_queue *q, u16 val)
 {
-	return __cpu_to_virtio16(q->flags & IFF_VNET_LE, val);
+	return __cpu_to_virtio16(q->flags & MACVTAP_VNET_LE, val);
 }
 
 static struct proto macvtap_proto = {
@@ -1082,6 +1084,21 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
 		q->vnet_hdr_sz = s;
 		return 0;
 
+	case TUNGETVNETLE:
+		s = !!(q->flags & MACVTAP_VNET_LE);
+		if (put_user(s, sp))
+			return -EFAULT;
+		return 0;
+
+	case TUNSETVNETLE:
+		if (get_user(s, sp))
+			return -EFAULT;
+		if (s)
+			q->flags |= MACVTAP_VNET_LE;
+		else
+			q->flags &= ~MACVTAP_VNET_LE;
+		return 0;
+
 	case TUNSETOFFLOAD:
 		/* let the user check for future flags */
 		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
-- 
MST

[PATCH 5/5] if_tun: drop broken IFF_VNET_LE

[Michael S. Tsirkin]

Everyone should use TUNSETVNETLE/TUNGETVNETLE instead.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/uapi/linux/if_tun.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h
index 274630c..50ae243 100644
--- a/include/uapi/linux/if_tun.h
+++ b/include/uapi/linux/if_tun.h
@@ -59,7 +59,6 @@
 #define IFF_ONE_QUEUE	0x2000
 #define IFF_VNET_HDR	0x4000
 #define IFF_TUN_EXCL	0x8000
-#define IFF_VNET_LE	0x10000
 #define IFF_MULTI_QUEUE 0x0100
 #define IFF_ATTACH_QUEUE 0x0200
 #define IFF_DETACH_QUEUE 0x0400
-- 
MST

Re: [PATCH 0/5] tun/macvtap: TUNSETIFF fixes

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 16 Dec 2014 15:04:53 +0200

> Dan Carpenter reported the following:
 ...
> And that's true: we have run out of IFF flags in tun.
> 
> So let's not try to add more: add simple GET/SET ioctls
> instead. Easy to test, leads to clear semantics.
> 
> Alternatively we'll have to revert the whole thing for 3.19,
> but that seems more work as this has dependencies
> in other places.
> 
> While here, I noticed that macvtap was actually reading
> ifreq flags as a 32 bit field.
> Fix that up as well.

Looks good, series applied, thanks.

Re: [PATCH 0/5] tun/macvtap: TUNSETIFF fixes

[Jason Wang]

On Tue, Dec 16, 2014 at 9:04 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> Dan Carpenter reported the following:
> 	static checker warning:
> 
> 		drivers/net/tun.c:1694 tun_set_iff()
> 		warn: 0x17100 is larger than 16 bits
> 
> 	drivers/net/tun.c
> 	  1692
> 	  1693          tun->flags = (tun->flags & ~TUN_FEATURES) |
> 	  1694                  (ifr->ifr_flags & TUN_FEATURES);
> 	  1695
> 
> 	It's complaining because the "ifr->ifr_flags" variable is a short
> 	(should it be unsigned?).  The new define:
> 
> 	#define IFF_VNET_LE    0x10000
> 
> 	doesn't fit in two bytes.  Other suspect looking code could be:
> 
> 		return __virtio16_to_cpu(q->flags & IFF_VNET_LE, val);
> 
> And that's true: we have run out of IFF flags in tun.

I don't have objections on this series.
Just note that we still have several bits available.
> 
> So let's not try to add more: add simple GET/SET ioctls
> instead. Easy to test, leads to clear semantics.
> 
> Alternatively we'll have to revert the whole thing for 3.19,
> but that seems more work as this has dependencies
> in other places.
> 
> While here, I noticed that macvtap was actually reading
> ifreq flags as a 32 bit field.
> Fix that up as well.
> 
> Michael S. Tsirkin (5):
>   macvtap: fix uninitialized access on TUNSETIFF
>   if_tun: add TUNSETVNETLE/TUNGETVNETLE
>   tun: drop broken IFF_VNET_LE
>   macvtap: drop broken IFF_VNET_LE
>   if_tun: drop broken IFF_VNET_LE
> 
>  include/uapi/linux/if_tun.h |  3 ++-
>  drivers/net/macvtap.c       | 30 ++++++++++++++++++++++++------
>  drivers/net/tun.c           | 26 +++++++++++++++++++++++---
>  3 files changed, 49 insertions(+), 10 deletions(-)
> 
> -- 
> MST
> 
> --
> 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: [PATCH 3/5] tun: drop broken IFF_VNET_LE

[Jason Wang]

On Tue, Dec 16, 2014 at 9:05 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> Use TUNSETVNETLE/TUNGETVNETLE instead.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/net/tun.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index c052bd6b..e3e8a0e 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -109,9 +109,11 @@ do {								\
>   * overload it to mean fasync when stored there.
>   */
>  #define TUN_FASYNC	IFF_ATTACH_QUEUE
> +/* High bits in flags field are unused. */
> +#define TUN_VNET_LE     0x80000000
>  
>  #define TUN_FEATURES (IFF_NO_PI | IFF_ONE_QUEUE | IFF_VNET_HDR | \
> -		      IFF_VNET_LE | IFF_MULTI_QUEUE)
> +		      IFF_MULTI_QUEUE)
>  #define GOODCOPY_LEN 128
>  
>  #define FLT_EXACT_COUNT 8
> @@ -207,12 +209,12 @@ struct tun_struct {
>  
>  static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 
> val)
>  {
> -	return __virtio16_to_cpu(tun->flags & IFF_VNET_LE, val);
> +	return __virtio16_to_cpu(tun->flags & TUN_VNET_LE, val);
>  }
>  
>  static inline __virtio16 cpu_to_tun16(struct tun_struct *tun, u16 
> val)
>  {
> -	return __cpu_to_virtio16(tun->flags & IFF_VNET_LE, val);
> +	return __cpu_to_virtio16(tun->flags & TUN_VNET_LE, val);
>  }
>  
>  static inline u32 tun_hashfn(u32 rxhash)
> @@ -1853,6 +1855,7 @@ static long __tun_chr_ioctl(struct file *file, 
> unsigned int cmd,
>  	int sndbuf;
>  	int vnet_hdr_sz;
>  	unsigned int ifindex;
> +	int le;
>  	int ret;
>  
>  	if (cmd == TUNSETIFF || cmd == TUNSETQUEUE || _IOC_TYPE(cmd) == 
> 0x89) {
> @@ -2052,6 +2055,23 @@ static long __tun_chr_ioctl(struct file *file, 
> unsigned int cmd,
>  		tun->vnet_hdr_sz = vnet_hdr_sz;
>  		break;
>  
> +	case TUNGETVNETLE:
> +		le = !!(tun->flags & TUN_VNET_LE);
> +		if (put_user(le, (int __user *)argp))
> +			ret = -EFAULT;
> +		break;
> +
> +	case TUNSETVNETLE:
> +		if (get_user(le, (int __user *)argp)) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +		if (le)
> +			tun->flags |= TUN_VNET_LE;
> +		else
> +			tun->flags &= ~TUN_VNET_LE;
> +		break;
> +

A little bit different from persistent devices:

- TUNSETPERSIST check argp instead
- Userspace may check persist flags through TUNGETIFF

Probably this patch may needs more modifications on userspace.
> 
>  	case TUNATTACHFILTER:
>  		/* Can be set only for TAPs */
>  		ret = -EINVAL;
> -- 
> MST
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/