[PATCH v2] macvlan: validate flags

Subsystems: networking drivers, the rest

STALE4785d

6 messages, 2 authors, 2013-08-01 · open the first message on its own page

[PATCH v2] macvlan: validate flags

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2013-08-01 16:08:07 

commit df8ef8f3aaa6692970a436204c4429210addb23a
    macvlan: add FDB bridge ops and macvlan flags
added a flags field to macvlan, which can be
controlled from userspace.
The idea is to make the interface future-proof
so we can add flags and not new fields.

However, flags value isn't validated, as a result,
userspace can't detect which flags are supported.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: John Fastabend <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Changes from v1:
	tweaked commit message
	no code changes

Please consider this patch for -stable.

The idea is by the time we add more flags,
everyone has updated to a kernel that
detects errors, so userspace will be able
to detect supported flags cleanly.


 drivers/net/macvlan.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 18373b6..8445a94 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -736,6 +736,10 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
 			return -EADDRNOTAVAIL;
 	}
 
+	if (data && data[IFLA_MACVLAN_FLAGS] &&
+	    nla_get_u16(data[IFLA_MACVLAN_FLAGS]) & ~MACVLAN_FLAG_NOPROMISC)
+		return -EINVAL;
+
 	if (data && data[IFLA_MACVLAN_MODE]) {
 		switch (nla_get_u32(data[IFLA_MACVLAN_MODE])) {
 		case MACVLAN_MODE_PRIVATE:
@@ -809,6 +813,9 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 	if (data && data[IFLA_MACVLAN_FLAGS])
 		vlan->flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);
 
+	if (vlan->flags & ~MACVLAN_FLAG_NOPROMISC)
+		return -EINVAL;
+
 	if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
 		if (port->count)
 			return -EINVAL;
-- 
MST

Re: [PATCH v2] macvlan: validate flags

From: John Fastabend <hidden>
Date: 2013-08-01 17:24:23 

On 8/1/2013 9:09 AM, Michael S. Tsirkin wrote:
commit df8ef8f3aaa6692970a436204c4429210addb23a
     macvlan: add FDB bridge ops and macvlan flags
added a flags field to macvlan, which can be
controlled from userspace.
The idea is to make the interface future-proof
so we can add flags and not new fields.

However, flags value isn't validated, as a result,
userspace can't detect which flags are supported.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: John Fastabend <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Changes from v1:
	tweaked commit message
	no code changes

Please consider this patch for -stable.

The idea is by the time we add more flags,
everyone has updated to a kernel that
detects errors, so userspace will be able
to detect supported flags cleanly.
Agreed and because we haven't added more flags yet this shouldn't
break uapi. Thanks for catching this.

  drivers/net/macvlan.c | 7 +++++++
  1 file changed, 7 insertions(+)
By the same logic should we also add the check to macvlan_changelink()?
quoted hunk
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 18373b6..8445a94 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -736,6 +736,10 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
  			return -EADDRNOTAVAIL;
  	}

+	if (data && data[IFLA_MACVLAN_FLAGS] &&
+	    nla_get_u16(data[IFLA_MACVLAN_FLAGS]) & ~MACVLAN_FLAG_NOPROMISC)
+		return -EINVAL;
+
  	if (data && data[IFLA_MACVLAN_MODE]) {
  		switch (nla_get_u32(data[IFLA_MACVLAN_MODE])) {
  		case MACVLAN_MODE_PRIVATE:
@@ -809,6 +813,9 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
  	if (data && data[IFLA_MACVLAN_FLAGS])
  		vlan->flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);

+	if (vlan->flags & ~MACVLAN_FLAG_NOPROMISC)
+		return -EINVAL;
+
Is there really a case where newlink is called without first calling
validate? I don't think there is so the snippet here in newlink could
be dropped.

Thanks,
John

Re: [PATCH v2] macvlan: validate flags

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2013-08-01 19:18:28 

On Thu, Aug 01, 2013 at 10:24:19AM -0700, John Fastabend wrote:
On 8/1/2013 9:09 AM, Michael S. Tsirkin wrote:
quoted
commit df8ef8f3aaa6692970a436204c4429210addb23a
    macvlan: add FDB bridge ops and macvlan flags
added a flags field to macvlan, which can be
controlled from userspace.
The idea is to make the interface future-proof
so we can add flags and not new fields.

However, flags value isn't validated, as a result,
userspace can't detect which flags are supported.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: John Fastabend <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Changes from v1:
tweaked commit message
no code changes

Please consider this patch for -stable.

The idea is by the time we add more flags,
everyone has updated to a kernel that
detects errors, so userspace will be able
to detect supported flags cleanly.
Agreed and because we haven't added more flags yet this shouldn't
break uapi. Thanks for catching this.
quoted
 drivers/net/macvlan.c | 7 +++++++
 1 file changed, 7 insertions(+)
By the same logic should we also add the check to macvlan_changelink()?
I'm not sure what do you mean "By the same logic" -
macvlan_changelink is static unlike macvlan_common_newlink
which is exported to modules.
So why isn't macvlan_validate sufficient for macvlan_changelink?
quoted
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 18373b6..8445a94 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -736,6 +736,10 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
 			return -EADDRNOTAVAIL;
 	}

+	if (data && data[IFLA_MACVLAN_FLAGS] &&
+	    nla_get_u16(data[IFLA_MACVLAN_FLAGS]) & ~MACVLAN_FLAG_NOPROMISC)
+		return -EINVAL;
+
 	if (data && data[IFLA_MACVLAN_MODE]) {
 		switch (nla_get_u32(data[IFLA_MACVLAN_MODE])) {
 		case MACVLAN_MODE_PRIVATE:
@@ -809,6 +813,9 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 	if (data && data[IFLA_MACVLAN_FLAGS])
 		vlan->flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);

+	if (vlan->flags & ~MACVLAN_FLAG_NOPROMISC)
+		return -EINVAL;
+
Is there really a case where newlink is called without first calling
validate? I don't think there is so the snippet here in newlink could
be dropped.

Thanks,
John
It seems so - macvtap_newlink calls macvlan_common_newlink.
macvtap does not seem to have .validate.

Re: [PATCH v2] macvlan: validate flags

From: John Fastabend <hidden>
Date: 2013-08-01 19:34:27 

On 8/1/2013 12:19 PM, Michael S. Tsirkin wrote:
On Thu, Aug 01, 2013 at 10:24:19AM -0700, John Fastabend wrote:
quoted
On 8/1/2013 9:09 AM, Michael S. Tsirkin wrote:
quoted
commit df8ef8f3aaa6692970a436204c4429210addb23a
     macvlan: add FDB bridge ops and macvlan flags
added a flags field to macvlan, which can be
controlled from userspace.
The idea is to make the interface future-proof
so we can add flags and not new fields.

However, flags value isn't validated, as a result,
userspace can't detect which flags are supported.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: John Fastabend <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Changes from v1:
	tweaked commit message
	no code changes

Please consider this patch for -stable.

The idea is by the time we add more flags,
everyone has updated to a kernel that
detects errors, so userspace will be able
to detect supported flags cleanly.
Agreed and because we haven't added more flags yet this shouldn't
break uapi. Thanks for catching this.
quoted
  drivers/net/macvlan.c | 7 +++++++
  1 file changed, 7 insertions(+)
By the same logic should we also add the check to macvlan_changelink()?
I'm not sure what do you mean "By the same logic" -
macvlan_changelink is static unlike macvlan_common_newlink
which is exported to modules.
"By the same logic" I only meant to allow userspace to cleanly detect
supported flags even in the changelink case.

So why isn't macvlan_validate sufficient for macvlan_changelink?
It is you are correct.

quoted
quoted
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 18373b6..8445a94 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -736,6 +736,10 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
  			return -EADDRNOTAVAIL;
  	}

+	if (data && data[IFLA_MACVLAN_FLAGS] &&
+	    nla_get_u16(data[IFLA_MACVLAN_FLAGS]) & ~MACVLAN_FLAG_NOPROMISC)
+		return -EINVAL;
+
  	if (data && data[IFLA_MACVLAN_MODE]) {
  		switch (nla_get_u32(data[IFLA_MACVLAN_MODE])) {
  		case MACVLAN_MODE_PRIVATE:
@@ -809,6 +813,9 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
  	if (data && data[IFLA_MACVLAN_FLAGS])
  		vlan->flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);

+	if (vlan->flags & ~MACVLAN_FLAG_NOPROMISC)
+		return -EINVAL;
+
Is there really a case where newlink is called without first calling
validate? I don't think there is so the snippet here in newlink could
be dropped.

Thanks,
John
It seems so - macvtap_newlink calls macvlan_common_newlink.
macvtap does not seem to have .validate.
but it calls macvlan_link_register() from macvtap_init which sets
up the validate ops,

int macvlan_link_register(struct rtnl_link_ops *ops)
{
         /* common fields */
         ops->priv_size          = sizeof(struct macvlan_dev);
         ops->validate           = macvlan_validate





  

    
Re: [PATCH v2] macvlan: validate flags

    

      From: John Fastabend <hidden>

      Date: 2013-08-01 19:34:32 
    

  

  
On 8/1/2013 12:19 PM, Michael S. Tsirkin wrote:
On Thu, Aug 01, 2013 at 10:24:19AM -0700, John Fastabend wrote:
quoted
On 8/1/2013 9:09 AM, Michael S. Tsirkin wrote:
quoted
commit df8ef8f3aaa6692970a436204c4429210addb23a
     macvlan: add FDB bridge ops and macvlan flags
added a flags field to macvlan, which can be
controlled from userspace.
The idea is to make the interface future-proof
so we can add flags and not new fields.

However, flags value isn't validated, as a result,
userspace can't detect which flags are supported.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: John Fastabend <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Changes from v1:
	tweaked commit message
	no code changes

Please consider this patch for -stable.

The idea is by the time we add more flags,
everyone has updated to a kernel that
detects errors, so userspace will be able
to detect supported flags cleanly.
Agreed and because we haven't added more flags yet this shouldn't
break uapi. Thanks for catching this.

quoted
  drivers/net/macvlan.c | 7 +++++++
  1 file changed, 7 insertions(+)
By the same logic should we also add the check to macvlan_changelink()?
I'm not sure what do you mean "By the same logic" -
macvlan_changelink is static unlike macvlan_common_newlink
which is exported to modules.
"By the same logic" I only meant to allow userspace to cleanly detect
supported flags even in the changelink case.

So why isn't macvlan_validate sufficient for macvlan_changelink?
It is you are correct.

quoted
quoted
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 18373b6..8445a94 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -736,6 +736,10 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
  			return -EADDRNOTAVAIL;
  	}

+	if (data && data[IFLA_MACVLAN_FLAGS] &&
+	    nla_get_u16(data[IFLA_MACVLAN_FLAGS]) & ~MACVLAN_FLAG_NOPROMISC)
+		return -EINVAL;
+
  	if (data && data[IFLA_MACVLAN_MODE]) {
  		switch (nla_get_u32(data[IFLA_MACVLAN_MODE])) {
  		case MACVLAN_MODE_PRIVATE:
@@ -809,6 +813,9 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
  	if (data && data[IFLA_MACVLAN_FLAGS])
  		vlan->flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);

+	if (vlan->flags & ~MACVLAN_FLAG_NOPROMISC)
+		return -EINVAL;
+
Is there really a case where newlink is called without first calling
validate? I don't think there is so the snippet here in newlink could
be dropped.

Thanks,
John
It seems so - macvtap_newlink calls macvlan_common_newlink.
macvtap does not seem to have .validate.
but it calls macvlan_link_register() from macvtap_init which sets
up the validate ops,

int macvlan_link_register(struct rtnl_link_ops *ops)
{
         /* common fields */
         ops->priv_size          = sizeof(struct macvlan_dev);
         ops->validate           = macvlan_validate





  

    
Re: [PATCH v2] macvlan: validate flags

    

      From: "Michael S. Tsirkin" <mst@redhat.com>

      Date: 2013-08-01 19:42:16 
    

  

  
On Thu, Aug 01, 2013 at 12:34:24PM -0700, John Fastabend wrote:
On 8/1/2013 12:19 PM, Michael S. Tsirkin wrote:
quoted
On Thu, Aug 01, 2013 at 10:24:19AM -0700, John Fastabend wrote:
quoted
On 8/1/2013 9:09 AM, Michael S. Tsirkin wrote:
quoted
commit df8ef8f3aaa6692970a436204c4429210addb23a
    macvlan: add FDB bridge ops and macvlan flags
added a flags field to macvlan, which can be
controlled from userspace.
The idea is to make the interface future-proof
so we can add flags and not new fields.

However, flags value isn't validated, as a result,
userspace can't detect which flags are supported.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: John Fastabend <redacted>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Changes from v1:
tweaked commit message
no code changes

Please consider this patch for -stable.

The idea is by the time we add more flags,
everyone has updated to a kernel that
detects errors, so userspace will be able
to detect supported flags cleanly.
Agreed and because we haven't added more flags yet this shouldn't
break uapi. Thanks for catching this.

quoted
 drivers/net/macvlan.c | 7 +++++++
 1 file changed, 7 insertions(+)
By the same logic should we also add the check to macvlan_changelink()?
I'm not sure what do you mean "By the same logic" -
macvlan_changelink is static unlike macvlan_common_newlink
which is exported to modules.
"By the same logic" I only meant to allow userspace to cleanly detect
supported flags even in the changelink case.

quoted
So why isn't macvlan_validate sufficient for macvlan_changelink?
It is you are correct.

quoted
quoted
quoted
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 18373b6..8445a94 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -736,6 +736,10 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
 			return -EADDRNOTAVAIL;
 	}

+	if (data && data[IFLA_MACVLAN_FLAGS] &&
+	    nla_get_u16(data[IFLA_MACVLAN_FLAGS]) & ~MACVLAN_FLAG_NOPROMISC)
+		return -EINVAL;
+
 	if (data && data[IFLA_MACVLAN_MODE]) {
 		switch (nla_get_u32(data[IFLA_MACVLAN_MODE])) {
 		case MACVLAN_MODE_PRIVATE:
@@ -809,6 +813,9 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 	if (data && data[IFLA_MACVLAN_FLAGS])
 		vlan->flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);

+	if (vlan->flags & ~MACVLAN_FLAG_NOPROMISC)
+		return -EINVAL;
+
Is there really a case where newlink is called without first calling
validate? I don't think there is so the snippet here in newlink could
be dropped.

Thanks,
John
It seems so - macvtap_newlink calls macvlan_common_newlink.
macvtap does not seem to have .validate.
but it calls macvlan_link_register() from macvtap_init which sets
up the validate ops,

int macvlan_link_register(struct rtnl_link_ops *ops)
{
        /* common fields */
        ops->priv_size          = sizeof(struct macvlan_dev);
        ops->validate           = macvlan_validate
I see. I'll drop this second chunk in the patch,
thanks for catching this.

-- 
MST




    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help